Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Tue, May 16, 2017 at 08:02:17PM +0200, Luis R. Rodriguez wrote:
> On Wed, Jan 11, 2017 at 09:08:57PM +0100, Luis R. Rodriguez wrote:
> > On Tue, Jan 10, 2017 at 07:57:10PM +0100, Luis R. Rodriguez wrote:
> > > On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> > > > 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:
> > > > > > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > > > > > +   return 0;
> > > > > > > > +   atomic_dec(_concurrent);
> > > > > > > > +   return -ENOMEM;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void kmod_umh_threads_put(void)
> > > > > > > > +{
> > > > > > > > +   atomic_dec(_concurrent);
> > > > > > > > +}
> > > > > > > 
> > > > > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > > > > atomic_t for reference counting...
> > > > > > 
> > > > > > That's a much broader functional change than I was looking for, but 
> > > > > > I am up for
> > > > > > it. Can you describe the benefit of using kref you expect or why 
> > > > > > this is an
> > > > > > ongoing crusade? Since its a larger functional change how about 
> > > > > > doing this
> > > > > > change later, and we can test impact with the tress test driver. In 
> > > > > > theory if
> > > > > > there are benefits can't we add a test case to prove the gains?
> > > > > 
> > > > > Kees probably refers to the kref improvements that Peter Zijlstra
> > > > > is working on, see
> > > > > https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> > > > > 
> > > > > The advantage is that the new refcount API handles over and
> > > > > underflow.
> > > > > 
> > > > > Another advantage is that it increments/decrements the value
> > > > > only when it is safe. It uses cmpxchg to make sure that
> > > > > the checks are valid.
> > > > 
> > > > Great thanks, will look into that.
> > > 
> > > OK I've done the conversion now, the only thing is linux-next as of today 
> > > lacks
> > > KREF_INIT() so I've open coded it for now. Once Peter's changes get 
> > > merged the
> > > only thing we'dneed is to change the open code line to KREF_INIT().
> > > 
> > > I'll annotate this as Suggested-by Kees and Petr, I did this as a 
> > > separate atomic
> > > step after this to make it easier for review.
> > 
> > Spoke too soon, kref_read() is not upstream yet either, so I can hold 
> > conversion
> > over until Peter's work is merged. Peter please Cc me on those patches if 
> > possible
> > :D
> 
> All the needed kref stuff is upstream now, however, kref is overkill for
> kmod_concurrent given this is just a counter, it is not used to release
> any object, and kref_put() requires such mechanism. The lightweight
> refcount_t is much more appropriate here so will use that and respin
> this series, finally.

And... even the refcount_t is overkill here given even with preemption stuff on
inc we still run into the warnings implemented by the recount stuff right away.
The only way to properly fix this is with a proper lock and I don't think this 
is
worth it at this point.

This would be an issue if the accounting here was for an object but since its
not and its just a loose estimate for a subjective "reasonable threshold" this
is all just overkill.

Lesson: (unless I hear otherwise)

