Re: kmod: provide wrappers for kmod_concurrent inc/dec

From: Jessica Yu
Date: Wed Dec 21 2016 - 23:48:39 EST


+++ Luis R. Rodriguez [16/12/16 09:05 +0100]:
On Thu, Dec 15, 2016 at 01:46:25PM +0100, Petr Mladek wrote:
On Thu 2016-12-08 22:08:59, Luis R. Rodriguez wrote:
> On Thu, Dec 08, 2016 at 12:29:42PM -0800, Kees Cook wrote:
> > On Thu, Dec 8, 2016 at 11:48 AM, Luis R. Rodriguez <mcgrof@xxxxxxxxxx> wrote:
> > > kmod_concurrent is used as an atomic counter for enabling
> > > the allowed limit of modprobe calls, provide wrappers for it
> > > to enable this to be expanded on more easily. This will be done
> > > later.
> > >
> > > Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxxxx>
> > > ---
> > > kernel/kmod.c | 27 +++++++++++++++++++++------
> > > 1 file changed, 21 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/kernel/kmod.c b/kernel/kmod.c
> > > index cb6f7ca7b8a5..049d7eabda38 100644
> > > --- a/kernel/kmod.c
> > > +++ b/kernel/kmod.c
> > > @@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
> > > return -ENOMEM;
> > > }
> > >
> > > +static int kmod_umh_threads_get(void)
> > > +{
> > > + atomic_inc(&kmod_concurrent);

This approach might actually cause false failures. If we
are on the limit and more processes do this increment
in parallel, it makes the number bigger that it should be.

This approach is *exactly* what the existing code does :P
I just provided wrappers. I agree with the old approach though,
reason is it acts as a lock in for the bump.

I think what Petr meant was that we could run into false failures when multiple
atomic increments happen between the first increment and the subsequent
atomic_read.

Say max_modprobes is 64 -

atomic_inc(&kmod_concurrent); // thread 1: kmod_concurrent is 63
atomic_inc(&kmod_concurrent); // thread 2: kmod_concurrent is 64
atomic_inc(&kmod_concurrent); // thread 3: kmod_concurrent is 65
if (atomic_read(&kmod_concurrent) < max_modprobes) // if all threads read 65 here, then all will error out
return 0; // when the first two should have succeeded (false failures)
atomic_dec(&kmod_concurrent);
return -ENOMEM;

But yeah, I think this issue was already in the existing kmod code..

Jessica