Linux-Modules Archive on lore.kernel.org
 help / Atom feed
* Re: [RFC PATCH] exec: Avoid recursive modprobe for binary format handlers
       [not found] ` <20170802001200.GD18884@wotan.suse.de>
@ 2017-08-09  0:09   ` Luis R. Rodriguez
  2017-09-08 21:23     ` Lucas De Marchi
  0 siblings, 1 reply; 2+ messages in thread
From: Luis R. Rodriguez @ 2017-08-09  0:09 UTC (permalink / raw)
  To: Luis R. Rodriguez, linux-modules, Mian Yousaf Kaukab
  Cc: Matt Redfearn, Alexander Viro, Andrew Morton, David Howells,
	Dmitry Torokhov, Dan Carpenter, Kees Cook, Jessica Yu,
	Michal Marek, Linus Torvalds, Greg Kroah-Hartman, linux-mips,
	Petr Mladek, linux-fsdevel, linux-kernel

On Wed, Aug 02, 2017 at 02:12:00AM +0200, Luis R. Rodriguez wrote:
> On Fri, Jul 21, 2017 at 03:05:20PM +0100, Matt Redfearn wrote:
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 62175cbcc801..004bb50a01fe 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1644,6 +1644,9 @@ int search_binary_handler(struct linux_binprm *bprm)
> >  		if (printable(bprm->buf[0]) && printable(bprm->buf[1]) &&
> >  		    printable(bprm->buf[2]) && printable(bprm->buf[3]))
> >  			return retval;
> > +		/* Game over if we need to load a module to execute modprobe */
> > +		if (strcmp(bprm->filename, modprobe_path) == 0)
> > +			return retval;
> 
> Wouldn't this just break having a binfmt used for modprobe always?

The place where you put the check is when a system has CONFIG_MODULES
and a first search for built-in handlers yielded no results so it would
not break that for built-in.

Thinking about this a little further, having an binfmd handler not built-in
seems to really be the issue in this particular case and indeed having one as
modular really just makes no sense as modprobe would be needed.

Although the alternative patch I suggested still makes sense for a *generic
loop detection complaint/error fix, putting this check in place and bailing
still makes sense as well, but this sort of thing seems to be the type of
system build error userspace could try to pick up on pro-actively, ie you
should not get to the point you boot into this, the build system should somehow
complain about it.

Cc'ing linux-modules folks to see if perhaps kmod could do something about this
more proactively.

Ideally if we could do this via kconfig for an architecture that'd be even
better but its not clear if this sort of thing is visible for MIPS on kconfig,
so kmod could be a next place to look for.

We'd need userpace kmod to verify the binary format for modprobe / kmod was
built-in otherwise fail.

> This also does not solve another issue I could think of now:
> 
> The *old* implementation would also prevent a set of binaries to daisy chain
> a set of 50 different binaries which require different binfmt loaders. The
> current implementation enables this and we'd just wait. There's a bound to
> the number of binfmd loaders though, so this would be bounded. If however
> a 2nd loader loaded the first binary we'd run into the same issue I think.

Upon testing -- the 2nd loader will not incur another new bump on kmod
concurrent given the original module would have a struct module already
present on the modules list, so these loops don't create a kmod concurrent
bump, they just keep the system waiting forever.

userspace kmod detects these sorts of loops but only for symbol references,
it doesn't check for request_module() calls, and even if it did, it would
then have to also consider aliasing.

kmod handles loops through export symbols references, it won't let a system
complete 'make modules_install' target as depmod will fail when this is
detected. The kmod git tree has some test for this, see
testsuite/module-playground/mod-loop* -- loading any of those yields an error
on modules_install target time as depmod picks it up:

depmod: ERROR: Found 7 modules in dependency cycles!
depmod: ERROR: Cycle detected: mod_loop_a -> mod_loop_b -> mod_loop_c -> mod_loop_a
depmod: ERROR: Cycle detected: mod_loop_a -> mod_loop_b -> mod_loop_c -> mod_loop_g
depmod: ERROR: Cycle detected: mod_loop_a -> mod_loop_b -> mod_loop_c -> mod_loop_f
depmod: ERROR: Cycle detected: mod_loop_d -> mod_loop_e -> mod_loop_d

So -- I will continue to submit the new generic alternative patch I suggested but
we should discuss this particular error further to try to more proactively
prevent it if possible.

It seems we already have in place userspace tools to prevent further loops, the
new warning should help catch others which escape our imagination at this time.
Two other types of issues would be desirable in the future for userspace
to detect proactively:

  o module loops using request_module() and aliases
  o when the modprobe binfmt is not built-in

  Luis

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [RFC PATCH] exec: Avoid recursive modprobe for binary format handlers
  2017-08-09  0:09   ` [RFC PATCH] exec: Avoid recursive modprobe for binary format handlers Luis R. Rodriguez