As such I see no real strong motivation for a change here now. Counters, used
without any object references or any real critical stuff is left best with the
old atomic counters.

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Tue, May 16, 2017 at 08:02:17PM +0200, Luis R. Rodriguez wrote:
> On Wed, Jan 11, 2017 at 09:08:57PM +0100, Luis R. Rodriguez wrote:
> > On Tue, Jan 10, 2017 at 07:57:10PM +0100, Luis R. Rodriguez wrote:
> > > On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> > > > 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:
> > > > > > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > > > > > +   return 0;
> > > > > > > > +   atomic_dec(_concurrent);
> > > > > > > > +   return -ENOMEM;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void kmod_umh_threads_put(void)
> > > > > > > > +{
> > > > > > > > +   atomic_dec(_concurrent);
> > > > > > > > +}
> > > > > > > 
> > > > > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > > > > atomic_t for reference counting...
> > > > > > 
> > > > > > That's a much broader functional change than I was looking for, but 
> > > > > > I am up for
> > > > > > it. Can you describe the benefit of using kref you expect or why 
> > > > > > this is an
> > > > > > ongoing crusade? Since its a larger functional change how about 
> > > > > > doing this
> > > > > > change later, and we can test impact with the tress test driver. In 
> > > > > > theory if
> > > > > > there are benefits can't we add a test case to prove the gains?
> > > > > 
> > > > > Kees probably refers to the kref improvements that Peter Zijlstra
> > > > > is working on, see
> > > > > https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> > > > > 
> > > > > The advantage is that the new refcount API handles over and
> > > > > underflow.
> > > > > 
> > > > > Another advantage is that it increments/decrements the value
> > > > > only when it is safe. It uses cmpxchg to make sure that
> > > > > the checks are valid.
> > > > 
> > > > Great thanks, will look into that.
> > > 
> > > OK I've done the conversion now, the only thing is linux-next as of today 
> > > lacks
> > > KREF_INIT() so I've open coded it for now. Once Peter's changes get 
> > > merged the
> > > only thing we'dneed is to change the open code line to KREF_INIT().
> > > 
> > > I'll annotate this as Suggested-by Kees and Petr, I did this as a 
> > > separate atomic
> > > step after this to make it easier for review.
> > 
> > Spoke too soon, kref_read() is not upstream yet either, so I can hold 
> > conversion
> > over until Peter's work is merged. Peter please Cc me on those patches if 
> > possible
> > :D
> 
> All the needed kref stuff is upstream now, however, kref is overkill for
> kmod_concurrent given this is just a counter, it is not used to release
> any object, and kref_put() requires such mechanism. The lightweight
> refcount_t is much more appropriate here so will use that and respin
> this series, finally.

And... even the refcount_t is overkill here given even with preemption stuff on
inc we still run into the warnings implemented by the recount stuff right away.
The only way to properly fix this is with a proper lock and I don't think this 
is
worth it at this point.

This would be an issue if the accounting here was for an object but since its
not and its just a loose estimate for a subjective "reasonable threshold" this
is all just overkill.

Lesson: (unless I hear otherwise)

As such I see no real strong motivation for a change here now. Counters, used
without any object references or any real critical stuff is left best with the
old atomic counters.

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Wed, Jan 11, 2017 at 09:08:57PM +0100, Luis R. Rodriguez wrote:
> On Tue, Jan 10, 2017 at 07:57:10PM +0100, Luis R. Rodriguez wrote:
> > On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> > > 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:
> > > > > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > > > > +   return 0;
> > > > > > > +   atomic_dec(_concurrent);
> > > > > > > +   return -ENOMEM;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void kmod_umh_threads_put(void)
> > > > > > > +{
> > > > > > > +   atomic_dec(_concurrent);
> > > > > > > +}
> > > > > > 
> > > > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > > > atomic_t for reference counting...
> > > > > 
> > > > > That's a much broader functional change than I was looking for, but I 
> > > > > am up for
> > > > > it. Can you describe the benefit of using kref you expect or why this 
> > > > > is an
> > > > > ongoing crusade? Since its a larger functional change how about doing 
> > > > > this
> > > > > change later, and we can test impact with the tress test driver. In 
> > > > > theory if
> > > > > there are benefits can't we add a test case to prove the gains?
> > > > 
> > > > Kees probably refers to the kref improvements that Peter Zijlstra
> > > > is working on, see
> > > > https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> > > > 
> > > > The advantage is that the new refcount API handles over and
> > > > underflow.
> > > > 
> > > > Another advantage is that it increments/decrements the value
> > > > only when it is safe. It uses cmpxchg to make sure that
> > > > the checks are valid.
> > > 
> > > Great thanks, will look into that.
> > 
> > OK I've done the conversion now, the only thing is linux-next as of today 
> > lacks
> > KREF_INIT() so I've open coded it for now. Once Peter's changes get merged 
> > the
> > only thing we'dneed is to change the open code line to KREF_INIT().
> > 
> > I'll annotate this as Suggested-by Kees and Petr, I did this as a separate 
> > atomic
> > step after this to make it easier for review.
> 
> Spoke too soon, kref_read() is not upstream yet either, so I can hold 
> conversion
> over until Peter's work is merged. Peter please Cc me on those patches if 
> possible
> :D

