DAOS-13194 gurt: environment APIs hook #12220

bfaccini · 2023-05-26T11:09:52Z

In order to prevent known race to occur due to lack of locking in Glibc environment APIs (getenv()/[uns]setenv()/ putenv()/clearenv()), they have been overloaded and strengthened in Gurt with hooks now all using a common lock/mutex.

Libgurt is the preferred place for this as it is the lowest layer in DAOS, so it will be the earliest to be loaded and will ensure the hook to be installed as early as possible and could prevent usage of LD_PRELOAD.

This will address the main lack of multi-thread protection in the Glibc APIs but do not handle all unsafe use-cases (like the change/removal of an env var when its value address has already been grabbed by a previous getenv(), ...).

Required-githooks: true

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

In order to prevent known race to occur due to lack of locking in Glibc environment APIs (getenv()/[uns]setenv()/ putenv()/clearenv()), they have been overloaded and strengthened in Gurt with hooks now all using a common lock/mutex. Libgurt is the preferred place for this as it is the lowest layer in DAOS, so it will be the earliest to be loaded and will ensure the hook to be installed as early as possible and could prevent usage of LD_PRELOAD. This will address the main lack of multi-thread protection in the Glibc APIs but do not handle all unsafe use-cases (like the change/removal of an env var when its value address has already been grabbed by a previous getenv(), ...). Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

github-actions · 2023-05-26T11:10:15Z

Bug-tracker data:
Ticket title is 'frontera: ior segfault when starting read phase'
Status is 'In Review'
Labels: 'TACC_Frontera'
https://daosio.atlassian.net/browse/DAOS-13194

daosbuild1

Style warning(s) for job https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-12220/1/
Please review https://wiki.hpdd.intel.com/display/DC/Coding+Rules

src/gurt/misc.c

daosbuild1 · 2023-05-26T11:11:30Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12220/1/execution/node/145/log

I had forgotten to remove debug fprintf()s to stderr, and there was some cosmetic fixes requested by our set of code beautifiers... Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

Updated patch

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-05-26T15:29:49Z

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12220/2/execution/node/1015/log

Unexpectedly Glibc seems not be already/early binded upon some commands start, so try to dlopen() it !!... Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-05-30T15:02:17Z

Test stage Build RPM on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12220/3/execution/node/301/log

daosbuild1 · 2023-05-30T15:02:49Z

Test stage Build RPM on Leap 15.4 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12220/3/execution/node/298/log

daosbuild1 · 2023-05-30T15:13:01Z

Test stage Build DEB on Ubuntu 20.04 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12220/3/execution/node/341/log

daosbuild1 · 2023-05-30T15:16:55Z

Test stage Build on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12220/3/execution/node/442/log

All is in the title !!... Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

Upon 2nd try to bind Glibc symbol, use the handle we got from dlopen() !! Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-05-31T12:50:47Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12220/5/execution/node/1209/log

bfaccini · 2023-05-31T15:22:22Z

The 3 functional tests failures seems to be related to DAOS-12883/DAOS-13488 ...

frostedcmos · 2023-05-31T15:46:07Z

src/gurt/misc.c

+	D_MUTEX_LOCK(&hook_env_lock);
+	if (real_getenv == NULL) {
+		real_getenv = (char * (*)(const char *))dlsym(RTLD_NEXT, "getenv");
+		if (real_getenv == NULL) {
+			/* Glibc symbols could not be resolved !!... */
+			void *handle;
+
+			handle = dlopen("libc.so.6", RTLD_LAZY);
+			D_ASSERT(handle != NULL);
+			real_getenv = (char * (*)(const char *))dlsym(handle, "getenv");
+		}
+		D_ASSERT(real_getenv != NULL);
+	}
+
+	p = real_getenv(name);
+	D_MUTEX_UNLOCK(&hook_env_lock);


Seems like this block is repeated across the code 5 times, once for each call, with the only difference being the symbol being looked up and used. Can this be simplified to use a shared function across those calls instead?
Something like

char *getenv(const char *name) { return shared_call(&real_getenv, "getenv"); } char *setenv(const char *name) { return shared_call(&real_setenv, "setenv"); }

etc...

Ok, you are right!

jolivier23 · 2023-05-31T19:19:10Z