@ 2017-09-08 21:23     ` Lucas De Marchi
  0 siblings, 0 replies; 2+ messages in thread
From: Lucas De Marchi @ 2017-09-08 21:23 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: linux-modules, Mian Yousaf Kaukab, Matt Redfearn, Alexander Viro,
	Andrew Morton, David Howells, Dmitry Torokhov, Dan Carpenter,
	Kees Cook, Jessica Yu, Michal Marek, Linus Torvalds,
	Greg Kroah-Hartman, linux-mips, Petr Mladek, linux-fsdevel, lkml

Hi,

On Tue, Aug 8, 2017 at 5:09 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Wed, Aug 02, 2017 at 02:12:00AM +0200, Luis R. Rodriguez wrote:
>> On Fri, Jul 21, 2017 at 03:05:20PM +0100, Matt Redfearn wrote:
>> > diff --git a/fs/exec.c b/fs/exec.c
>> > index 62175cbcc801..004bb50a01fe 100644
>> > --- a/fs/exec.c
>> > +++ b/fs/exec.c
>> > @@ -1644,6 +1644,9 @@ int search_binary_handler(struct linux_binprm *bprm)
>> >             if (printable(bprm->buf[0]) && printable(bprm->buf[1]) &&
>> >                 printable(bprm->buf[2]) && printable(bprm->buf[3]))
>> >                     return retval;
>> > +           /* Game over if we need to load a module to execute modprobe */
>> > +           if (strcmp(bprm->filename, modprobe_path) == 0)
>> > +                   return retval;
>>
>> Wouldn't this just break having a binfmt used for modprobe always?
>
> The place where you put the check is when a system has CONFIG_MODULES
> and a first search for built-in handlers yielded no results so it would
> not break that for built-in.
>
> Thinking about this a little further, having an binfmd handler not built-in
> seems to really be the issue in this particular case and indeed having one as
> modular really just makes no sense as modprobe would be needed.
>
> Although the alternative patch I suggested still makes sense for a *generic
> loop detection complaint/error fix, putting this check in place and bailing
> still makes sense as well, but this sort of thing seems to be the type of
> system build error userspace could try to pick up on pro-actively, ie you
> should not get to the point you boot into this, the build system should somehow
> complain about it.
>
> Cc'ing linux-modules folks to see if perhaps kmod could do something about this
> more proactively.

Tracking at runtime with modprobe/libkmod would be really difficult as
a module can be loaded
from different sources. I don't see a reliable way to do that. One
thing often forgotten
is that due to install rules the user can even add anything as a
dependency with kmod not
even knowing about (softdep is related, but at least kmod knows what
the user is trying to do
and use it to handle dependencies).

For this particular case, not going through the modprobe helper would
be a way to accomplish that since
you wouldn't need the corresponding binfmt module to run modprobe.
Udev handles module
loading via libkmod , but the only way to trigger it is via the rules
rather than via a request from kernel.


Lucas De Marchi

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1500645920-28490-1-git-send-email-matt.redfearn@imgtec.com>
     [not found] ` <20170802001200.GD18884@wotan.suse.de>
2017-08-09  0:09   ` [RFC PATCH] exec: Avoid recursive modprobe for binary format handlers Luis R. Rodriguez
2017-09-08 21:23     ` Lucas De Marchi

Linux-Modules Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-modules/0 linux-modules/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-modules linux-modules/ https://lore.kernel.org/linux-modules \
		linux-modules@vger.kernel.org linux-modules@archiver.kernel.org
	public-inbox-index linux-modules


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-modules


AGPL code for this site: git clone https://public-inbox.org/ public-inbox