All the needed kref stuff is upstream now, however, kref is overkill for
kmod_concurrent given this is just a counter, it is not used to release
any object, and kref_put() requires such mechanism. The lightweight
refcount_t is much more appropriate here so will use that and respin
this series, finally.

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Wed, Jan 11, 2017 at 09:08:57PM +0100, Luis R. Rodriguez wrote:
> On Tue, Jan 10, 2017 at 07:57:10PM +0100, Luis R. Rodriguez wrote:
> > On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> > > 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:
> > > > > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > > > > +   return 0;
> > > > > > > +   atomic_dec(_concurrent);
> > > > > > > +   return -ENOMEM;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void kmod_umh_threads_put(void)
> > > > > > > +{
> > > > > > > +   atomic_dec(_concurrent);
> > > > > > > +}
> > > > > > 
> > > > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > > > atomic_t for reference counting...
> > > > > 
> > > > > That's a much broader functional change than I was looking for, but I 
> > > > > am up for
> > > > > it. Can you describe the benefit of using kref you expect or why this 
> > > > > is an
> > > > > ongoing crusade? Since its a larger functional change how about doing 
> > > > > this
> > > > > change later, and we can test impact with the tress test driver. In 
> > > > > theory if
> > > > > there are benefits can't we add a test case to prove the gains?
> > > > 
> > > > Kees probably refers to the kref improvements that Peter Zijlstra
> > > > is working on, see
> > > > https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> > > > 
> > > > The advantage is that the new refcount API handles over and
> > > > underflow.
> > > > 
> > > > Another advantage is that it increments/decrements the value
> > > > only when it is safe. It uses cmpxchg to make sure that
> > > > the checks are valid.
> > > 
> > > Great thanks, will look into that.
> > 
> > OK I've done the conversion now, the only thing is linux-next as of today 
> > lacks
> > KREF_INIT() so I've open coded it for now. Once Peter's changes get merged 
> > the
> > only thing we'dneed is to change the open code line to KREF_INIT().
> > 
> > I'll annotate this as Suggested-by Kees and Petr, I did this as a separate 
> > atomic
> > step after this to make it easier for review.
> 
> Spoke too soon, kref_read() is not upstream yet either, so I can hold 
> conversion
> over until Peter's work is merged. Peter please Cc me on those patches if 
> possible
> :D

All the needed kref stuff is upstream now, however, kref is overkill for
kmod_concurrent given this is just a counter, it is not used to release
any object, and kref_put() requires such mechanism. The lightweight
refcount_t is much more appropriate here so will use that and respin
this series, finally.

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Tue, Jan 10, 2017 at 07:57:10PM +0100, Luis R. Rodriguez wrote:
> On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> > 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:
> > > > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > > > +   return 0;
> > > > > > +   atomic_dec(_concurrent);
> > > > > > +   return -ENOMEM;
> > > > > > +}
> > > > > > +
> > > > > > +static void kmod_umh_threads_put(void)
> > > > > > +{
> > > > > > +   atomic_dec(_concurrent);
> > > > > > +}
> > > > > 
> > > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > > atomic_t for reference counting...
> > > > 
> > > > That's a much broader functional change than I was looking for, but I 
> > > > am up for
> > > > it. Can you describe the benefit of using kref you expect or why this 
> > > > is an
> > > > ongoing crusade? Since its a larger functional change how about doing 
> > > > this
> > > > change later, and we can test impact with the tress test driver. In 
> > > > theory if
> > > > there are benefits can't we add a test case to prove the gains?
> > > 
> > > Kees probably refers to the kref improvements that Peter Zijlstra
> > > is working on, see
> > > https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> > > 
> > > The advantage is that the new refcount API handles over and
> > > underflow.
> > > 
> > > Another advantage is that it increments/decrements the value
> > > only when it is safe. It uses cmpxchg to make sure that
> > > the checks are valid.
> > 
> > Great thanks, will look into that.
> 
> OK I've done the conversion now, the only thing is linux-next as of today 
> lacks
> KREF_INIT() so I've open coded it for now. Once Peter's changes get merged the
> only thing we'dneed is to change the open code line to KREF_INIT().
> 
> I'll annotate this as Suggested-by Kees and Petr, I did this as a separate 
> atomic
> step after this to make it easier for review.

