Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1521929 > unrolled thread

[RFC][PATCH 0/7] kref improvements

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH 0/7] kref improvements Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 5/7] kref: Implement kref_put_lock() Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
      Re: [RFC][PATCH 5/7] kref: Implement kref_put_lock() Kees Cook <keescook@chromium.org> - 2016-11-14 21:40 +0100
        Re: [RFC][PATCH 5/7] kref: Implement kref_put_lock() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 09:00 +0100
    [RFC][PATCH 6/7] kref: Avoid more abuse Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 4/7] kref: Use kref_get_unless_zero() more Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
    [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-14 18:50 +0100
      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:50 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 10:50 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 11:10 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 11:50 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-15 14:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-15 19:10 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 20:20 +0100
                    Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-15 20:30 +0100
                      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-16 09:40 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 10:00 +0100
                          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-16 10:10 +0100
                            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 10:30 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:20 +0100
                          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-16 20:00 +0100
                            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 09:40 +0100
                              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-17 21:00 +0100
                        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Kees Cook <keescook@chromium.org> - 2016-11-16 19:50 +0100
      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-15 13:40 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-15 14:10 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-15 15:20 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 10:30 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-17 10:50 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 11:40 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 11:50 +0100
                    Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2016-11-17 12:10 +0100
                      Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:10 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:30 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-17 18:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-18 09:30 +0100
                  Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-18 11:20 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-17 18:10 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:30 +0100
      RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 11:10 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 12:40 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Will Deacon <will.deacon@arm.com> - 2016-11-18 18:10 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 20:00 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 05:10 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Ingo Molnar <mingo@kernel.org> - 2016-11-21 08:50 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 09:40 +0100
          Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 09:50 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-21 10:10 +0100
              Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Boqun Feng <boqun.feng@gmail.com> - 2016-11-21 10:40 +0100
      RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 11:50 +0100
        Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 12:00 +0100
          RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-18 18:00 +0100
            Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-18 20:00 +0100
              RE: [RFC][PATCH 7/7] kref: Implement using refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-19 08:20 +0100
                Re: [RFC][PATCH 7/7] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2016-11-19 12:50 +0100
    Re: [RFC][PATCH 2/7] kref: Add kref_read() Christoph Hellwig <hch@infradead.org> - 2016-11-14 19:20 +0100
      Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:30 +0100
        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 08:50 +0100
        [PATCH] printk, locking/atomics, kref: Introduce new %pAr and %pAk  format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:40 +0100
          [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 09:50 +0100
            Re: [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Peter Zijlstra <peterz@infradead.org> - 2016-11-15 10:30 +0100
              [PATCH v3] printk, locking/atomics, kref: Introduce new %pAa and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-15 10:50 +0100
            Re: [PATCH v2] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' kbuild test robot <lkp@intel.com> - 2016-11-15 11:10 +0100
          Re: [PATCH] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Linus Torvalds <torvalds@linux-foundation.org> - 2016-11-15 17:50 +0100
            Re: [PATCH] printk, locking/atomics, kref: Introduce new %pAr and  %pAk format string options for atomic_t and 'struct kref' Ingo Molnar <mingo@kernel.org> - 2016-11-16 09:20 +0100
    Re: [RFC][PATCH 0/7] kref improvements Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:30 +0100
      Re: [RFC][PATCH 0/7] kref improvements Ingo Molnar <mingo@kernel.org> - 2016-11-15 08:50 +0100
        Re: [RFC][PATCH 0/7] kref improvements Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 16:10 +0100
      Re: [RFC][PATCH 0/7] kref improvements Peter Zijlstra <peterz@infradead.org> - 2016-11-15 08:50 +0100
    Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-15 08:40 +0100
      Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-15 09:10 +0100
        Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-15 22:00 +0100
          Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 09:30 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:20 +0100
              Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 11:20 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Greg KH <gregkh@linuxfoundation.org> - 2016-11-16 11:20 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Daniel Borkmann <daniel@iogearbox.net> - 2016-11-16 11:20 +0100
          Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-16 11:10 +0100
            Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-16 20:00 +0100
              Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 09:40 +0100
                Re: [RFC][PATCH 2/7] kref: Add kref_read() David Windsor <dave@progbits.org> - 2016-11-17 13:50 +0100
                  RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 15:40 +0100
                    Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:10 +0100
                      RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 18:10 +0100
                      RE: [RFC][PATCH 2/7] kref: Add kref_read() "Reshetova, Elena" <elena.reshetova@intel.com> - 2016-11-17 19:10 +0100
                        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 20:20 +0100
                        Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 20:40 +0100
                  Re: [RFC][PATCH 2/7] kref: Add kref_read() Peter Zijlstra <peterz@infradead.org> - 2016-11-17 18:20 +0100
                Re: [RFC][PATCH 2/7] kref: Add kref_read() Kees Cook <keescook@chromium.org> - 2016-11-17 20:40 +0100

Page 1 of 5  [1] 2 3 4 5  Next page →

#1521929 — [RFC][PATCH 0/7] kref improvements

This series unfscks kref and then implements it in terms of refcount_t.

x86_64-allyesconfig compile tested and boot tested with my regular config.

refcount_t is as per the previous thread, it BUGs on over-/underflow and
saturates at UINT_MAX, such that if we ever overflow, we'll never free again.

[toc] | [next] | [standalone]

#1521932 — [RFC][PATCH 5/7] kref: Implement kref_put_lock()

Because home-rolling your own is _awesome_, stop doing it. Provide
kref_put_lock(), just like kref_put_mutex() but for a spinlock.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/kref.h |   21 +++++++++++++++------
 net/sunrpc/svcauth.c |   15 ++++++++++-----
 2 files changed, 25 insertions(+), 11 deletions(-)

--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -86,12 +86,21 @@ static inline int kref_put_mutex(struct
 				 struct mutex *lock)
 {
 	WARN_ON(release == NULL);
-	if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
-		mutex_lock(lock);
-		if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
-			mutex_unlock(lock);
-			return 0;
-		}
+
+	if (atomic_dec_and_mutex_lock(&kref->refcount, lock)) {
+		release(kref);
+		return 1;
+	}
+	return 0;
+}
+
+static inline int kref_put_lock(struct kref *kref,
+				void (*release)(struct kref *kref),
+				spinlock_t *lock)
+{
+	WARN_ON(release == NULL);
+
+	if (atomic_dec_and_lock(&kref->refcount, lock)) {
 		release(kref);
 		return 1;
 	}
--- a/net/sunrpc/svcauth.c
+++ b/net/sunrpc/svcauth.c
@@ -127,13 +127,18 @@ static struct hlist_head	auth_domain_tab
 static spinlock_t	auth_domain_lock =
 	__SPIN_LOCK_UNLOCKED(auth_domain_lock);
 
+static void auth_domain_release(struct kref *kref)
+{
+	struct auth_domain *dom = container_of(kref, struct auth_domain, ref);
+
+	hlist_del(&dom->hash);
+	dom->flavour->domain_release(dom);
+	spin_unlock(&auth_domain_lock);
+}
+
 void auth_domain_put(struct auth_domain *dom)
 {
-	if (atomic_dec_and_lock(&dom->ref.refcount, &auth_domain_lock)) {
-		hlist_del(&dom->hash);
-		dom->flavour->domain_release(dom);
-		spin_unlock(&auth_domain_lock);
-	}
+	kref_put_lock(&dom->ref, auth_domain_release, &auth_domain_lock);
 }
 EXPORT_SYMBOL_GPL(auth_domain_put);
 

[toc] | [prev] | [next] | [standalone]

#1522069 — Re: [RFC][PATCH 5/7] kref: Implement kref_put_lock()

On Mon, Nov 14, 2016 at 9:39 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> Because home-rolling your own is _awesome_, stop doing it. Provide
> kref_put_lock(), just like kref_put_mutex() but for a spinlock.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/kref.h |   21 +++++++++++++++------
>  net/sunrpc/svcauth.c |   15 ++++++++++-----
>  2 files changed, 25 insertions(+), 11 deletions(-)
>
> --- a/include/linux/kref.h
> +++ b/include/linux/kref.h
> @@ -86,12 +86,21 @@ static inline int kref_put_mutex(struct
>                                  struct mutex *lock)
>  {
>         WARN_ON(release == NULL);

This WARN_ON makes sense, yes, though it seems like it should be deal
with differently. If it's NULL, we'll just Oops when we call release()
later... Seems like this should saturate the kref or something else
similar.

> -       if (unlikely(!atomic_add_unless(&kref->refcount, -1, 1))) {
> -               mutex_lock(lock);
> -               if (unlikely(!atomic_dec_and_test(&kref->refcount))) {
> -                       mutex_unlock(lock);
> -                       return 0;
> -               }
> +
> +       if (atomic_dec_and_mutex_lock(&kref->refcount, lock)) {
> +               release(kref);
> +               return 1;
> +       }
> +       return 0;
> +}
> +
> +static inline int kref_put_lock(struct kref *kref,
> +                               void (*release)(struct kref *kref),
> +                               spinlock_t *lock)
> +{
> +       WARN_ON(release == NULL);
> +
> +       if (atomic_dec_and_lock(&kref->refcount, lock)) {
>                 release(kref);
>                 return 1;
>         }
> --- a/net/sunrpc/svcauth.c
> +++ b/net/sunrpc/svcauth.c
> @@ -127,13 +127,18 @@ static struct hlist_head  auth_domain_tab
>  static spinlock_t      auth_domain_lock =
>         __SPIN_LOCK_UNLOCKED(auth_domain_lock);
>
> +static void auth_domain_release(struct kref *kref)
> +{
> +       struct auth_domain *dom = container_of(kref, struct auth_domain, ref);
> +
> +       hlist_del(&dom->hash);
> +       dom->flavour->domain_release(dom);
> +       spin_unlock(&auth_domain_lock);
> +}
> +
>  void auth_domain_put(struct auth_domain *dom)
>  {
> -       if (atomic_dec_and_lock(&dom->ref.refcount, &auth_domain_lock)) {
> -               hlist_del(&dom->hash);
> -               dom->flavour->domain_release(dom);
> -               spin_unlock(&auth_domain_lock);
> -       }
> +       kref_put_lock(&dom->ref, auth_domain_release, &auth_domain_lock);
>  }
>  EXPORT_SYMBOL_GPL(auth_domain_put);
>
>
>



-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1522417 — Re: [RFC][PATCH 5/7] kref: Implement kref_put_lock()

On Mon, Nov 14, 2016 at 12:35:48PM -0800, Kees Cook wrote:
> On Mon, Nov 14, 2016 at 9:39 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > Because home-rolling your own is _awesome_, stop doing it. Provide
> > kref_put_lock(), just like kref_put_mutex() but for a spinlock.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  include/linux/kref.h |   21 +++++++++++++++------
> >  net/sunrpc/svcauth.c |   15 ++++++++++-----
> >  2 files changed, 25 insertions(+), 11 deletions(-)
> >
> > --- a/include/linux/kref.h
> > +++ b/include/linux/kref.h
> > @@ -86,12 +86,21 @@ static inline int kref_put_mutex(struct
> >                                  struct mutex *lock)
> >  {
> >         WARN_ON(release == NULL);
> 
> This WARN_ON makes sense, yes, though it seems like it should be deal
> with differently. If it's NULL, we'll just Oops when we call release()
> later... Seems like this should saturate the kref or something else
> similar.

So I simply took the pattern from the existing kref_put().

But I like it more in these kref_put_{lock,mutex}() variants, because
someone will need to unlock. If we simply crash/bug without unlock we'll
have broken state the rest of the kernel cannot fix up.

[toc] | [prev] | [next] | [standalone]

#1521933 — [RFC][PATCH 6/7] kref: Avoid more abuse

Leak references by unbalanced get, instead of poking at kref
implementation details.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/usb/mon/mon_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/usb/mon/mon_main.c
+++ b/drivers/usb/mon/mon_main.c
@@ -409,7 +409,7 @@ static void __exit mon_exit(void)
 			printk(KERN_ERR TAG
 			    ": Outstanding opens (%d) on usb%d, leaking...\n",
 			    mbus->nreaders, mbus->u_bus->busnum);
-			atomic_set(&mbus->ref.refcount, 2);	/* Force leak */
+			kref_get(&mbus->ref); /* Force leak */
 		}
 
 		mon_dissolve(mbus, mbus->u_bus);

[toc] | [prev] | [next] | [standalone]

#1521936 — [RFC][PATCH 4/7] kref: Use kref_get_unless_zero() more

For some obscure reason apparmor thinks its needs to locally implement
kref primitives that already exist. Stop doing this.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 security/apparmor/include/apparmor.h |    6 ------
 security/apparmor/include/policy.h   |    4 ++--
 2 files changed, 2 insertions(+), 8 deletions(-)

--- a/security/apparmor/include/apparmor.h
+++ b/security/apparmor/include/apparmor.h
@@ -78,12 +78,6 @@ static inline void *kvzalloc(size_t size
 	return __aa_kvmalloc(size, __GFP_ZERO);
 }
 
-/* returns 0 if kref not incremented */
-static inline int kref_get_not0(struct kref *kref)
-{
-	return atomic_inc_not_zero(&kref->refcount);
-}
-
 /**
  * aa_strneq - compare null terminated @str to a non null terminated substring
  * @str: a null terminated string
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -287,7 +287,7 @@ static inline struct aa_profile *aa_get_
  */
 static inline struct aa_profile *aa_get_profile_not0(struct aa_profile *p)
 {
-	if (p && kref_get_not0(&p->count))
+	if (p && kref_get_unless_zero(&p->count))
 		return p;
 
 	return NULL;
@@ -307,7 +307,7 @@ static inline struct aa_profile *aa_get_
 	rcu_read_lock();
 	do {
 		c = rcu_dereference(*p);
-	} while (c && !kref_get_not0(&c->count));
+	} while (c && !kref_get_unless_zero(&c->count));
 	rcu_read_unlock();
 
 	return c;

[toc] | [prev] | [next] | [standalone]

#1521937 — [RFC][PATCH 7/7] kref: Implement using refcount_t

Provide refcount_t, an atomic_t like primitive built just for
refcounting.

It provides overflow and underflow checks as well as saturation
semantics such that when it overflows, we'll never attempt to free it
again, ever.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/kref.h     |   29 ++----
 include/linux/refcount.h |  221 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 232 insertions(+), 18 deletions(-)

--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -15,16 +15,13 @@
 #ifndef _KREF_H_
 #define _KREF_H_
 
-#include <linux/bug.h>
-#include <linux/atomic.h>
-#include <linux/kernel.h>
-#include <linux/mutex.h>
+#include <linux/refcount.h>
 
 struct kref {
-	atomic_t refcount;
+	refcount_t refcount;
 };
 
-#define KREF_INIT(n)	{ .refcount = ATOMIC_INIT(n), }
+#define KREF_INIT(n)	{ .refcount = REFCOUNT_INIT(n), }
 
 /**
  * kref_init - initialize object.
@@ -32,12 +29,12 @@ struct kref {
  */
 static inline void kref_init(struct kref *kref)
 {
-	atomic_set(&kref->refcount, 1);
+	refcount_set(&kref->refcount, 1);
 }
 
-static inline int kref_read(const struct kref *kref)
+static inline unsigned int kref_read(const struct kref *kref)
 {
-	return atomic_read(&kref->refcount);
+	return refcount_read(&kref->refcount);
 }
 
 /**
@@ -46,11 +43,7 @@ static inline int kref_read(const struct
  */
 static inline void kref_get(struct kref *kref)
 {
-	/* If refcount was 0 before incrementing then we have a race
-	 * condition when this kref is freeing by some other thread right now.
-	 * In this case one should use kref_get_unless_zero()
-	 */
-	WARN_ON_ONCE(atomic_inc_return(&kref->refcount) < 2);
+	refcount_inc(&kref->refcount);
 }
 
 /**
@@ -74,7 +67,7 @@ static inline int kref_put(struct kref *
 {
 	WARN_ON(release == NULL);
 
-	if (atomic_dec_and_test(&kref->refcount)) {
+	if (refcount_dec_and_test(&kref->refcount)) {
 		release(kref);
 		return 1;
 	}
@@ -87,7 +80,7 @@ static inline int kref_put_mutex(struct
 {
 	WARN_ON(release == NULL);
 
-	if (atomic_dec_and_mutex_lock(&kref->refcount, lock)) {
+	if (refcount_dec_and_mutex_lock(&kref->refcount, lock)) {
 		release(kref);
 		return 1;
 	}
@@ -100,7 +93,7 @@ static inline int kref_put_lock(struct k
 {
 	WARN_ON(release == NULL);
 
-	if (atomic_dec_and_lock(&kref->refcount, lock)) {
+	if (refcount_dec_and_lock(&kref->refcount, lock)) {
 		release(kref);
 		return 1;
 	}
@@ -125,6 +118,6 @@ static inline int kref_put_lock(struct k
  */
 static inline int __must_check kref_get_unless_zero(struct kref *kref)
 {
-	return atomic_add_unless(&kref->refcount, 1, 0);
+	return refcount_inc_not_zero(&kref->refcount);
 }
 #endif /* _KREF_H_ */
--- /dev/null
+++ b/include/linux/refcount.h
@@ -0,0 +1,221 @@
+#ifndef _LINUX_REFCOUNT_H
+#define _LINUX_REFCOUNT_H
+
+/*
+ * Variant of atomic_t specialized for refcounting.
+ *
+ * The interface matches the atomic_t interface (to aid in porting) but only
+ * provides the few functions one should use for refcounting.
+ *
+ * They add explicit overflow and underflow tests, once a refcount hits
+ * UINT_MAX it stays there.
+ *
+ * Memory ordering rules are slightly relaxed wrt regular atomic_t functions
+ * and provide only what is strictly required for refcounts.
+ *
+ * The increments are fully relaxed; these will not provide ordering. The
+ * rationale is that whatever is used to obtain the object we're increasing the
+ * reference count on will provide the ordering. For locked data structures,
+ * its the lock acquire, for RCU/lockless data structures its the dependent
+ * read.
+ *
+ * Do note that inc_not_zero() provides a control dependency which will order
+ * future stores against the inc, this ensures we'll never modify the object
+ * if we did not in fact acquire a reference.
+ *
+ * The decrements will provide release order, such that all the prior loads and
+ * stores will be issued before we proceed with freeing the object.
+ *
+ */
+
+#include <linux/atomic.h>
+#include <linux/bug.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+
+typedef struct refcount_struct {
+	atomic_t refs;
+} refcount_t;
+
+#define REFCOUNT_INIT(n)	{ .refs = ATOMIC_INIT(n), }
+
+static inline void refcount_set(refcount_t *r, int n)
+{
+	atomic_set(&r->refs, n);
+}
+
+static inline unsigned int refcount_read(const refcount_t *r)
+{
+	return atomic_read(&r->refs);
+}
+
+/*
+ * Similar to atomic_inc(), will BUG on overflow and saturate at UINT_MAX.
+ *
+ * Provides no memory ordering, it is assumed the caller already has a
+ * reference on the object, will WARN when this is not so.
+ */
+static inline void refcount_inc(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		WARN_ON_ONCE(!val);
+
+		new = val + 1;
+		if (new < val)
+			BUG(); /* overflow */
+
+		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+}
+
+/*
+ * Similar to atomic_inc_not_zero(), will BUG on overflow and saturate at UINT_MAX.
+ *
+ * Provides no memory ordering, it is assumed the caller has guaranteed the
+ * object memory to be stable (RCU, etc.). It does provide a control dependency
+ * and thereby orders future stores.
+ */
+static inline __must_check
+bool refcount_inc_not_zero(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		if (!val)
+			return false;
+
+		new = val + 1;
+		if (new < val)
+			BUG(); /* overflow */
+
+		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	return true;
+}
+
+/*
+ * Similar to atomic_dec_and_test(), it will BUG on underflow and fail to
+ * decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before a subsequent free.
+ */
+static inline __must_check
+bool refcount_dec_and_test(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		if (val == UINT_MAX)
+			return false;
+
+		new = val - 1;
+		if (new > val)
+			BUG(); /* underflow */
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	return !new;
+}
+
+/*
+ * Similar to atomic_dec_and_mutex_lock(), it will BUG on underflow and fail
+ * to decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before a subsequent free. This allows free() while holding the mutex.
+ */
+static inline __must_check
+bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+	bool locked = false;
+
+	for (;;) {
+		if (val == UINT_MAX)
+			return false;
+
+		if (val == 1 && !locked) {
+			locked = true;
+			mutex_lock(lock);
+		}
+
+		new = val - 1;
+		if (new > val) {
+			if (locked)
+				mutex_unlock(lock);
+			BUG(); /* underflow */
+		}
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	if (new && locked)
+		mutex_unlock(lock);
+
+	return !new;
+}
+
+/*
+ * Similar to atomic_dec_and_lock(), it will BUG on underflow and fail
+ * to decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before a subsequent free. This allows free() while holding the lock.
+ */
+static inline __must_check
+bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+	bool locked = false;
+
+	for (;;) {
+		if (val == UINT_MAX)
+			return false;
+
+		if (val == 1 && !locked) {
+			locked = true;
+			spin_lock(lock);
+		}
+
+		new = val - 1;
+		if (new > val) {
+			if (locked)
+				spin_unlock(lock);
+			BUG(); /* underflow */
+		}
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	if (new && locked)
+		spin_unlock(lock);
+
+	return !new;
+}
+
+#endif /* _LINUX_REFCOUNT_H */

[toc] | [prev] | [next] | [standalone]

#1522444 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

* Peter Zijlstra <peterz@infradead.org> wrote:

> Provide refcount_t, an atomic_t like primitive built just for
> refcounting.
> 
> It provides overflow and underflow checks as well as saturation
> semantics such that when it overflows, we'll never attempt to free it
> again, ever.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/kref.h     |   29 ++----
>  include/linux/refcount.h |  221 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 232 insertions(+), 18 deletions(-)

I'd suggest splitting this patch into two parts: first patch introduces the 
refcount.h facility, second patch changes over struct kref to the new facility.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1522494 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Tue, Nov 15, 2016 at 09:40:09AM +0100, Ingo Molnar wrote:
> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Provide refcount_t, an atomic_t like primitive built just for
> > refcounting.
> > 
> > It provides overflow and underflow checks as well as saturation
> > semantics such that when it overflows, we'll never attempt to free it
> > again, ever.
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  include/linux/kref.h     |   29 ++----
> >  include/linux/refcount.h |  221 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 232 insertions(+), 18 deletions(-)
> 
> I'd suggest splitting this patch into two parts: first patch introduces the 
> refcount.h facility, second patch changes over struct kref to the new facility.

You're right, I was just really glad I got it to compile and didn't want
to prod more at it.

Should I also make a CONFIG knob that implements refcount_t with the
'normal' atomic_t primitives?

And possibly another knob to toggle the BUG()s into WARN()s. With the
full saturation semantics WARN() is a lot safer and will not corrupt
kernel state as much.

[toc] | [prev] | [next] | [standalone]

#1522509 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Nov 15, 2016 at 09:40:09AM +0100, Ingo Molnar wrote:
> > 
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > Provide refcount_t, an atomic_t like primitive built just for
> > > refcounting.
> > > 
> > > It provides overflow and underflow checks as well as saturation
> > > semantics such that when it overflows, we'll never attempt to free it
> > > again, ever.
> > > 
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > >  include/linux/kref.h     |   29 ++----
> > >  include/linux/refcount.h |  221 +++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 232 insertions(+), 18 deletions(-)
> > 
> > I'd suggest splitting this patch into two parts: first patch introduces the 
> > refcount.h facility, second patch changes over struct kref to the new facility.
> 
> You're right, I was just really glad I got it to compile and didn't want
> to prod more at it.
>
> Should I also make a CONFIG knob that implements refcount_t with the
> 'normal' atomic_t primitives?

I'd suggest doing the saturation/safe-wrap semantics only for now (i.e. the 
current patch, split into two perhaps), and reconsider if there's any complaints?

> And possibly another knob to toggle the BUG()s into WARN()s. With the
> full saturation semantics WARN() is a lot safer and will not corrupt
> kernel state as much.

I'd suggest changing it to a WARN() straight away, no extra knobs.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1522560 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Tue, Nov 15, 2016 at 11:03:59AM +0100, Ingo Molnar wrote:
> > Should I also make a CONFIG knob that implements refcount_t with the
> > 'normal' atomic_t primitives?
> 
> I'd suggest doing the saturation/safe-wrap semantics only for now (i.e. the 
> current patch, split into two perhaps), and reconsider if there's any complaints?
> 
> > And possibly another knob to toggle the BUG()s into WARN()s. With the
> > full saturation semantics WARN() is a lot safer and will not corrupt
> > kernel state as much.
> 
> I'd suggest changing it to a WARN() straight away, no extra knobs.

OK, a little like so then? Note that the overflow tests went away
because increments guarantee we saturate before we overflow.

---
Subject: refcount_t: A special purpose refcount type
From: Peter Zijlstra <peterz@infradead.org>
Date: Mon Nov 14 18:06:19 CET 2016

Provide refcount_t, an atomic_t like primitive built just for
refcounting.

It provides saturation semantics such that overflow becomes impossible
and thereby 'spurious' use-after-free is avoided.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/refcount.h |  229 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 229 insertions(+)

--- /dev/null
+++ b/include/linux/refcount.h
@@ -0,0 +1,229 @@
+#ifndef _LINUX_REFCOUNT_H
+#define _LINUX_REFCOUNT_H
+
+/*
+ * Variant of atomic_t specialized for refcounting.
+ *
+ * The interface matches the atomic_t interface (to aid in porting) but only
+ * provides the few functions one should use for refcounting.
+ *
+ * It differs in that the counter saturates at UINT_MAX and will not move once
+ * there. This avoids wrapping the counter and causing 'spurious'
+ * use-after-free issues.
+ *
+ * Memory ordering rules are slightly relaxed wrt regular atomic_t functions
+ * and provide only what is strictly required for refcounts.
+ *
+ * The increments are fully relaxed; these will not provide ordering. The
+ * rationale is that whatever is used to obtain the object we're increasing the
+ * reference count on will provide the ordering. For locked data structures,
+ * its the lock acquire, for RCU/lockless data structures its the dependent
+ * load.
+ *
+ * Do note that inc_not_zero() provides a control dependency which will order
+ * future stores against the inc, this ensures we'll never modify the object
+ * if we did not in fact acquire a reference.
+ *
+ * The decrements will provide release order, such that all the prior loads and
+ * stores will be issued before we proceed with freeing the object.
+ *
+ * Note: the implementation hard relies on increments, bigger than 1 additions
+ *       need explicit overflow -> saturation logic.
+ *
+ */
+
+#include <linux/atomic.h>
+#include <linux/bug.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+
+typedef struct refcount_struct {
+	atomic_t refs;
+} refcount_t;
+
+#define REFCOUNT_INIT(n)	{ .refs = ATOMIC_INIT(n), }
+
+static inline void refcount_set(refcount_t *r, int n)
+{
+	atomic_set(&r->refs, n);
+}
+
+static inline unsigned int refcount_read(const refcount_t *r)
+{
+	return atomic_read(&r->refs);
+}
+
+/*
+ * Similar to atomic_inc(), will saturate at UINT_MAX and WARN.
+ *
+ * Provides no memory ordering, it is assumed the caller already has a
+ * reference on the object, will WARN when this is not so.
+ */
+static inline void refcount_inc(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		WARN(!val, "refcount_t: increment on 0; use-after-free.\n");
+
+		if (unlikely(val == UINT_MAX))
+			return;
+
+		new = val + 1;
+		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+}
+
+/*
+ * Similar to atomic_inc_not_zero(), will saturate at UINT_MAX and WARN.
+ *
+ * Provides no memory ordering, it is assumed the caller has guaranteed the
+ * object memory to be stable (RCU, etc.). It does provide a control dependency
+ * and thereby orders future stores.
+ */
+static inline __must_check
+bool refcount_inc_not_zero(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		if (!val)
+			return false;
+
+		if (unlikely(val == UINT_MAX))
+			return true;
+
+		new = val + 1;
+		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+
+	return true;
+}
+
+/*
+ * Similar to atomic_dec_and_test(), it will WARN on underflow and fail to
+ * decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before a subsequent free.
+ */
+static inline __must_check
+bool refcount_dec_and_test(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		if (val == UINT_MAX)
+			return false;
+
+		new = val - 1;
+		if (WARN(new > val, "refcount_t: underflow; use-after-free.\n"))
+			return false;
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	return !new;
+}
+
+/*
+ * Similar to atomic_dec_and_mutex_lock(), it will WARN on underflow and fail
+ * to decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before a subsequent free. This allows free() while holding the mutex.
+ */
+static inline __must_check
+bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+	bool locked = false;
+
+	for (;;) {
+		if (val == UINT_MAX)
+			return false;
+
+		if (val == 1 && !locked) {
+			locked = true;
+			mutex_lock(lock);
+		}
+
+		new = val - 1;
+		if (WARN(new > val, "refcount_t: underflow; use-after-free.\n")) {
+			if (locked)
+				mutex_unlock(lock);
+			return false;
+		}
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	if (new && locked)
+		mutex_unlock(lock);
+
+	return !new;
+}
+
+/*
+ * Similar to atomic_dec_and_lock(), it will WARN on underflow and fail to
+ * decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before a subsequent free. This allows free() while holding the lock.
+ */
+static inline __must_check
+bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+	bool locked = false;
+
+	for (;;) {
+		if (val == UINT_MAX)
+			return false;
+
+		if (val == 1 && !locked) {
+			locked = true;
+			spin_lock(lock);
+		}
+
+		new = val - 1;
+		if (WARN(new > val, "refcount_t: underflow; use-after-free.\n")) {
+			if (locked)
+				mutex_unlock(lock);
+			return false;
+		}
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	if (new && locked)
+		spin_unlock(lock);
+
+	return !new;
+}
+
+#endif /* _LINUX_REFCOUNT_H */

[toc] | [prev] | [next] | [standalone]

#1522656 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Nov 15, 2016 at 11:03:59AM +0100, Ingo Molnar wrote:
> > > Should I also make a CONFIG knob that implements refcount_t with the
> > > 'normal' atomic_t primitives?
> > 
> > I'd suggest doing the saturation/safe-wrap semantics only for now (i.e. the 
> > current patch, split into two perhaps), and reconsider if there's any complaints?
> > 
> > > And possibly another knob to toggle the BUG()s into WARN()s. With the
> > > full saturation semantics WARN() is a lot safer and will not corrupt
> > > kernel state as much.
> > 
> > I'd suggest changing it to a WARN() straight away, no extra knobs.
> 
> OK, a little like so then? Note that the overflow tests went away
> because increments guarantee we saturate before we overflow.

Looks good to me!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1522969 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Tue, Nov 15, 2016 at 5:03 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Peter Zijlstra <peterz@infradead.org> wrote:
>
>> On Tue, Nov 15, 2016 at 11:03:59AM +0100, Ingo Molnar wrote:
>> > > Should I also make a CONFIG knob that implements refcount_t with the
>> > > 'normal' atomic_t primitives?
>> >
>> > I'd suggest doing the saturation/safe-wrap semantics only for now (i.e. the
>> > current patch, split into two perhaps), and reconsider if there's any complaints?
>> >
>> > > And possibly another knob to toggle the BUG()s into WARN()s. With the
>> > > full saturation semantics WARN() is a lot safer and will not corrupt
>> > > kernel state as much.

I'll want to modify this in the future; I have a config already doing
"Bug on data structure corruption" that makes the warn/bug choice.
It'll need some massaging to fit into the new refcount_t checks, but
it should be okay -- there needs to be a way to complete the
saturation, etc, but still kill the offending process group.

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1523024 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On 15 November 2016 19:06:28 CET, Kees Cook <keescook@chromium.org> wrote:

>I'll want to modify this in the future; I have a config already doing
>"Bug on data structure corruption" that makes the warn/bug choice.
>It'll need some massaging to fit into the new refcount_t checks, but
>it should be okay -- there needs to be a way to complete the
>saturation, etc, but still kill the offending process group.

Ideally we'd create a new WARN like construct that continues in kernel space and terminates the process on return to user. That way there would be minimal kernel state corruption.

[toc] | [prev] | [next] | [standalone]

#1523026 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Tue, Nov 15, 2016 at 11:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
>
> On 15 November 2016 19:06:28 CET, Kees Cook <keescook@chromium.org> wrote:
>
>>I'll want to modify this in the future; I have a config already doing
>>"Bug on data structure corruption" that makes the warn/bug choice.
>>It'll need some massaging to fit into the new refcount_t checks, but
>>it should be okay -- there needs to be a way to complete the
>>saturation, etc, but still kill the offending process group.
>
> Ideally we'd create a new WARN like construct that continues in kernel space and terminates the process on return to user. That way there would be minimal kernel state corruption.

Right, though I'd like to be conservative about the kernel execution
continuing... I'll experiment with it.

-Kees

-- 
Kees Cook
Nexus Security

[toc] | [prev] | [next] | [standalone]

#1523297 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

* Kees Cook <keescook@chromium.org> wrote:

> On Tue, Nov 15, 2016 at 11:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >
> > On 15 November 2016 19:06:28 CET, Kees Cook <keescook@chromium.org> wrote:
> >
> >>I'll want to modify this in the future; I have a config already doing
> >>"Bug on data structure corruption" that makes the warn/bug choice.
> >>It'll need some massaging to fit into the new refcount_t checks, but
> >>it should be okay -- there needs to be a way to complete the
> >>saturation, etc, but still kill the offending process group.
> >
> > Ideally we'd create a new WARN like construct that continues in kernel space 
> > and terminates the process on return to user. That way there would be minimal 
> > kernel state corruption.

Yeah, so the problem is that sometimes you are p0wned the moment you return to a 
corrupted stack, and some of these checks only detect corruption after the fact.

> Right, though I'd like to be conservative about the kernel execution 
> continuing... I'll experiment with it.

So what I'd love to see is to have a kernel option that re-introduces some 
historic root (and other) holes that can be exploited deterministically - 
obviously default disabled.

I'd restrict this to reasonably 'deterministic' holes, and the exploits themselves 
could be somewhere in tools/. (Obviously only where the maintainers agree to host 
the code.) They wouldn't give a root shell, they'd only test whether they reached 
uid0 (or some other elevated privilege).

The advantages of such a suite would be:

 - Uptodate tests on modern kernels: It would allow the (controlled) testing of 
   live kernel exploits even on the latest kernel - and would allow the testing of 
   various defensive measures.

 - It would also make sure that defensive measures _remain_ effective against 
   similar categories of bugs. We've had defensive measure regressions in the 
   past, which was only discovered when the next exploit came out ...

 - Testing of new defensive measures: It would help convert this whole 
   probabilistic and emotion driven "kernel protection" business into something 
   somewhat more rational. For example new protection mechanisms should have a 
   demonstrated ability to turn an existing exploit test into something less 
   dangerous.

 - Education: It would teach kernel developers the various patterns of holes, 
   right in the code. Maybe being more directly exposed to what can get you p0wned 
   is both a stronger education force plus it could give people ideas about how to 
   protect better.

 - I also think that collecting the various problems into a single place will give 
   us new insights into patterns, bug counts and various exploit techniques.

The disadvantages would be:

 - Maintenance: do we want to add extra (compiled out by default) code to the 
   kernel whose only purpose is to demonstrate certain types of bugs?

 - Exposing exploits: Do we want to host a powerful collection of almost-exploits 
   in tools/ ? I don't think we have a choice but to face the problem directly - 
   but others might disagree.

I think most of the negatives could be kept small by starting small, allowing 
maintainers to explicitly opt-in, and observing the effects as we go. But YMMV.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1523315 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Wed, Nov 16, 2016 at 09:31:55AM +0100, Ingo Molnar wrote:
> So what I'd love to see is to have a kernel option that re-introduces some 
> historic root (and other) holes that can be exploited deterministically - 
> obviously default disabled.

Ick, I don't want to have to support nasty #ifdefs for
"CONFIG_TOTALLY_INSECURE" type options in code logic for the next 20+
years, do you?

> I'd restrict this to reasonably 'deterministic' holes, and the exploits themselves 
> could be somewhere in tools/. (Obviously only where the maintainers agree to host 
> the code.) They wouldn't give a root shell, they'd only test whether they reached 
> uid0 (or some other elevated privilege).

Having exploits in tools/ would be good, I would like to see that, as
then we can ensure that we don't ever introduce old problems that we
have fixed again in the future.  That I have no objection to.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1523316 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

* Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Nov 16, 2016 at 09:31:55AM +0100, Ingo Molnar wrote:
> > So what I'd love to see is to have a kernel option that re-introduces some 
> > historic root (and other) holes that can be exploited deterministically - 
> > obviously default disabled.
> 
> Ick, I don't want to have to support nasty #ifdefs for
> "CONFIG_TOTALLY_INSECURE" type options in code logic for the next 20+
> years, do you?

I'd write it in C, not CPP, so it would be an 'if', but yeah, it would be extra 
code otherwise.

So I'd restrict this strictly to cases:

 - Where the maintainer absolutely agrees to carry it.

 - Where it's still easy to do technically - for example a single unobtrusive 
   'if' condition or so, in cases where the current upstream code still has a 
   similar structure conductive to the re-introducion of the bug. Such testcases
   can be dropped the moment they interfere with active development.

 - Plus an additional approach could be that some of the typical holes can be
   reproduced in completely separate code that is not seen by anyone who doesn't 
   want to see it.

I doubt many bugs have 20 years life times in face of frequent code reorganization 
- and if code is static for 20 years then there won't be much extra maintenance 
overhead, right?

> > I'd restrict this to reasonably 'deterministic' holes, and the exploits themselves 
> > could be somewhere in tools/. (Obviously only where the maintainers agree to host 
> > the code.) They wouldn't give a root shell, they'd only test whether they reached 
> > uid0 (or some other elevated privilege).
> 
> Having exploits in tools/ would be good, I would like to see that, as
> then we can ensure that we don't ever introduce old problems that we
> have fixed again in the future.  That I have no objection to.

Heh, I actually guessed that this would be the more contentious part of my 
suggestion - go figure! ;-)

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1523327 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Wed, Nov 16, 2016 at 10:07:37AM +0100, Ingo Molnar wrote:
> 
> * Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Nov 16, 2016 at 09:31:55AM +0100, Ingo Molnar wrote:
> > > So what I'd love to see is to have a kernel option that re-introduces some 
> > > historic root (and other) holes that can be exploited deterministically - 
> > > obviously default disabled.
> > 
> > Ick, I don't want to have to support nasty #ifdefs for
> > "CONFIG_TOTALLY_INSECURE" type options in code logic for the next 20+
> > years, do you?
> 
> I'd write it in C, not CPP, so it would be an 'if', but yeah, it would be extra 
> code otherwise.
> 
> So I'd restrict this strictly to cases:
> 
>  - Where the maintainer absolutely agrees to carry it.
> 
>  - Where it's still easy to do technically - for example a single unobtrusive 
>    'if' condition or so, in cases where the current upstream code still has a 
>    similar structure conductive to the re-introducion of the bug. Such testcases
>    can be dropped the moment they interfere with active development.
> 
>  - Plus an additional approach could be that some of the typical holes can be
>    reproduced in completely separate code that is not seen by anyone who doesn't 
>    want to see it.

