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

[PATCH] 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>
---
 mdopen.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mdopen.c b/mdopen.c
index 26f0c716..30cf781b 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -39,6 +39,18 @@ int create_named_array(char *devnm)
 
 	fd = open(new_array_file, O_WRONLY);
 	if (fd < 0 && errno == ENOENT) {
+		char buf[PATH_MAX];
+
+		/*
+		 * 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.
+		 */
+		memset(buf, 0, PATH_MAX);
+		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.47.1

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

[Mariusz Tkaczyk]

On Tue, 21 Jan 2025 23:16:03 +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
> 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.

Hi Coly,
Nice explanation, thanks!

> 
> Signed-off-by: Coly Li <colyli@suse.de>
> ---
>  mdopen.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/mdopen.c b/mdopen.c
> index 26f0c716..30cf781b 100644
> --- a/mdopen.c
> +++ b/mdopen.c
> @@ -39,6 +39,18 @@ int create_named_array(char *devnm)
>  
>  	fd = open(new_array_file, O_WRONLY);
>  	if (fd < 0 && errno == ENOENT) {
> +		char buf[PATH_MAX];
> +
> +		/*
> +		 * 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.
> +		 */
> +		memset(buf, 0, PATH_MAX);

just:
char buf[PATH_MAX] = {0};

> +		snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"),
> +			 "/sbin:/usr/sbin:/usr/local/sbin");
> +		setenv("PATH", buf, 1);

Isn't it over-complicated? Why not simply:
system("/sbin/modprobe md_mod");

If modprobe is not always in /sbin (checked on my opensuse only)
we can make in configured during compilation, simple call `which
modprobe` should do the job.
What do you think?

Thanks,
Mariusz

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

[Coly Li]

> 2025年1月22日 02:09，Mariusz Tkaczyk <mtkaczyk@kernel.org> 写道：
> 
> On Tue, 21 Jan 2025 23:16:03 +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
>> 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.
> 
> Hi Coly,
> Nice explanation, thanks!
> 
>> 
>> Signed-off-by: Coly Li <colyli@suse.de>
>> ---
>> mdopen.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>> 
>> diff --git a/mdopen.c b/mdopen.c
>> index 26f0c716..30cf781b 100644
>> --- a/mdopen.c
>> +++ b/mdopen.c
>> @@ -39,6 +39,18 @@ int create_named_array(char *devnm)
>> 
>> fd = open(new_array_file, O_WRONLY);
>> if (fd < 0 && errno == ENOENT) {
>> + char buf[PATH_MAX];
>> +
>> + /*
>> +  * 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.
>> +  */
>> + memset(buf, 0, PATH_MAX);
> 
> just:
> char buf[PATH_MAX] = {0};

OK, let me change this.

> 
>> + snprintf(buf, PATH_MAX - 1, "%s:%s", getenv("PATH"),
>> +  "/sbin:/usr/sbin:/usr/local/sbin");
>> + setenv("PATH", buf, 1);
> 
> Isn't it over-complicated? Why not simply:
> system("/sbin/modprobe md_mod");
> 
> If modprobe is not always in /sbin (checked on my opensuse only)

Yes, it might be in /usr/local/sbin, /usr/sbin/ or somewhere else which I am not
able to take it for granted on other known or unknown distributions.


> we can make in configured during compilation, simple call `which
> modprobe` should do the job.
> What do you think?

Sure, it should work. But might be more complicated, I mean lines of code in total.

Thanks for the review.

Coly Li