Spoke too soon, kref_read() is not upstream yet either, so I can hold conversion
over until Peter's work is merged. Peter please Cc me on those patches if 
possible
:D

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Tue, Jan 10, 2017 at 07:57:10PM +0100, Luis R. Rodriguez wrote:
> On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> > 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:
> > > > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > > > +   return 0;
> > > > > > +   atomic_dec(_concurrent);
> > > > > > +   return -ENOMEM;
> > > > > > +}
> > > > > > +
> > > > > > +static void kmod_umh_threads_put(void)
> > > > > > +{
> > > > > > +   atomic_dec(_concurrent);
> > > > > > +}
> > > > > 
> > > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > > atomic_t for reference counting...
> > > > 
> > > > That's a much broader functional change than I was looking for, but I 
> > > > am up for
> > > > it. Can you describe the benefit of using kref you expect or why this 
> > > > is an
> > > > ongoing crusade? Since its a larger functional change how about doing 
> > > > this
> > > > change later, and we can test impact with the tress test driver. In 
> > > > theory if
> > > > there are benefits can't we add a test case to prove the gains?
> > > 
> > > Kees probably refers to the kref improvements that Peter Zijlstra
> > > is working on, see
> > > https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> > > 
> > > The advantage is that the new refcount API handles over and
> > > underflow.
> > > 
> > > Another advantage is that it increments/decrements the value
> > > only when it is safe. It uses cmpxchg to make sure that
> > > the checks are valid.
> > 
> > Great thanks, will look into that.
> 
> OK I've done the conversion now, the only thing is linux-next as of today 
> lacks
> KREF_INIT() so I've open coded it for now. Once Peter's changes get merged the
> only thing we'dneed is to change the open code line to KREF_INIT().
> 
> I'll annotate this as Suggested-by Kees and Petr, I did this as a separate 
> atomic
> step after this to make it easier for review.

Spoke too soon, kref_read() is not upstream yet either, so I can hold conversion
over until Peter's work is merged. Peter please Cc me on those patches if 
possible
:D

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> 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:
> > > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > > +   return 0;
> > > > > +   atomic_dec(_concurrent);
> > > > > +   return -ENOMEM;
> > > > > +}
> > > > > +
> > > > > +static void kmod_umh_threads_put(void)
> > > > > +{
> > > > > +   atomic_dec(_concurrent);
> > > > > +}
> > > > 
> > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > atomic_t for reference counting...
> > > 
> > > That's a much broader functional change than I was looking for, but I am 
> > > up for
> > > it. Can you describe the benefit of using kref you expect or why this is 
> > > an
> > > ongoing crusade? Since its a larger functional change how about doing this
> > > change later, and we can test impact with the tress test driver. In 
> > > theory if
> > > there are benefits can't we add a test case to prove the gains?
> > 
> > Kees probably refers to the kref improvements that Peter Zijlstra
> > is working on, see
> > https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> > 
> > The advantage is that the new refcount API handles over and
> > underflow.
> > 
> > Another advantage is that it increments/decrements the value
> > only when it is safe. It uses cmpxchg to make sure that
> > the checks are valid.
> 
> Great thanks, will look into that.

OK I've done the conversion now, the only thing is linux-next as of today lacks
KREF_INIT() so I've open coded it for now. Once Peter's changes get merged the
only thing we'dneed is to change the open code line to KREF_INIT().

I'll annotate this as Suggested-by Kees and Petr, I did this as a separate 
atomic
step after this to make it easier for review.

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

On Fri, Dec 16, 2016 at 09:05:00AM +0100, Luis R. Rodriguez wrote:
> 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:
> > > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > > +   return 0;
> > > > > +   atomic_dec(_concurrent);
> > > > > +   return -ENOMEM;
> > > > > +}
> > > > > +
> > > > > +static void kmod_umh_threads_put(void)
> > > > > +{
> > > > > +   atomic_dec(_concurrent);
> > > > > +}
> > > > 
> > > > Can you use a kref here instead? We're trying to kill raw use of
> > > > atomic_t for reference counting...
> > > 
> > > That's a much broader functional change than I was looking for, but I am 
> > > up for
> > > it. Can you describe the benefit of using kref you expect or why this is 
> > > an
> > > ongoing crusade? Since its a larger functional change how about doing this
> > > change later, and we can test impact with the tress test driver. In 
> > > theory if
> > > there are benefits can't we add a test case to prove the gains?
> > 
> > Kees probably refers to the kref improvements that Peter Zijlstra
> > is working on, see
> > https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> > 
> > The advantage is that the new refcount API handles over and
> > underflow.
> > 
> > Another advantage is that it increments/decrements the value
> > only when it is safe. It uses cmpxchg to make sure that
> > the checks are valid.
> 
> Great thanks, will look into that.