Ok, but in looking at a number of "security" fixes over the past year or
so, I don't think that many of them would really work well for this.
Just look at all of the "don't reference a NULL pointer" bugs for an
example of that.

> I doubt many bugs have 20 years life times in face of frequent code reorganization 
> - and if code is static for 20 years then there won't be much extra maintenance 
> overhead, right?

Hah, you obviously are not in charge of maintaining the tty layer :)

Anyway, if you want to try this for the next type of security "issue" in
your area of the kernel, be my guest, but I think it's going to be a lot
harder than you think.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1523355 — Re: [RFC][PATCH 7/7] kref: Implement using refcount_t

On Wed, Nov 16, 2016 at 09:31:55AM +0100, Ingo Molnar wrote:
> 
> * Kees Cook <keescook@chromium.org> wrote:
> 
> > On Tue, Nov 15, 2016 at 11:16 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > >
> > > On 15 November 2016 19:06:28 CET, Kees Cook <keescook@chromium.org> wrote:
> > >
> > >>I'll want to modify this in the future; I have a config already doing
> > >>"Bug on data structure corruption" that makes the warn/bug choice.
> > >>It'll need some massaging to fit into the new refcount_t checks, but
> > >>it should be okay -- there needs to be a way to complete the
> > >>saturation, etc, but still kill the offending process group.
> > >
> > > Ideally we'd create a new WARN like construct that continues in kernel space 
> > > and terminates the process on return to user. That way there would be minimal 
> > > kernel state corruption.
> 
> Yeah, so the problem is that sometimes you are p0wned the moment you return to a 
> corrupted stack, and some of these checks only detect corruption after the fact.

So the case here is about refcounts, with the saturation semantics we
avoid the use-after-free case which is all this is about. So actually
continuation of execution is harmless vs the attack vector in question.

Corrupting the stack is another attack vector, one that refcount
overflow is entirely unrelated to and not one I think we should consider
here.

The problem with BUG and insta killing the task is that refcounts are
typically done under locks, if you kill the task before the unlock,
you've wrecked kernel state in unrecoverable ways.

[toc] | [prev] | [next] | [standalone]

Page 1 of 5  [1] 2 3 4 5  Next page →

Back to top | Article view | linux.kernel

csiph-web