Messages in this thread Patch in this message | | | Date | Wed, 21 Dec 2016 20:48:06 -0800 | From | Jessica Yu <> | Subject | Re: kmod: provide wrappers for kmod_concurrent inc/dec |
| |
+++ 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@kernel.org> 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@kernel.org> >> > > > --- >> > > > 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
| |