OK I've done the conversion now, the only thing is linux-next as of today lacks
KREF_INIT() so I've open coded it for now. Once Peter's changes get merged the
only thing we'dneed is to change the open code line to KREF_INIT().

I'll annotate this as Suggested-by Kees and Petr, I did this as a separate 
atomic
step after this to make it easier for review.

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

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(_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. What seems rather
stupid though is to just reject with an error on limit without first
taking a breather. I've now added a little clutch so that we first
take some fresh air when close to the limit, this reduces the chances
of going fatal.

With a clutch in place we can still go over the limit, its just we'd
have a few threads waiting until previous calls clear out. If there
is enough calls waiting eventually we'll fail.

Note though that __request_module() can wait, but here is an option
to not wait so such a clutch can only wait if we're allowed to.

> > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > +   return 0;
> > > > +   atomic_dec(_concurrent);
> > > > +   return -ENOMEM;
> > > > +}
> > > > +
> > > > +static void kmod_umh_threads_put(void)
> > > > +{
> > > > +   atomic_dec(_concurrent);
> > > > +}
> > > 
> > > Can you use a kref here instead? We're trying to kill raw use of
> > > atomic_t for reference counting...
> > 
> > That's a much broader functional change than I was looking for, but I am up 
> > for
> > it. Can you describe the benefit of using kref you expect or why this is an
> > ongoing crusade? Since its a larger functional change how about doing this
> > change later, and we can test impact with the tress test driver. In theory 
> > if
> > there are benefits can't we add a test case to prove the gains?
> 
> Kees probably refers to the kref improvements that Peter Zijlstra
> is working on, see
> https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> 
> The advantage is that the new refcount API handles over and
> underflow.
> 
> Another advantage is that it increments/decrements the value
> only when it is safe. It uses cmpxchg to make sure that
> the checks are valid.

Great thanks, will look into that.

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

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(_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. What seems rather
stupid though is to just reject with an error on limit without first
taking a breather. I've now added a little clutch so that we first
take some fresh air when close to the limit, this reduces the chances
of going fatal.

With a clutch in place we can still go over the limit, its just we'd
have a few threads waiting until previous calls clear out. If there
is enough calls waiting eventually we'll fail.

Note though that __request_module() can wait, but here is an option
to not wait so such a clutch can only wait if we're allowed to.

> > > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > > +   return 0;
> > > > +   atomic_dec(_concurrent);
> > > > +   return -ENOMEM;
> > > > +}
> > > > +
> > > > +static void kmod_umh_threads_put(void)
> > > > +{
> > > > +   atomic_dec(_concurrent);
> > > > +}
> > > 
> > > Can you use a kref here instead? We're trying to kill raw use of
> > > atomic_t for reference counting...
> > 
> > That's a much broader functional change than I was looking for, but I am up 
> > for
> > it. Can you describe the benefit of using kref you expect or why this is an
> > ongoing crusade? Since its a larger functional change how about doing this
> > change later, and we can test impact with the tress test driver. In theory 
> > if
> > there are benefits can't we add a test case to prove the gains?
> 
> Kees probably refers to the kref improvements that Peter Zijlstra
> is working on, see
> https://lkml.kernel.org/r/20161114174446.832175...@infradead.org
> 
> The advantage is that the new refcount API handles over and
> underflow.
> 
> Another advantage is that it increments/decrements the value
> only when it is safe. It uses cmpxchg to make sure that
> the checks are valid.

Great thanks, will look into that.

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Petr Mladek]

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(_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.

> > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > +   return 0;
> > > +   atomic_dec(_concurrent);
> > > +   return -ENOMEM;
> > > +}
> > > +
> > > +static void kmod_umh_threads_put(void)
> > > +{
> > > +   atomic_dec(_concurrent);
> > > +}
> > 
> > Can you use a kref here instead? We're trying to kill raw use of
> > atomic_t for reference counting...
> 
> That's a much broader functional change than I was looking for, but I am up 
> for
> it. Can you describe the benefit of using kref you expect or why this is an
> ongoing crusade? Since its a larger functional change how about doing this
> change later, and we can test impact with the tress test driver. In theory if
> there are benefits can't we add a test case to prove the gains?