src/gurt/misc.c

+static int (*real_unsetenv)(const char *);
+static int (*real_clearenv)(void);
+
+char *getenv(const char *name)


the question is I have here is whether this actually works in practice. Do calls to getenv from dependent libraries such as libfabric get intercepted by hooks in libgurt?

As I have already indicated in DAOS-13194 JiRA/ticket :

I have manually verified that this approach works for the daos/dfuse/dmg/daos_agent/daos_server/daos_engine/daos_server_helper binaries.

jolivier23 · 2023-05-31T19:20:03Z

src/gurt/misc.c

+
+	D_MUTEX_LOCK(&hook_env_lock);
+	if (real_getenv == NULL) {
+		real_getenv = (char * (*)(const char *))dlsym(RTLD_NEXT, "getenv");


dlsym returns a void *. You don't need to cast it

Ah... and do you know where does this rule come from ? Early Clang definitions, Kernel developers, ...??

void * will alias to anything, it never needs a cast.

jolivier23 · 2023-05-31T19:20:59Z

src/gurt/misc.c

+{
+	int rc;
+
+	D_MUTEX_LOCK(&hook_env_lock);


I might suggest using pthread_once and initializing all of the hooks at once?

Ah yes, you are right !!

jolivier23 · 2023-05-31T19:22:18Z

src/gurt/misc.c

+		D_ASSERT(real_getenv != NULL);
+	}
+
+	p = real_getenv(name);


If you separate init from this, the lock here should really only protect the real call.

If you separate init from this, the lock here should really only protect the real call.

Well, initialisation of the "real_..." variables is racy too, but may be I can switch to using "atomic" variables then ?!

Finally, I have switched to using Atomics for the initialisation part and also switched to rw-lock usage (instead of simple mutex, mainly to allow concurrent getenv()s) for the exception part.

ashleypittman · 2023-06-19T10:46:38Z

This has a conflict.

To fix conflicts !!... Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

bfaccini · 2023-06-20T09:06:09Z

This has a conflict.

Ok, just merged with latest master ... really sorry for my reviewers :-(

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-06-20T14:43:42Z

Test stage Unit Test on EL 8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12220/16/testReport/

mchaarawi

PR has not passed unit test stage.
please rebase with master and repush

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-07-10T17:28:09Z

Test stage Unit Test on EL 8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12220/17/testReport/

No comment ....... Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

bfaccini · 2023-07-13T08:00:28Z

After a merge with latest master, a forced test rerun, a new merge with latest master, I have been able to get 2 consecutive and successful CI sessions with no more apparently unrelated unit-tests errors (due to "nvme_control_ctests" unexpectedly missing) !!...

In order to prevent known race to occur due to lack of locking in Glibc environment APIs (getenv()/[uns]setenv()/ putenv()/clearenv()), they have been overloaded and strengthened in Gurt with hooks now all using a common lock/mutex. Libgurt is the preferred place for this as it is the lowest layer in DAOS, so it will be the earliest to be loaded and will ensure the hook to be installed as early as possible and could prevent usage of LD_PRELOAD. This will address the main lack of multi-thread protection in the Glibc APIs but do not handle all unsafe use-cases (like the change/removal of an env var when its value address has already been grabbed by a previous getenv(), ...). Change-Id: I38cda09746ddb4e79f0297fee26c2a22e1cb881b Signed-off-by: Bruno Faccini <[email protected]>

In order to prevent known race to occur due to lack of locking in Glibc environment APIs (getenv()/[uns]setenv()/ putenv()/clearenv()), they have been overloaded and strengthened in Gurt with hooks now all using a common lock/mutex. Libgurt is the preferred place for this as it is the lowest layer in DAOS, so it will be the earliest to be loaded and will ensure the hook to be installed as early as possible and could prevent usage of LD_PRELOAD. This will address the main lack of multi-thread protection in the Glibc APIs but do not handle all unsafe use-cases (like the change/removal of an env var when its value address has already been grabbed by a previous getenv(), ...). Signed-off-by: Bruno Faccini <[email protected]>

In order to prevent known race to occur due to lack of locking in Glibc environment APIs (getenv()/[uns]setenv()/ putenv()/clearenv()), they have been overloaded and strengthened in Gurt with hooks now all using a common lock/mutex. Libgurt is the preferred place for this as it is the lowest layer in DAOS, so it will be the earliest to be loaded and will ensure the hook to be installed as early as possible and could prevent usage of LD_PRELOAD. This will address the main lack of multi-thread protection in the Glibc APIs but do not handle all unsafe use-cases (like the change/removal of an env var when its value address has already been grabbed by a previous getenv(), ...). Change-Id: I38cda09746ddb4e79f0297fee26c2a22e1cb881b Signed-off-by: Bruno Faccini <[email protected]>

bfaccini self-assigned this May 26, 2023

github-actions bot added priority Ticket has high priority (automatically managed) release-2.4 PR is eventually targeted for 2.4 labels May 26, 2023

daosbuild1 previously requested changes May 26, 2023

View reviewed changes

DAOS-13194 gurt: remove debug prints and some cosmetics

51f5789

I had forgotten to remove debug fprintf()s to stderr, and there was some cosmetic fixes requested by our set of code beautifiers... Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1 reviewed May 26, 2023

View reviewed changes

DAOS-13194 gurt: retry with dlopen() to bind Glibc

6deb540

Unexpectedly Glibc seems not be already/early binded upon some commands start, so try to dlopen() it !!... Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1 reviewed May 30, 2023

View reviewed changes

DAOS-13194 gurt: fix errors in previous commit

cdaf9ec

All is in the title !!... Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1 reviewed May 31, 2023

View reviewed changes

DAOS-13194 gurt: use new handle instead of RTLD_NEXT

e0ff359

Upon 2nd try to bind Glibc symbol, use the handle we got from dlopen() !! Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1 reviewed May 31, 2023

View reviewed changes

bfaccini requested review from jolivier23 and frostedcmos May 31, 2023 09:25

frostedcmos reviewed May 31, 2023

View reviewed changes

jolivier23 reviewed May 31, 2023

View reviewed changes

bfaccini requested a review from a team June 19, 2023 09:03

Merge branch 'master' into bfaccini/DAOS-13194_2nd

36e5b40

To fix conflicts !!... Required-githooks: true Signed-off-by: Bruno Faccini <[email protected]>

bfaccini dismissed stale reviews from jolivier23 and soumagne via 36e5b40 June 20, 2023 09:04

bfaccini requested review from soumagne and jolivier23 June 20, 2023 09:05

ashleypittman approved these changes Jun 20, 2023

View reviewed changes

daosbuild1 reviewed Jun 20, 2023

View reviewed changes

frostedcmos approved these changes Jun 20, 2023

View reviewed changes

bfaccini requested a review from a team June 29, 2023 05:20

mchaarawi reviewed Jul 10, 2023

View reviewed changes

Merge branch 'master' into bfaccini/DAOS-13194_2nd

a57c96a

bfaccini requested a review from mchaarawi July 10, 2023 16:59

daosbuild1 reviewed Jul 10, 2023

View reviewed changes

Merge branch 'master' into bfaccini/DAOS-13194_2nd

8838d3e

No comment ....... Signed-off-by: Bruno Faccini <[email protected]>

daosbuild1 reviewed Jul 12, 2023

View reviewed changes

bfaccini requested a review from a team July 13, 2023 07:54

ashleypittman merged commit 7d2bc9b into master Jul 13, 2023

ashleypittman deleted the bfaccini/DAOS-13194_2nd branch July 13, 2023 08:06

mjmac mentioned this pull request Jan 2, 2024

mjmac/dstress #13552

Closed

DAOS-13194 gurt: environment APIs hook #12220

DAOS-13194 gurt: environment APIs hook #12220

Conversation

bfaccini commented May 26, 2023

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented May 26, 2023 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented May 26, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented May 26, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented May 30, 2023

daosbuild1 commented May 30, 2023

daosbuild1 commented May 30, 2023

daosbuild1 commented May 30, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented May 31, 2023

bfaccini commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleypittman commented Jun 19, 2023

bfaccini commented Jun 20, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jun 20, 2023

mchaarawi left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jul 10, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

bfaccini commented Jul 13, 2023

github-actions bot commented May 26, 2023 •

edited

Loading