[PATCH v2] mdopen: add sbin path to env PATH when call system("modprobe md_mod")

[PATCH v2] mdopen: add sbin path to env PATH when call system("modprobe md_mod")

[Coly Li]

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(+)

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);
 	}
-- 
2.48.1

Re: [PATCH v2] mdopen: add sbin path to env PATH when call system("modprobe md_mod")

[Mariusz Tkaczyk]

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

Re: [PATCH v2] mdopen: add sbin path to env PATH when call system("modprobe md_mod")

[Coly Li]

> 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

Re: [PATCH v2] mdopen: add sbin path to env PATH when call system("modprobe md_mod")

[Mariusz Tkaczyk]

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

Re: [PATCH v2] mdopen: add sbin path to env PATH when call system("modprobe md_mod")

[Coly Li]

> 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