Kees probably refers to the kref improvements that Peter Zijlstra
is working on, see
https://lkml.kernel.org/r/20161114174446.832175...@infradead.org

The advantage is that the new refcount API handles over and
underflow.

Another advantage is that it increments/decrements the value
only when it is safe. It uses cmpxchg to make sure that
the checks are valid.

Best Regards,
Petr

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Petr Mladek]

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(_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.

> > > +   if (atomic_read(_concurrent) < max_modprobes)
> > > +   return 0;
> > > +   atomic_dec(_concurrent);
> > > +   return -ENOMEM;
> > > +}
> > > +
> > > +static void kmod_umh_threads_put(void)
> > > +{
> > > +   atomic_dec(_concurrent);
> > > +}
> > 
> > Can you use a kref here instead? We're trying to kill raw use of
> > atomic_t for reference counting...
> 
> That's a much broader functional change than I was looking for, but I am up 
> for
> it. Can you describe the benefit of using kref you expect or why this is an
> ongoing crusade? Since its a larger functional change how about doing this
> change later, and we can test impact with the tress test driver. In theory if
> there are benefits can't we add a test case to prove the gains?

Kees probably refers to the kref improvements that Peter Zijlstra
is working on, see
https://lkml.kernel.org/r/20161114174446.832175...@infradead.org

The advantage is that the new refcount API handles over and
underflow.

Another advantage is that it increments/decrements the value
only when it is safe. It uses cmpxchg to make sure that
the checks are valid.

Best Regards,
Petr

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

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
> > @@ -44,6 +44,9 @@
> >  #include 
> >
> >  extern int max_threads;
> > +
> > +static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> > +
> >  unsigned int max_modprobes;
> >  module_param(max_modprobes, uint, 0644);
> >  MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent 
> > modprobes");
> > @@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
> > return -ENOMEM;
> >  }
> >
> > +static int kmod_umh_threads_get(void)
> > +{
> > +   atomic_inc(_concurrent);
> > +   if (atomic_read(_concurrent) < max_modprobes)
> > +   return 0;
> > +   atomic_dec(_concurrent);
> > +   return -ENOMEM;
> > +}
> > +
> > +static void kmod_umh_threads_put(void)
> > +{
> > +   atomic_dec(_concurrent);
> > +}
> 
> Can you use a kref here instead? We're trying to kill raw use of
> atomic_t for reference counting...

That's a much broader functional change than I was looking for, but I am up for
it. Can you describe the benefit of using kref you expect or why this is an
ongoing crusade? Since its a larger functional change how about doing this
change later, and we can test impact with the tress test driver. In theory if
there are benefits can't we add a test case to prove the gains?

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Luis R. Rodriguez]

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
> > @@ -44,6 +44,9 @@
> >  #include 
> >
> >  extern int max_threads;
> > +
> > +static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> > +
> >  unsigned int max_modprobes;
> >  module_param(max_modprobes, uint, 0644);
> >  MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent 
> > modprobes");
> > @@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
> > return -ENOMEM;
> >  }
> >
> > +static int kmod_umh_threads_get(void)
> > +{
> > +   atomic_inc(_concurrent);
> > +   if (atomic_read(_concurrent) < max_modprobes)
> > +   return 0;
> > +   atomic_dec(_concurrent);
> > +   return -ENOMEM;
> > +}
> > +
> > +static void kmod_umh_threads_put(void)
> > +{
> > +   atomic_dec(_concurrent);
> > +}
> 
> Can you use a kref here instead? We're trying to kill raw use of
> atomic_t for reference counting...

That's a much broader functional change than I was looking for, but I am up for
it. Can you describe the benefit of using kref you expect or why this is an
ongoing crusade? Since its a larger functional change how about doing this
change later, and we can test impact with the tress test driver. In theory if
there are benefits can't we add a test case to prove the gains?

  Luis

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Kees Cook]

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
> @@ -44,6 +44,9 @@
>  #include 
>
>  extern int max_threads;
> +
> +static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> +
>  unsigned int max_modprobes;
>  module_param(max_modprobes, uint, 0644);
>  MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent 
> modprobes");
> @@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
> return -ENOMEM;
>  }
>
> +static int kmod_umh_threads_get(void)
> +{
> +   atomic_inc(_concurrent);
> +   if (atomic_read(_concurrent) < max_modprobes)
> +   return 0;
> +   atomic_dec(_concurrent);
> +   return -ENOMEM;
> +}
> +
> +static void kmod_umh_threads_put(void)
> +{
> +   atomic_dec(_concurrent);
> +}

