Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1763410AbcLVEsR (ORCPT ); Wed, 21 Dec 2016 23:48:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53558 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750916AbcLVEsQ (ORCPT ); Wed, 21 Dec 2016 23:48:16 -0500 Date: Wed, 21 Dec 2016 20:48:06 -0800 From: Jessica Yu To: "Luis R. Rodriguez" Cc: Petr Mladek , Kees Cook , shuah@kernel.org, Rusty Russell , "Eric W. Biederman" , Dmitry Torokhov , Arnaldo Carvalho de Melo , Jonathan Corbet , martin.wilck@suse.com, Michal Marek , hare@suse.com, rwright@hpe.com, Jeff Mahoney , DSterba@suse.com, fdmanana@suse.com, neilb@suse.com, Guenter Roeck , rgoldwyn@suse.com, subashab@codeaurora.org, Heinrich Schuchardt , Aaron Tomlin , mbenes@suse.cz, "Paul E. McKenney" , Dan Williams , Josh Poimboeuf , "David S. Miller" , Ingo Molnar , Andrew Morton , Linus Torvalds , linux-kselftest@vger.kernel.org, "linux-doc@vger.kernel.org" , LKML Subject: Re: kmod: provide wrappers for kmod_concurrent inc/dec Message-ID: <20161222044806.bajz6tdg5gtvud2t@jeyu> References: <20161208184801.1689-1-mcgrof@kernel.org> <20161208194824.2532-1-mcgrof@kernel.org> <20161208210859.GZ1402@wotan.suse.de> <20161215124625.GA14324@pathway.suse.cz> <20161216080500.GE13946@wotan.suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20161216080500.GE13946@wotan.suse.de> X-OS: Linux jeyu 4.9.0 x86_64 User-Agent: NeoMutt/20161126 (1.7.1) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 22 Dec 2016 04:48:15 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2304 Lines: 52 +++ 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 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 >> > > > --- >> > > > 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