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

[Mariusz Tkaczyk <mtkaczyk@kernel.org>]

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