Can you use a kref here instead? We're trying to kill raw use of
atomic_t for reference counting...

> +
>  /**
>   * __request_module - try to load a kernel module
>   * @wait: wait (or not) for the operation to complete
> @@ -129,7 +146,6 @@ int __request_module(bool wait, const char *fmt, ...)
> va_list args;
> char module_name[MODULE_NAME_LEN];
> int ret;
> -   static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> static int kmod_loop_msg;
>
> /*
> @@ -153,8 +169,8 @@ int __request_module(bool wait, const char *fmt, ...)
> if (ret)
> return ret;
>
> -   atomic_inc(_concurrent);
> -   if (atomic_read(_concurrent) > max_modprobes) {
> +   ret = kmod_umh_threads_get();
> +   if (ret) {
> /* We may be blaming an innocent here, but unlikely */
> if (kmod_loop_msg < 5) {
> printk(KERN_ERR
> @@ -162,15 +178,14 @@ int __request_module(bool wait, const char *fmt, ...)
>module_name);
> kmod_loop_msg++;
> }
> -   atomic_dec(_concurrent);
> -   return -ENOMEM;
> +   return ret;
> }
>
> trace_module_request(module_name, wait, _RET_IP_);
>
> ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : 
> UMH_WAIT_EXEC);
>
> -   atomic_dec(_concurrent);
> +   kmod_umh_threads_put();
> return ret;
>  }
>  EXPORT_SYMBOL(__request_module);
> --
> 2.10.1
>



-- 
Kees Cook
Nexus Security

Re: [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec

[Kees Cook]

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
> @@ -44,6 +44,9 @@
>  #include 
>
>  extern int max_threads;
> +
> +static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> +
>  unsigned int max_modprobes;
>  module_param(max_modprobes, uint, 0644);
>  MODULE_PARM_DESC(max_modprobes, "Max number of allowed concurrent 
> modprobes");
> @@ -108,6 +111,20 @@ static int call_modprobe(char *module_name, int wait)
> return -ENOMEM;
>  }
>
> +static int kmod_umh_threads_get(void)
> +{
> +   atomic_inc(_concurrent);
> +   if (atomic_read(_concurrent) < max_modprobes)
> +   return 0;
> +   atomic_dec(_concurrent);
> +   return -ENOMEM;
> +}
> +
> +static void kmod_umh_threads_put(void)
> +{
> +   atomic_dec(_concurrent);
> +}

Can you use a kref here instead? We're trying to kill raw use of
atomic_t for reference counting...

> +
>  /**
>   * __request_module - try to load a kernel module
>   * @wait: wait (or not) for the operation to complete
> @@ -129,7 +146,6 @@ int __request_module(bool wait, const char *fmt, ...)
> va_list args;
> char module_name[MODULE_NAME_LEN];
> int ret;
> -   static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> static int kmod_loop_msg;
>
> /*
> @@ -153,8 +169,8 @@ int __request_module(bool wait, const char *fmt, ...)
> if (ret)
> return ret;
>
> -   atomic_inc(_concurrent);
> -   if (atomic_read(_concurrent) > max_modprobes) {
> +   ret = kmod_umh_threads_get();
> +   if (ret) {
> /* We may be blaming an innocent here, but unlikely */
> if (kmod_loop_msg < 5) {
> printk(KERN_ERR
> @@ -162,15 +178,14 @@ int __request_module(bool wait, const char *fmt, ...)
>module_name);
> kmod_loop_msg++;
> }
> -   atomic_dec(_concurrent);
> -   return -ENOMEM;
> +   return ret;
> }
>
> trace_module_request(module_name, wait, _RET_IP_);
>
> ret = call_modprobe(module_name, wait ? UMH_WAIT_PROC : 
> UMH_WAIT_EXEC);
>
> -   atomic_dec(_concurrent);
> +   kmod_umh_threads_put();
> return ret;
>  }
>  EXPORT_SYMBOL(__request_module);
> --
> 2.10.1
>



-- 
Kees Cook
Nexus Security