| Message ID | 20250122035359.251194-1-colyli@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] mdopen: add sbin path to env PATH when call system("modprobe md_mod") | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_14-PR | fail | merge-conflict |
Hi Coly, I read that once again and I have more comments. Even if it looks simple, we are calling many env commands in mdadm, that is why I'm trying to be careful. On Wed, 22 Jan 2025 11:53:59 +0800 Coly Li <colyli@suse.de> wrote: > During the boot process if mdadm is called in udev context, sbin paths > like /sbin, /usr/sbin, /usr/local/sbin normally not defined in PATH normally defined? Please remove "not". > env variable, calling system("modprobe md_mod") in > create_named_array() may fail with 'sh: modprobe: command not found' > error message. > > We don't want to move modprobe binary into udev private directory, so > setting the PATH env is a more proper method to avoid the above issue. Curios, did you verified what is happening to our "systemctl" calls? mdmon and grow-continue are started this way, they are later followed by "WANTS=" in udev rule so the issue there is probably hidden, maybe we should fix these calls too? > > This patch sets PATH env variable with > "/sbin:/usr/sbin:/usr/local/sbin" before calling system("modprobe > md_mod"). The change only takes effect within the udev worker > context, not seen by global udev environment. If we are running app from terminal (i.e mdadm -I, or ./mdadm -I) this change should not affect the terminal environment. I verified it to be sure. Could you please mention that in description? > > Signed-off-by: Coly Li <colyli@suse.de> > --- > Changelog, > v2: set buf[PATH_MAX] to 0 in stack variable announcement. > v1: the original version. > > > mdopen.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/mdopen.c b/mdopen.c > index 26f0c716..65bd8a1b 100644 > --- a/mdopen.c > +++ b/mdopen.c > @@ -39,6 +39,17 @@ int create_named_array(char *devnm) > > fd = open(new_array_file, O_WRONLY); > if (fd < 0 && errno == ENOENT) { > + char buf[PATH_MAX] = {0}; > + > + /* > + * When called by udev worker context, path of > modprobe > + * might not be in env PATH. Set sbin paths into PATH > + * env to avoid potential failure when run modprobe > here. > + */ > + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"), > + "/sbin:/usr/sbin:/usr/local/sbin"); We can get NULL returned by getenv("PATH"), should we handle it? We probably rely on compiler behavior here. I did simple test. I tried: printf("%s\n", getenv("NOT_EXISTING")); I got segmentation fault. > + setenv("PATH", buf, 1); I see here portability issues. We, assume that these binaries must be in locations we added here. We may even double them, if they are already defined. Even if I know that probably no one is enough brave to not have base binaries there, we should not force our PATH. I think it is not our task and responsibility to deal with binaries location issues. We should take what system provided. I still think that we should pass locations during compilation. Here example with EXTRAVERSION, of course it may require some adjustments but it is generally the way it can be achieved: https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=03ab9763f51ddf2030f60f83e76cf9c1b50b726c I'm not strong convinced to the option I proposed, I just need argument because more or less code is not an argument. What we will choose today will stay here for years, we need to choose the best possible way we see. > + > if (system("modprobe md_mod") == 0) > fd = open(new_array_file, O_WRONLY); > } The change will affect code executed later, probably we don't want that. Shouldn't we restore old PATH here to minimize risk? Thanks, Mariusz
> 2025年1月22日 20:01，Mariusz Tkaczyk <mtkaczyk@kernel.org> 写道： > > Hi Coly, > I read that once again and I have more comments. Even if it > looks simple, we are calling many env commands in mdadm, that is why I'm > trying to be careful. > > On Wed, 22 Jan 2025 11:53:59 +0800 > Coly Li <colyli@suse.de> wrote: > >> During the boot process if mdadm is called in udev context, sbin paths >> like /sbin, /usr/sbin, /usr/local/sbin normally not defined in PATH > > normally defined? Please remove "not". This is correct, normally NOT defined. In boot time udev tasks just normally called binaries in their private directory, they don’t call system binaries. > >> env variable, calling system("modprobe md_mod") in >> create_named_array() may fail with 'sh: modprobe: command not found' >> error message. >> >> We don't want to move modprobe binary into udev private directory, so >> setting the PATH env is a more proper method to avoid the above issue. > > Curios, did you verified what is happening to our "systemctl" calls? > > mdmon and grow-continue are started this way, they are later followed by > "WANTS=" in udev rule so the issue there is probably hidden, maybe we > should fix these calls too? For this specific case, md kernel module was not loaded yet, it was in quite early stage from observation of me and bug reporter. > >> >> This patch sets PATH env variable with >> "/sbin:/usr/sbin:/usr/local/sbin" before calling system("modprobe >> md_mod"). The change only takes effect within the udev worker >> context, not seen by global udev environment. > > If we are running app from terminal (i.e mdadm -I, or ./mdadm -I) this > change should not affect the terminal environment. I verified it to be > sure. Could you please mention that in description? > OK, let me do it in next version. >> >> Signed-off-by: Coly Li <colyli@suse.de> >> --- >> Changelog, >> v2: set buf[PATH_MAX] to 0 in stack variable announcement. >> v1: the original version. >> >> >> mdopen.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/mdopen.c b/mdopen.c >> index 26f0c716..65bd8a1b 100644 >> --- a/mdopen.c >> +++ b/mdopen.c >> @@ -39,6 +39,17 @@ int create_named_array(char *devnm) >> >> fd = open(new_array_file, O_WRONLY); >> if (fd < 0 && errno == ENOENT) { >> + char buf[PATH_MAX] = {0}; >> + >> + /* >> + * When called by udev worker context, path of >> modprobe >> + * might not be in env PATH. Set sbin paths into PATH >> + * env to avoid potential failure when run modprobe >> here. >> + */ >> + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"), >> + "/sbin:/usr/sbin:/usr/local/sbin"); > > We can get NULL returned by getenv("PATH"), should we handle it? We > probably rely on compiler behavior here. > > I did simple test. I tried: printf("%s\n", getenv("NOT_EXISTING")); > I got segmentation fault. Yes, this was my fault, I took it for granted that PATH should always be set in udev context. You are right, I will add a NULL check in next version. > >> + setenv("PATH", buf, 1); > > I see here portability issues. We, assume that these binaries must be > in locations we added here. We may even double them, if they are > already defined. > Even if I know that probably no one is enough brave to > not have base binaries there, we should not force our PATH. I think it > is not our task and responsibility to deal with binaries location > issues. We should take what system provided. > > I still think that we should pass locations during compilation. Here > example with EXTRAVERSION, of course it may require some adjustments > but it is generally the way it can be achieved: > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=03ab9763f51ddf2030f60f83e76cf9c1b50b726c > > I'm not strong convinced to the option I proposed, I just need > argument because more or less code is not an argument. What we will > choose today will stay here for years, we need to choose the best > possible way we see. The installation path might vary, depends on the way how mdadm is installed. If from rpm or other installation pack, the dest location can be predicted. If mdadm is installed from source code compiling, the destination varies depends on the pre-defined installation location, similar situation happens in containers. So the patch is just a best-effort-try, if the binary is not installed in /sbin, /usr/sbin or /usr/local/sbin, my patch just gives up. > >> + >> if (system("modprobe md_mod") == 0) >> fd = open(new_array_file, O_WRONLY); > >> } > > The change will affect code executed later, probably we don't want > that. Shouldn't we restore old PATH here to minimize risk? For my understanding if the code was called after boot up, these path should be set already by shell initialization scripts. And for udev context, it is called and exited, and NOT shared with other udev tasks, almost no influence. Thanks for your detailed comments. Coly Li
On Wed, 22 Jan 2025 20:43:19 +0800 Coly Li <colyli@suse.de> wrote: > > 2025年1月22日 20:01，Mariusz Tkaczyk <mtkaczyk@kernel.org> 写道： > > > > Hi Coly, > > I read that once again and I have more comments. Even if it > > looks simple, we are calling many env commands in mdadm, that is > > why I'm trying to be careful. > > > > On Wed, 22 Jan 2025 11:53:59 +0800 > > Coly Li <colyli@suse.de> wrote: > > > >> During the boot process if mdadm is called in udev context, sbin > >> paths like /sbin, /usr/sbin, /usr/local/sbin normally not defined > >> in PATH > > > > normally defined? Please remove "not". > > This is correct, normally NOT defined. In boot time udev tasks just > normally called binaries in their private directory, they don’t call > system binaries. > Oh, got you. Thanks! > > > >> env variable, calling system("modprobe md_mod") in > >> create_named_array() may fail with 'sh: modprobe: command not > >> found' error message. > >> > >> We don't want to move modprobe binary into udev private directory, > >> so setting the PATH env is a more proper method to avoid the above > >> issue. > > > > Curios, did you verified what is happening to our "systemctl" calls? > > > > mdmon and grow-continue are started this way, they are later > > followed by "WANTS=" in udev rule so the issue there is probably > > hidden, maybe we should fix these calls too? > > For this specific case, md kernel module was not loaded yet, it was > in quite early stage from observation of me and bug reporter. > > > > > >> > >> This patch sets PATH env variable with > >> "/sbin:/usr/sbin:/usr/local/sbin" before calling system("modprobe > >> md_mod"). The change only takes effect within the udev worker > >> context, not seen by global udev environment. > > > > If we are running app from terminal (i.e mdadm -I, or ./mdadm -I) > > this change should not affect the terminal environment. I verified > > it to be sure. Could you please mention that in description? > > > > OK, let me do it in next version. > > >> > >> Signed-off-by: Coly Li <colyli@suse.de> > >> --- > >> Changelog, > >> v2: set buf[PATH_MAX] to 0 in stack variable announcement. > >> v1: the original version. > >> > >> > >> mdopen.c | 11 +++++++++++ > >> 1 file changed, 11 insertions(+) > >> > >> diff --git a/mdopen.c b/mdopen.c > >> index 26f0c716..65bd8a1b 100644 > >> --- a/mdopen.c > >> +++ b/mdopen.c > >> @@ -39,6 +39,17 @@ int create_named_array(char *devnm) > >> > >> fd = open(new_array_file, O_WRONLY); > >> if (fd < 0 && errno == ENOENT) { > >> + char buf[PATH_MAX] = {0}; > >> + > >> + /* > >> + * When called by udev worker context, path of > >> modprobe > >> + * might not be in env PATH. Set sbin paths into PATH > >> + * env to avoid potential failure when run modprobe > >> here. > >> + */ > >> + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"), > >> + "/sbin:/usr/sbin:/usr/local/sbin"); > > > > We can get NULL returned by getenv("PATH"), should we handle it? We > > probably rely on compiler behavior here. > > > > I did simple test. I tried: printf("%s\n", getenv("NOT_EXISTING")); > > I got segmentation fault. > > Yes, this was my fault, I took it for granted that PATH should always > be set in udev context. > > You are right, I will add a NULL check in next version. > > > > >> + setenv("PATH", buf, 1); > > > > I see here portability issues. We, assume that these binaries must > > be in locations we added here. We may even double them, if they are > > already defined. > > Even if I know that probably no one is enough brave to > > not have base binaries there, we should not force our PATH. I think > > it is not our task and responsibility to deal with binaries location > > issues. We should take what system provided. > > > > I still think that we should pass locations during compilation. Here > > example with EXTRAVERSION, of course it may require some adjustments > > but it is generally the way it can be achieved: > > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=03ab9763f51ddf2030f60f83e76cf9c1b50b726c > > > > I'm not strong convinced to the option I proposed, I just need > > argument because more or less code is not an argument. What we will > > choose today will stay here for years, we need to choose the best > > possible way we see. > > The installation path might vary, depends on the way how mdadm is > installed. If from rpm or other installation pack, the dest location > can be predicted. If mdadm is installed from source code compiling, > the destination varies depends on the pre-defined installation > location, similar situation happens in containers. This is what I wanted to propose: in makefile: # If provided respect that, otherwise, search for it MODPROBE_PATH ?= $(which modprobe) DMODPROBE_PATH = $(if $(MODPROBE_PATH),-DMODPROBE_PATH="\" - $(MODPROBE_PATH)\"",) +CFLAGS += $(DVERS) $(DDATE) $(DEXTRAVERSION) $(DMODPROBE_PATH) and finally in code: if (system(MODPROBE_PATH " md_mod") with that we would detect location of modprobe during compilation or rpm building, or allow user to customize it. It assumes that location of modprobe won't change. I checked whether it is provided by pkg-config with no success. > > So the patch is just a best-effort-try, if the binary is not > installed in /sbin, /usr/sbin or /usr/local/sbin, my patch just gives > up. > Maybe we can print error then? It would be useful for programmers to understand the problem. Sometimes, to debug early stages I simply redirected all error messages from mdadm to /dev/kmsg. > > > >> + > >> if (system("modprobe md_mod") == 0) > >> fd = open(new_array_file, O_WRONLY); > > > >> } > > > > The change will affect code executed later, probably we don't want > > that. Shouldn't we restore old PATH here to minimize risk? > > For my understanding if the code was called after boot up, these path > should be set already by shell initialization scripts. And for udev > context, it is called and exited, and NOT shared with other udev > tasks, almost no influence. I see, it is more theoretical problem, almost to possible to met. All fine then, we can continue with no clean up here as I cannot find normal scenario it can occur. Your change (after getenv fixes) LGTM. You can take a look into concept I proposed but I have no strong preference. Thanks, Mariusz
> 2025年1月22日 21:26，Mariusz Tkaczyk <mtkaczyk@kernel.org> 写道： > > On Wed, 22 Jan 2025 20:43:19 +0800 > Coly Li <colyli@suse.de> wrote: >> [snipped] >> So the patch is just a best-effort-try, if the binary is not >> installed in /sbin, /usr/sbin or /usr/local/sbin, my patch just gives >> up. >> > > Maybe we can print error then? It would be useful for programmers > to understand the problem. Sometimes, to debug early stages I simply > redirected all error messages from mdadm to /dev/kmsg. > Currently there is error message: sh: modprobe: command not found. This message was caught from console and this was how the root cause was found. This might be enough? > >>> >>>> + >>>> if (system("modprobe md_mod") == 0) >>>> fd = open(new_array_file, O_WRONLY); >>> >>>> } >>> >>> The change will affect code executed later, probably we don't want >>> that. Shouldn't we restore old PATH here to minimize risk? >> >> For my understanding if the code was called after boot up, these path >> should be set already by shell initialization scripts. And for udev >> context, it is called and exited, and NOT shared with other udev >> tasks, almost no influence. > > I see, it is more theoretical problem, almost to possible to met. > All fine then, we can continue with no clean up here as I cannot find > normal scenario it can occur. > > Your change (after getenv fixes) LGTM. You can take a look into concept > I proposed but I have no strong preference. Copied. V3 posted. Thanks. Coly Li
diff --git a/mdopen.c b/mdopen.c index 26f0c716..65bd8a1b 100644 --- a/mdopen.c +++ b/mdopen.c @@ -39,6 +39,17 @@ int create_named_array(char *devnm) fd = open(new_array_file, O_WRONLY); if (fd < 0 && errno == ENOENT) { + char buf[PATH_MAX] = {0}; + + /* + * When called by udev worker context, path of modprobe + * might not be in env PATH. Set sbin paths into PATH + * env to avoid potential failure when run modprobe here. + */ + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"), + "/sbin:/usr/sbin:/usr/local/sbin"); + setenv("PATH", buf, 1); + if (system("modprobe md_mod") == 0) fd = open(new_array_file, O_WRONLY); }
During the boot process if mdadm is called in udev context, sbin paths like /sbin, /usr/sbin, /usr/local/sbin normally not defined in PATH env variable, calling system("modprobe md_mod") in create_named_array() may fail with 'sh: modprobe: command not found' error message. We don't want to move modprobe binary into udev private directory, so setting the PATH env is a more proper method to avoid the above issue. This patch sets PATH env variable with "/sbin:/usr/sbin:/usr/local/sbin" before calling system("modprobe md_mod"). The change only takes effect within the udev worker context, not seen by global udev environment. Signed-off-by: Coly Li <colyli@suse.de> --- Changelog, v2: set buf[PATH_MAX] to 0 in stack variable announcement. v1: the original version. mdopen.c | 11 +++++++++++ 1 file changed, 11 insertions(+)