[PATCH] kcov: convert kcov.refcount to refcount_t

[PATCH] kcov: convert kcov.refcount to refcount_t

[Elena Reshetova]

atomic_t variables are currently used to implement reference
counters with the following properties:
 - counter is initialized to 1 using atomic_set()
 - a resource is freed upon counter reaching zero
 - once counter reaches zero, its further
   increments aren't allowed
 - counter schema uses basic atomic operations
   (set, inc, inc_not_zero, dec_and_test, etc.)

Such atomic variables should be converted to a newly provided
refcount_t type and API that prevents accidental counter overflows
and underflows. This is important since overflows and underflows
can lead to use-after-free situation and be exploitable.

The variable kcov.refcount is used as pure reference counter.
Convert it to refcount_t and fix up the operations.

**Important note for maintainers:

Some functions from refcount_t API defined in lib/refcount.c
have different memory ordering guarantees than their atomic
counterparts.
The full comparison can be seen in
https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
in state to be merged to the documentation tree.
Normally the differences should not matter since refcount_t provides
enough guarantees to satisfy the refcounting use cases, but in
some rare cases it might matter.
Please double check that you don't have some undocumented
memory guarantees for this variable usage.

For the kcov.refcount it might make a difference
in following places:
 - kcov_put(): decrement in refcount_dec_and_test() only
   provides RELEASE ordering and control dependency on success
   vs. fully ordered atomic counterpart

Suggested-by: Kees Cook <keescook@chromium.org>
Reviewed-by: David Windsor <dwindsor@gmail.com>
Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
---
 kernel/kcov.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index c2277db..051e86e 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -20,6 +20,7 @@
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
 #include <linux/kcov.h>
+#include <linux/refcount.h>
 #include <asm/setup.h>
 
 /* Number of 64-bit words written per one comparison: */
@@ -44,7 +45,7 @@ struct kcov {
 	 *  - opened file descriptor
 	 *  - task with enabled coverage (we can't unwire it from another task)
 	 */
-	atomic_t		refcount;
+	refcount_t		refcount;
 	/* The lock protects mode, size, area and t. */
 	spinlock_t		lock;
 	enum kcov_mode		mode;
@@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
 
 static void kcov_get(struct kcov *kcov)
 {
-	atomic_inc(&kcov->refcount);
+	refcount_inc(&kcov->refcount);
 }
 
 static void kcov_put(struct kcov *kcov)
 {
-	if (atomic_dec_and_test(&kcov->refcount)) {
+	if (refcount_dec_and_test(&kcov->refcount)) {
 		vfree(kcov->area);
 		kfree(kcov);
 	}
@@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
 	if (!kcov)
 		return -ENOMEM;
 	kcov->mode = KCOV_MODE_DISABLED;
-	atomic_set(&kcov->refcount, 1);
+	refcount_set(&kcov->refcount, 1);
 	spin_lock_init(&kcov->lock);
 	filep->private_data = kcov;
 	return nonseekable_open(inode, filep);
-- 
2.7.4

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

On Wed, Jan 16, 2019 at 11:27 AM Elena Reshetova
<elena.reshetova@intel.com> wrote:
>
> atomic_t variables are currently used to implement reference
> counters with the following properties:
>  - counter is initialized to 1 using atomic_set()
>  - a resource is freed upon counter reaching zero
>  - once counter reaches zero, its further
>    increments aren't allowed
>  - counter schema uses basic atomic operations
>    (set, inc, inc_not_zero, dec_and_test, etc.)
>
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
>
> The variable kcov.refcount is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
>
> **Important note for maintainers:
>
> Some functions from refcount_t API defined in lib/refcount.c
> have different memory ordering guarantees than their atomic
> counterparts.
> The full comparison can be seen in
> https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
> in state to be merged to the documentation tree.
> Normally the differences should not matter since refcount_t provides
> enough guarantees to satisfy the refcounting use cases, but in
> some rare cases it might matter.
> Please double check that you don't have some undocumented
> memory guarantees for this variable usage.
>
> For the kcov.refcount it might make a difference
> in following places:
>  - kcov_put(): decrement in refcount_dec_and_test() only
>    provides RELEASE ordering and control dependency on success
>    vs. fully ordered atomic counterpart

Reviewed-by: Dmitry Vyukov <dvyukov@google.com>

Thanks for improving this.

KCOV uses refcounts in a very simple canonical way, so no hidden
ordering implied.

Am I missing something or refcount_dec_and_test does not in fact
provide ACQUIRE ordering?

+case 5) - decrement-based RMW ops that return a value
+-----------------------------------------------------
+
+Function changes:
+                atomic_dec_and_test() --> refcount_dec_and_test()
+                atomic_sub_and_test() --> refcount_sub_and_test()
+                no atomic counterpart --> refcount_dec_if_one()
+                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
+
+Memory ordering guarantees changes:
+                fully ordered --> RELEASE ordering + control dependency

I think that's against the expected refcount guarantees. When I
privatize an  atomic_dec_and_test I would expect that not only stores,
but also loads act on a quiescent object. But loads can hoist outside
of the control dependency.

Consider the following example, is it the case that the BUG_ON can still fire?

struct X {
  refcount_t rc; // == 2
  int done1, done2; // == 0
};

// thread 1:
x->done1 = 1;
if (refcount_dec_and_test(&x->rc))
  BUG_ON(!x->done2);

// thread 2:
x->done2 = 1;
if (refcount_dec_and_test(&x->rc))
  BUG_ON(!x->done1);



> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> ---
>  kernel/kcov.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index c2277db..051e86e 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -20,6 +20,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/uaccess.h>
>  #include <linux/kcov.h>
> +#include <linux/refcount.h>
>  #include <asm/setup.h>
>
>  /* Number of 64-bit words written per one comparison: */
> @@ -44,7 +45,7 @@ struct kcov {
>          *  - opened file descriptor
>          *  - task with enabled coverage (we can't unwire it from another task)
>          */
> -       atomic_t                refcount;
> +       refcount_t              refcount;
>         /* The lock protects mode, size, area and t. */
>         spinlock_t              lock;
>         enum kcov_mode          mode;
> @@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
>
>  static void kcov_get(struct kcov *kcov)
>  {
> -       atomic_inc(&kcov->refcount);
> +       refcount_inc(&kcov->refcount);
>  }
>
>  static void kcov_put(struct kcov *kcov)
>  {
> -       if (atomic_dec_and_test(&kcov->refcount)) {
> +       if (refcount_dec_and_test(&kcov->refcount)) {
>                 vfree(kcov->area);
>                 kfree(kcov);
>         }
> @@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
>         if (!kcov)
>                 return -ENOMEM;
>         kcov->mode = KCOV_MODE_DISABLED;
> -       atomic_set(&kcov->refcount, 1);
> +       refcount_set(&kcov->refcount, 1);
>         spin_lock_init(&kcov->lock);
>         filep->private_data = kcov;
>         return nonseekable_open(inode, filep);
> --
> 2.7.4
>

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

On Wed, Jan 16, 2019 at 1:51 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Wed, Jan 16, 2019 at 11:27 AM Elena Reshetova
> <elena.reshetova@intel.com> wrote:
> >
> > atomic_t variables are currently used to implement reference
> > counters with the following properties:
> >  - counter is initialized to 1 using atomic_set()
> >  - a resource is freed upon counter reaching zero
> >  - once counter reaches zero, its further
> >    increments aren't allowed
> >  - counter schema uses basic atomic operations
> >    (set, inc, inc_not_zero, dec_and_test, etc.)
> >
> > Such atomic variables should be converted to a newly provided
> > refcount_t type and API that prevents accidental counter overflows
> > and underflows. This is important since overflows and underflows
> > can lead to use-after-free situation and be exploitable.
> >
> > The variable kcov.refcount is used as pure reference counter.
> > Convert it to refcount_t and fix up the operations.
> >
> > **Important note for maintainers:
> >
> > Some functions from refcount_t API defined in lib/refcount.c
> > have different memory ordering guarantees than their atomic
> > counterparts.
> > The full comparison can be seen in
> > https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
> > in state to be merged to the documentation tree.
> > Normally the differences should not matter since refcount_t provides
> > enough guarantees to satisfy the refcounting use cases, but in
> > some rare cases it might matter.
> > Please double check that you don't have some undocumented
> > memory guarantees for this variable usage.
> >
> > For the kcov.refcount it might make a difference
> > in following places:
> >  - kcov_put(): decrement in refcount_dec_and_test() only
> >    provides RELEASE ordering and control dependency on success
> >    vs. fully ordered atomic counterpart
>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>
> Thanks for improving this.
>
> KCOV uses refcounts in a very simple canonical way, so no hidden
> ordering implied.
>
> Am I missing something or refcount_dec_and_test does not in fact
> provide ACQUIRE ordering?
>
> +case 5) - decrement-based RMW ops that return a value
> +-----------------------------------------------------
> +
> +Function changes:
> +                atomic_dec_and_test() --> refcount_dec_and_test()
> +                atomic_sub_and_test() --> refcount_sub_and_test()
> +                no atomic counterpart --> refcount_dec_if_one()
> +                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
> +
> +Memory ordering guarantees changes:
> +                fully ordered --> RELEASE ordering + control dependency
>
> I think that's against the expected refcount guarantees. When I
> privatize an  atomic_dec_and_test I would expect that not only stores,
> but also loads act on a quiescent object. But loads can hoist outside
> of the control dependency.
>
> Consider the following example, is it the case that the BUG_ON can still fire?
>
> struct X {
>   refcount_t rc; // == 2
>   int done1, done2; // == 0
> };
>
> // thread 1:
> x->done1 = 1;
> if (refcount_dec_and_test(&x->rc))
>   BUG_ON(!x->done2);
>
> // thread 2:
> x->done2 = 1;
> if (refcount_dec_and_test(&x->rc))
>   BUG_ON(!x->done1);

+more people knowledgeable in memory ordering

Unfortunately I can't find a way to reply to the
Documentation/core-api/refcount-vs-atomic.rst patch review thread.

The refcount_dec_and_test guarantees look too weak to me, see the
example above. Shouldn't refcount_dec_and_test returning true give the
object in fully quiescent state? Why only control dependency? Loads
can hoist across control dependency, no?



> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: David Windsor <dwindsor@gmail.com>
> > Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > ---
> >  kernel/kcov.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index c2277db..051e86e 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/kcov.h>
> > +#include <linux/refcount.h>
> >  #include <asm/setup.h>
> >
> >  /* Number of 64-bit words written per one comparison: */
> > @@ -44,7 +45,7 @@ struct kcov {
> >          *  - opened file descriptor
> >          *  - task with enabled coverage (we can't unwire it from another task)
> >          */
> > -       atomic_t                refcount;
> > +       refcount_t              refcount;
> >         /* The lock protects mode, size, area and t. */
> >         spinlock_t              lock;
> >         enum kcov_mode          mode;
> > @@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
> >
> >  static void kcov_get(struct kcov *kcov)
> >  {
> > -       atomic_inc(&kcov->refcount);
> > +       refcount_inc(&kcov->refcount);
> >  }
> >
> >  static void kcov_put(struct kcov *kcov)
> >  {
> > -       if (atomic_dec_and_test(&kcov->refcount)) {
> > +       if (refcount_dec_and_test(&kcov->refcount)) {
> >                 vfree(kcov->area);
> >                 kfree(kcov);
> >         }
> > @@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
> >         if (!kcov)
> >                 return -ENOMEM;
> >         kcov->mode = KCOV_MODE_DISABLED;
> > -       atomic_set(&kcov->refcount, 1);
> > +       refcount_set(&kcov->refcount, 1);
> >         spin_lock_init(&kcov->lock);
> >         filep->private_data = kcov;
> >         return nonseekable_open(inode, filep);
> > --
> > 2.7.4
> >

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Andrea Parri]

On Mon, Jan 21, 2019 at 10:52:37AM +0100, Dmitry Vyukov wrote:

[...]

> > Am I missing something or refcount_dec_and_test does not in fact
> > provide ACQUIRE ordering?
> >
> > +case 5) - decrement-based RMW ops that return a value
> > +-----------------------------------------------------
> > +
> > +Function changes:
> > +                atomic_dec_and_test() --> refcount_dec_and_test()
> > +                atomic_sub_and_test() --> refcount_sub_and_test()
> > +                no atomic counterpart --> refcount_dec_if_one()
> > +                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
> > +
> > +Memory ordering guarantees changes:
> > +                fully ordered --> RELEASE ordering + control dependency
> >
> > I think that's against the expected refcount guarantees. When I
> > privatize an  atomic_dec_and_test I would expect that not only stores,
> > but also loads act on a quiescent object. But loads can hoist outside
> > of the control dependency.
> >
> > Consider the following example, is it the case that the BUG_ON can still fire?

Can't it fire in an SC world? (wrong example, or a Monday morning? ;D)


> >
> > struct X {
> >   refcount_t rc; // == 2
> >   int done1, done2; // == 0
> > };
> >
> > // thread 1:
> > x->done1 = 1;
> > if (refcount_dec_and_test(&x->rc))
> >   BUG_ON(!x->done2);
> >
> > // thread 2:
> > x->done2 = 1;
> > if (refcount_dec_and_test(&x->rc))
> >   BUG_ON(!x->done1);
> 
> +more people knowledgeable in memory ordering
> 
> Unfortunately I can't find a way to reply to the
> Documentation/core-api/refcount-vs-atomic.rst patch review thread.
> 
> The refcount_dec_and_test guarantees look too weak to me, see the
> example above. Shouldn't refcount_dec_and_test returning true give the
> object in fully quiescent state? Why only control dependency? Loads
> can hoist across control dependency, no?

As you remarked, the doc. says CTRL+RELEASE (so yes, loads can hoist);
AFAICR, implementations do comply to this requirement.

(FWIW, I sometimes think at this "weird" ordering as a weak "acq_rel",
 the latter, acq_rel, being missing from the current APIs.)

 Andrea


> 
> 
> 
> > > Suggested-by: Kees Cook <keescook@chromium.org>
> > > Reviewed-by: David Windsor <dwindsor@gmail.com>
> > > Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > ---
> > >  kernel/kcov.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > > index c2277db..051e86e 100644
> > > --- a/kernel/kcov.c
> > > +++ b/kernel/kcov.c
> > > @@ -20,6 +20,7 @@
> > >  #include <linux/debugfs.h>
> > >  #include <linux/uaccess.h>
> > >  #include <linux/kcov.h>
> > > +#include <linux/refcount.h>
> > >  #include <asm/setup.h>
> > >
> > >  /* Number of 64-bit words written per one comparison: */
> > > @@ -44,7 +45,7 @@ struct kcov {
> > >          *  - opened file descriptor
> > >          *  - task with enabled coverage (we can't unwire it from another task)
> > >          */
> > > -       atomic_t                refcount;
> > > +       refcount_t              refcount;
> > >         /* The lock protects mode, size, area and t. */
> > >         spinlock_t              lock;
> > >         enum kcov_mode          mode;
> > > @@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
> > >
> > >  static void kcov_get(struct kcov *kcov)
> > >  {
> > > -       atomic_inc(&kcov->refcount);
> > > +       refcount_inc(&kcov->refcount);
> > >  }
> > >
> > >  static void kcov_put(struct kcov *kcov)
> > >  {
> > > -       if (atomic_dec_and_test(&kcov->refcount)) {
> > > +       if (refcount_dec_and_test(&kcov->refcount)) {
> > >                 vfree(kcov->area);
> > >                 kfree(kcov);
> > >         }
> > > @@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
> > >         if (!kcov)
> > >                 return -ENOMEM;
> > >         kcov->mode = KCOV_MODE_DISABLED;
> > > -       atomic_set(&kcov->refcount, 1);
> > > +       refcount_set(&kcov->refcount, 1);
> > >         spin_lock_init(&kcov->lock);
> > >         filep->private_data = kcov;
> > >         return nonseekable_open(inode, filep);
> > > --
> > > 2.7.4
> > >

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Andrea Parri]

On Wed, Jan 16, 2019 at 12:27:09PM +0200, Elena Reshetova wrote:
> atomic_t variables are currently used to implement reference
> counters with the following properties:
>  - counter is initialized to 1 using atomic_set()
>  - a resource is freed upon counter reaching zero
>  - once counter reaches zero, its further
>    increments aren't allowed
>  - counter schema uses basic atomic operations
>    (set, inc, inc_not_zero, dec_and_test, etc.)
> 
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
> 
> The variable kcov.refcount is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
> 
> **Important note for maintainers:
> 
> Some functions from refcount_t API defined in lib/refcount.c
> have different memory ordering guarantees than their atomic
> counterparts.
> The full comparison can be seen in
> https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
> in state to be merged to the documentation tree.
> Normally the differences should not matter since refcount_t provides
> enough guarantees to satisfy the refcounting use cases, but in
> some rare cases it might matter.
> Please double check that you don't have some undocumented
> memory guarantees for this variable usage.
> 
> For the kcov.refcount it might make a difference
> in following places:
>  - kcov_put(): decrement in refcount_dec_and_test() only
>    provides RELEASE ordering and control dependency on success
>    vs. fully ordered atomic counterpart
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>

Reviewed-by: Andrea Parri <andrea.parri@amarulasolutions.com>

(Same remark about the reference in the commit message. ;-) )

  Andrea


> ---
>  kernel/kcov.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index c2277db..051e86e 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -20,6 +20,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/uaccess.h>
>  #include <linux/kcov.h>
> +#include <linux/refcount.h>
>  #include <asm/setup.h>
>  
>  /* Number of 64-bit words written per one comparison: */
> @@ -44,7 +45,7 @@ struct kcov {
>  	 *  - opened file descriptor
>  	 *  - task with enabled coverage (we can't unwire it from another task)
>  	 */
> -	atomic_t		refcount;
> +	refcount_t		refcount;
>  	/* The lock protects mode, size, area and t. */
>  	spinlock_t		lock;
>  	enum kcov_mode		mode;
> @@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
>  
>  static void kcov_get(struct kcov *kcov)
>  {
> -	atomic_inc(&kcov->refcount);
> +	refcount_inc(&kcov->refcount);
>  }
>  
>  static void kcov_put(struct kcov *kcov)
>  {
> -	if (atomic_dec_and_test(&kcov->refcount)) {
> +	if (refcount_dec_and_test(&kcov->refcount)) {
>  		vfree(kcov->area);
>  		kfree(kcov);
>  	}
> @@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
>  	if (!kcov)
>  		return -ENOMEM;
>  	kcov->mode = KCOV_MODE_DISABLED;
> -	atomic_set(&kcov->refcount, 1);
> +	refcount_set(&kcov->refcount, 1);
>  	spin_lock_init(&kcov->lock);
>  	filep->private_data = kcov;
>  	return nonseekable_open(inode, filep);
> -- 
> 2.7.4
> 

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

 On Mon, Jan 21, 2019 at 12:45 PM Andrea Parri
<andrea.parri@amarulasolutions.com> wrote:
>
> On Mon, Jan 21, 2019 at 10:52:37AM +0100, Dmitry Vyukov wrote:
>
> [...]
>
> > > Am I missing something or refcount_dec_and_test does not in fact
> > > provide ACQUIRE ordering?
> > >
> > > +case 5) - decrement-based RMW ops that return a value
> > > +-----------------------------------------------------
> > > +
> > > +Function changes:
> > > +                atomic_dec_and_test() --> refcount_dec_and_test()
> > > +                atomic_sub_and_test() --> refcount_sub_and_test()
> > > +                no atomic counterpart --> refcount_dec_if_one()
> > > +                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
> > > +
> > > +Memory ordering guarantees changes:
> > > +                fully ordered --> RELEASE ordering + control dependency
> > >
> > > I think that's against the expected refcount guarantees. When I
> > > privatize an  atomic_dec_and_test I would expect that not only stores,
> > > but also loads act on a quiescent object. But loads can hoist outside
> > > of the control dependency.
> > >
> > > Consider the following example, is it the case that the BUG_ON can still fire?
>
> Can't it fire in an SC world? (wrong example, or a Monday morning? ;D)

I don't see how. Maybe there is a stupid off-by-one, but overall
that's the example I wanted to show. refcount is 2, each thread sets
own done flag, drops refcount, last thread checks done flag of the
other thread.



> > > struct X {
> > >   refcount_t rc; // == 2
> > >   int done1, done2; // == 0
> > > };
> > >
> > > // thread 1:
> > > x->done1 = 1;
> > > if (refcount_dec_and_test(&x->rc))
> > >   BUG_ON(!x->done2);
> > >
> > > // thread 2:
> > > x->done2 = 1;
> > > if (refcount_dec_and_test(&x->rc))
> > >   BUG_ON(!x->done1);
> >
> > +more people knowledgeable in memory ordering
> >
> > Unfortunately I can't find a way to reply to the
> > Documentation/core-api/refcount-vs-atomic.rst patch review thread.
> >
> > The refcount_dec_and_test guarantees look too weak to me, see the
> > example above. Shouldn't refcount_dec_and_test returning true give the
> > object in fully quiescent state? Why only control dependency? Loads
> > can hoist across control dependency, no?
>
> As you remarked, the doc. says CTRL+RELEASE (so yes, loads can hoist);
> AFAICR, implementations do comply to this requirement.
>
> (FWIW, I sometimes think at this "weird" ordering as a weak "acq_rel",
>  the latter, acq_rel, being missing from the current APIs.)

But doesn't it break like half of use cases?

Iv'e skimmed through few uses. This read of db_info->views after
refcount_dec_and_test looks like potential unordered read:
https://elixir.bootlin.com/linux/v5.0-rc3/source/arch/s390/kernel/debug.c#L412

This read of vdata->maddr after refcount_dec_and_test looks like
potential unordered read:
https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/char/mspec.c#L171

This read of buf->sgt_base looks like potential unordered read:
https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/media/common/videobuf2/videobuf2-dma-contig.c#L129

Also this:
https://elixir.bootlin.com/linux/v5.0-rc3/source/fs/btrfs/scrub.c#L544
and this:
https://elixir.bootlin.com/linux/v5.0-rc3/source/fs/nfs/inode.c#L992

For each case it's quite hard to prove if there is anything else that
provides read ordering, or if the field was initialized before the
object was first published and then never changed. But overall, why
are we making it so error-prone and subtle?


> > > > Suggested-by: Kees Cook <keescook@chromium.org>
> > > > Reviewed-by: David Windsor <dwindsor@gmail.com>
> > > > Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> > > > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
> > > > ---
> > > >  kernel/kcov.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > > > index c2277db..051e86e 100644
> > > > --- a/kernel/kcov.c
> > > > +++ b/kernel/kcov.c
> > > > @@ -20,6 +20,7 @@
> > > >  #include <linux/debugfs.h>
> > > >  #include <linux/uaccess.h>
> > > >  #include <linux/kcov.h>
> > > > +#include <linux/refcount.h>
> > > >  #include <asm/setup.h>
> > > >
> > > >  /* Number of 64-bit words written per one comparison: */
> > > > @@ -44,7 +45,7 @@ struct kcov {
> > > >          *  - opened file descriptor
> > > >          *  - task with enabled coverage (we can't unwire it from another task)
> > > >          */
> > > > -       atomic_t                refcount;
> > > > +       refcount_t              refcount;
> > > >         /* The lock protects mode, size, area and t. */
> > > >         spinlock_t              lock;
> > > >         enum kcov_mode          mode;
> > > > @@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
> > > >
> > > >  static void kcov_get(struct kcov *kcov)
> > > >  {
> > > > -       atomic_inc(&kcov->refcount);
> > > > +       refcount_inc(&kcov->refcount);
> > > >  }
> > > >
> > > >  static void kcov_put(struct kcov *kcov)
> > > >  {
> > > > -       if (atomic_dec_and_test(&kcov->refcount)) {
> > > > +       if (refcount_dec_and_test(&kcov->refcount)) {
> > > >                 vfree(kcov->area);
> > > >                 kfree(kcov);
> > > >         }
> > > > @@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
> > > >         if (!kcov)
> > > >                 return -ENOMEM;
> > > >         kcov->mode = KCOV_MODE_DISABLED;
> > > > -       atomic_set(&kcov->refcount, 1);
> > > > +       refcount_set(&kcov->refcount, 1);
> > > >         spin_lock_init(&kcov->lock);
> > > >         filep->private_data = kcov;
> > > >         return nonseekable_open(inode, filep);
> > > > --
> > > > 2.7.4
> > > >

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Mark Rutland]

On Wed, Jan 16, 2019 at 12:27:09PM +0200, Elena Reshetova wrote:
> atomic_t variables are currently used to implement reference
> counters with the following properties:
>  - counter is initialized to 1 using atomic_set()
>  - a resource is freed upon counter reaching zero
>  - once counter reaches zero, its further
>    increments aren't allowed
>  - counter schema uses basic atomic operations
>    (set, inc, inc_not_zero, dec_and_test, etc.)
> 
> Such atomic variables should be converted to a newly provided
> refcount_t type and API that prevents accidental counter overflows
> and underflows. This is important since overflows and underflows
> can lead to use-after-free situation and be exploitable.
> 
> The variable kcov.refcount is used as pure reference counter.
> Convert it to refcount_t and fix up the operations.
> 
> **Important note for maintainers:
> 
> Some functions from refcount_t API defined in lib/refcount.c
> have different memory ordering guarantees than their atomic
> counterparts.
> The full comparison can be seen in
> https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
> in state to be merged to the documentation tree.
> Normally the differences should not matter since refcount_t provides
> enough guarantees to satisfy the refcounting use cases, but in
> some rare cases it might matter.
> Please double check that you don't have some undocumented
> memory guarantees for this variable usage.
> 
> For the kcov.refcount it might make a difference
> in following places:
>  - kcov_put(): decrement in refcount_dec_and_test() only
>    provides RELEASE ordering and control dependency on success
>    vs. fully ordered atomic counterpart
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: David Windsor <dwindsor@gmail.com>
> Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>

Just to check, has this been tested with CONFIG_REFCOUNT_FULL and
something poking kcov?

Given lib/refcount.c is instrumented, the refcount_*() calls will
recurse back into the kcov code. It looks like that's fine, given these
are only manipulated in setup/teardown paths, but it would be nice to be
sure.

Thanks,
Mark.

> ---
>  kernel/kcov.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index c2277db..051e86e 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -20,6 +20,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/uaccess.h>
>  #include <linux/kcov.h>
> +#include <linux/refcount.h>
>  #include <asm/setup.h>
>  
>  /* Number of 64-bit words written per one comparison: */
> @@ -44,7 +45,7 @@ struct kcov {
>  	 *  - opened file descriptor
>  	 *  - task with enabled coverage (we can't unwire it from another task)
>  	 */
> -	atomic_t		refcount;
> +	refcount_t		refcount;
>  	/* The lock protects mode, size, area and t. */
>  	spinlock_t		lock;
>  	enum kcov_mode		mode;
> @@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
>  
>  static void kcov_get(struct kcov *kcov)
>  {
> -	atomic_inc(&kcov->refcount);
> +	refcount_inc(&kcov->refcount);
>  }
>  
>  static void kcov_put(struct kcov *kcov)
>  {
> -	if (atomic_dec_and_test(&kcov->refcount)) {
> +	if (refcount_dec_and_test(&kcov->refcount)) {
>  		vfree(kcov->area);
>  		kfree(kcov);
>  	}
> @@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
>  	if (!kcov)
>  		return -ENOMEM;
>  	kcov->mode = KCOV_MODE_DISABLED;
> -	atomic_set(&kcov->refcount, 1);
> +	refcount_set(&kcov->refcount, 1);
>  	spin_lock_init(&kcov->lock);
>  	filep->private_data = kcov;
>  	return nonseekable_open(inode, filep);
> -- 
> 2.7.4
> 

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

On Mon, Jan 21, 2019 at 1:38 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 16, 2019 at 12:27:09PM +0200, Elena Reshetova wrote:
> > atomic_t variables are currently used to implement reference
> > counters with the following properties:
> >  - counter is initialized to 1 using atomic_set()
> >  - a resource is freed upon counter reaching zero
> >  - once counter reaches zero, its further
> >    increments aren't allowed
> >  - counter schema uses basic atomic operations
> >    (set, inc, inc_not_zero, dec_and_test, etc.)
> >
> > Such atomic variables should be converted to a newly provided
> > refcount_t type and API that prevents accidental counter overflows
> > and underflows. This is important since overflows and underflows
> > can lead to use-after-free situation and be exploitable.
> >
> > The variable kcov.refcount is used as pure reference counter.
> > Convert it to refcount_t and fix up the operations.
> >
> > **Important note for maintainers:
> >
> > Some functions from refcount_t API defined in lib/refcount.c
> > have different memory ordering guarantees than their atomic
> > counterparts.
> > The full comparison can be seen in
> > https://lkml.org/lkml/2017/11/15/57 and it is hopefully soon
> > in state to be merged to the documentation tree.
> > Normally the differences should not matter since refcount_t provides
> > enough guarantees to satisfy the refcounting use cases, but in
> > some rare cases it might matter.
> > Please double check that you don't have some undocumented
> > memory guarantees for this variable usage.
> >
> > For the kcov.refcount it might make a difference
> > in following places:
> >  - kcov_put(): decrement in refcount_dec_and_test() only
> >    provides RELEASE ordering and control dependency on success
> >    vs. fully ordered atomic counterpart
> >
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Reviewed-by: David Windsor <dwindsor@gmail.com>
> > Reviewed-by: Hans Liljestrand <ishkamiel@gmail.com>
> > Signed-off-by: Elena Reshetova <elena.reshetova@intel.com>
>
> Just to check, has this been tested with CONFIG_REFCOUNT_FULL and
> something poking kcov?
>
> Given lib/refcount.c is instrumented, the refcount_*() calls will
> recurse back into the kcov code. It looks like that's fine, given these
> are only manipulated in setup/teardown paths, but it would be nice to be
> sure.

A simple program using KCOV is available here:
https://elixir.bootlin.com/linux/v5.0-rc3/source/Documentation/dev-tools/kcov.rst#L42
or here (it's like strace but collects and prints KCOV coverage):
https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c

> Thanks,
> Mark.
>
> > ---
> >  kernel/kcov.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/kcov.c b/kernel/kcov.c
> > index c2277db..051e86e 100644
> > --- a/kernel/kcov.c
> > +++ b/kernel/kcov.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/kcov.h>
> > +#include <linux/refcount.h>
> >  #include <asm/setup.h>
> >
> >  /* Number of 64-bit words written per one comparison: */
> > @@ -44,7 +45,7 @@ struct kcov {
> >        *  - opened file descriptor
> >        *  - task with enabled coverage (we can't unwire it from another task)
> >        */
> > -     atomic_t                refcount;
> > +     refcount_t              refcount;
> >       /* The lock protects mode, size, area and t. */
> >       spinlock_t              lock;
> >       enum kcov_mode          mode;
> > @@ -228,12 +229,12 @@ EXPORT_SYMBOL(__sanitizer_cov_trace_switch);
> >
> >  static void kcov_get(struct kcov *kcov)
> >  {
> > -     atomic_inc(&kcov->refcount);
> > +     refcount_inc(&kcov->refcount);
> >  }
> >
> >  static void kcov_put(struct kcov *kcov)
> >  {
> > -     if (atomic_dec_and_test(&kcov->refcount)) {
> > +     if (refcount_dec_and_test(&kcov->refcount)) {
> >               vfree(kcov->area);
> >               kfree(kcov);
> >       }
> > @@ -312,7 +313,7 @@ static int kcov_open(struct inode *inode, struct file *filep)
> >       if (!kcov)
> >               return -ENOMEM;
> >       kcov->mode = KCOV_MODE_DISABLED;
> > -     atomic_set(&kcov->refcount, 1);
> > +     refcount_set(&kcov->refcount, 1);
> >       spin_lock_init(&kcov->lock);
> >       filep->private_data = kcov;
> >       return nonseekable_open(inode, filep);
> > --
> > 2.7.4
> >

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Peter Zijlstra]

On Mon, Jan 21, 2019 at 10:52:37AM +0100, Dmitry Vyukov wrote:
> On Wed, Jan 16, 2019 at 1:51 PM Dmitry Vyukov <dvyukov@google.com> wrote:

> > KCOV uses refcounts in a very simple canonical way, so no hidden
> > ordering implied.
> >
> > Am I missing something or refcount_dec_and_test does not in fact
> > provide ACQUIRE ordering?
> >
> > +case 5) - decrement-based RMW ops that return a value
> > +-----------------------------------------------------
> > +
> > +Function changes:
> > +                atomic_dec_and_test() --> refcount_dec_and_test()
> > +                atomic_sub_and_test() --> refcount_sub_and_test()
> > +                no atomic counterpart --> refcount_dec_if_one()
> > +                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
> > +
> > +Memory ordering guarantees changes:
> > +                fully ordered --> RELEASE ordering + control dependency
> >
> > I think that's against the expected refcount guarantees. When I
> > privatize an  atomic_dec_and_test I would expect that not only stores,
> > but also loads act on a quiescent object. But loads can hoist outside
> > of the control dependency.
> >
> > Consider the following example, is it the case that the BUG_ON can still fire?
> >
> > struct X {
> >   refcount_t rc; // == 2
> >   int done1, done2; // == 0
> > };
> >
> > // thread 1:
> > x->done1 = 1;
> > if (refcount_dec_and_test(&x->rc))
> >   BUG_ON(!x->done2);
> >
> > // thread 2:
> > x->done2 = 1;
> > if (refcount_dec_and_test(&x->rc))
> >   BUG_ON(!x->done1);

I'm the one responsible for that refcount_t ordering.

The rationale for REL+CTRL is that for the final put we want to ensure
all prior load/store are complete, because any later access could be a
UAF; consider:


P0()
{
	x->foo=0;
	if (refcount_dec_and_test(&x->rc))
		free(x);
}

P1()
{
	x->bar=1;
	if (refcount_dec_and_test(&->rc))
		free(x);
}


without release, if would be possible for either (foo,bar) store to
happen after the free().

Additionally we also need the CTRL to ensure that the actual free()
happens _after_ the dec_and_test, freeing early would be bad.

But after these two requirements, the basic refcounting works.

> The refcount_dec_and_test guarantees look too weak to me, see the
> example above. Shouldn't refcount_dec_and_test returning true give the
> object in fully quiescent state? Why only control dependency? Loads
> can hoist across control dependency, no?

Yes, loads can escape like you say.

Any additional ordering; like the one you have above; are not strictly
required for the proper functioning of the refcount. Rather, you rely on
additional ordering and will need to provide this explicitly:


	if (refcount_dec_and_text(&x->rc)) {
		/*
		 * Comment that explains what we order against....
		 */
		smp_mb__after_atomic();
		BUG_ON(!x->done*);
		free(x);
	}


Also; these patches explicitly mention that refcount_t is weaker,
specifically to make people aware of this difference.

A full smp_mb() (or two) would also be much more expensive on a number
of platforms and in the vast majority of the cases it is not required.

RE: [PATCH] kcov: convert kcov.refcount to refcount_t

[Reshetova, Elena]

 > Just to check, has this been tested with CONFIG_REFCOUNT_FULL and
> > something poking kcov?
> >
> > Given lib/refcount.c is instrumented, the refcount_*() calls will
> > recurse back into the kcov code. It looks like that's fine, given these
> > are only manipulated in setup/teardown paths, but it would be nice to be
> > sure.
> 
> A simple program using KCOV is available here:
> https://elixir.bootlin.com/linux/v5.0-rc3/source/Documentation/dev-
> tools/kcov.rst#L42
> or here (it's like strace but collects and prints KCOV coverage):
> https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c
> 

No, this one hasn't gone via any particular testing apart
the tests that zero day runs automatically (like boot tests, etc.) since normally
it is hard for me to know how exactly to test a particular component in a meaningful
way. 

But I can try to test with the above example now, if it helps!

Best Regards,
Elena.

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Andrea Parri]

On Mon, Jan 21, 2019 at 01:29:11PM +0100, Dmitry Vyukov wrote:
>  On Mon, Jan 21, 2019 at 12:45 PM Andrea Parri
> <andrea.parri@amarulasolutions.com> wrote:
> >
> > On Mon, Jan 21, 2019 at 10:52:37AM +0100, Dmitry Vyukov wrote:
> >
> > [...]
> >
> > > > Am I missing something or refcount_dec_and_test does not in fact
> > > > provide ACQUIRE ordering?
> > > >
> > > > +case 5) - decrement-based RMW ops that return a value
> > > > +-----------------------------------------------------
> > > > +
> > > > +Function changes:
> > > > +                atomic_dec_and_test() --> refcount_dec_and_test()
> > > > +                atomic_sub_and_test() --> refcount_sub_and_test()
> > > > +                no atomic counterpart --> refcount_dec_if_one()
> > > > +                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
> > > > +
> > > > +Memory ordering guarantees changes:
> > > > +                fully ordered --> RELEASE ordering + control dependency
> > > >
> > > > I think that's against the expected refcount guarantees. When I
> > > > privatize an  atomic_dec_and_test I would expect that not only stores,
> > > > but also loads act on a quiescent object. But loads can hoist outside
> > > > of the control dependency.
> > > >
> > > > Consider the following example, is it the case that the BUG_ON can still fire?
> >
> > Can't it fire in an SC world? (wrong example, or a Monday morning? ;D)
> 
> I don't see how. Maybe there is a stupid off-by-one, but overall
> that's the example I wanted to show. refcount is 2, each thread sets
> own done flag, drops refcount, last thread checks done flag of the
> other thread.

You're right: looking at the example again, I think that the BUG_ON()
in your example can indeed trigger with a CTRL+RELEASE semantics (but
_not with the fully-ordered semantics).

I apologize for the confusion (it must have been _my Monday...  ;-/).

  Andrea


> 
> 
> 
> > > > struct X {
> > > >   refcount_t rc; // == 2
> > > >   int done1, done2; // == 0
> > > > };
> > > >
> > > > // thread 1:
> > > > x->done1 = 1;
> > > > if (refcount_dec_and_test(&x->rc))
> > > >   BUG_ON(!x->done2);
> > > >
> > > > // thread 2:
> > > > x->done2 = 1;
> > > > if (refcount_dec_and_test(&x->rc))
> > > >   BUG_ON(!x->done1);

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Alan Stern]

On Mon, 21 Jan 2019, Peter Zijlstra wrote:

> On Mon, Jan 21, 2019 at 10:52:37AM +0100, Dmitry Vyukov wrote:
> > On Wed, Jan 16, 2019 at 1:51 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> 
> > > KCOV uses refcounts in a very simple canonical way, so no hidden
> > > ordering implied.
> > >
> > > Am I missing something or refcount_dec_and_test does not in fact
> > > provide ACQUIRE ordering?
> > >
> > > +case 5) - decrement-based RMW ops that return a value
> > > +-----------------------------------------------------
> > > +
> > > +Function changes:
> > > +                atomic_dec_and_test() --> refcount_dec_and_test()
> > > +                atomic_sub_and_test() --> refcount_sub_and_test()
> > > +                no atomic counterpart --> refcount_dec_if_one()
> > > +                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
> > > +
> > > +Memory ordering guarantees changes:
> > > +                fully ordered --> RELEASE ordering + control dependency
> > >
> > > I think that's against the expected refcount guarantees. When I
> > > privatize an  atomic_dec_and_test I would expect that not only stores,
> > > but also loads act on a quiescent object. But loads can hoist outside
> > > of the control dependency.
> > >
> > > Consider the following example, is it the case that the BUG_ON can still fire?
> > >
> > > struct X {
> > >   refcount_t rc; // == 2
> > >   int done1, done2; // == 0
> > > };
> > >
> > > // thread 1:
> > > x->done1 = 1;
> > > if (refcount_dec_and_test(&x->rc))
> > >   BUG_ON(!x->done2);
> > >
> > > // thread 2:
> > > x->done2 = 1;
> > > if (refcount_dec_and_test(&x->rc))
> > >   BUG_ON(!x->done1);
> 
> I'm the one responsible for that refcount_t ordering.
> 
> The rationale for REL+CTRL is that for the final put we want to ensure
> all prior load/store are complete, because any later access could be a
> UAF; consider:
> 
> 
> P0()
> {
> 	x->foo=0;
> 	if (refcount_dec_and_test(&x->rc))
> 		free(x);
> }
> 
> P1()
> {
> 	x->bar=1;
> 	if (refcount_dec_and_test(&->rc))
> 		free(x);
> }
> 
> 
> without release, if would be possible for either (foo,bar) store to
> happen after the free().
> 
> Additionally we also need the CTRL to ensure that the actual free()
> happens _after_ the dec_and_test, freeing early would be bad.
> 
> But after these two requirements, the basic refcounting works.
> 
> > The refcount_dec_and_test guarantees look too weak to me, see the
> > example above. Shouldn't refcount_dec_and_test returning true give the
> > object in fully quiescent state? Why only control dependency? Loads
> > can hoist across control dependency, no?
> 
> Yes, loads can escape like you say.
> 
> Any additional ordering; like the one you have above; are not strictly
> required for the proper functioning of the refcount. Rather, you rely on
> additional ordering and will need to provide this explicitly:
> 
> 
> 	if (refcount_dec_and_text(&x->rc)) {
> 		/*
> 		 * Comment that explains what we order against....
> 		 */
> 		smp_mb__after_atomic();
> 		BUG_ON(!x->done*);
> 		free(x);
> 	}
> 
> 
> Also; these patches explicitly mention that refcount_t is weaker,
> specifically to make people aware of this difference.
> 
> A full smp_mb() (or two) would also be much more expensive on a number
> of platforms and in the vast majority of the cases it is not required.

How about adding smp_rmb() into refcount_dec_and_test()?  That would
give acq+rel semantics, which seems to be what people will expect.  And
it wouldn't be nearly as expensive as a full smp_mb().

Alan Stern

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

On Mon, Jan 21, 2019 at 5:05 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, 21 Jan 2019, Peter Zijlstra wrote:
>
> > On Mon, Jan 21, 2019 at 10:52:37AM +0100, Dmitry Vyukov wrote:
> > > On Wed, Jan 16, 2019 at 1:51 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > > > KCOV uses refcounts in a very simple canonical way, so no hidden
> > > > ordering implied.
> > > >
> > > > Am I missing something or refcount_dec_and_test does not in fact
> > > > provide ACQUIRE ordering?
> > > >
> > > > +case 5) - decrement-based RMW ops that return a value
> > > > +-----------------------------------------------------
> > > > +
> > > > +Function changes:
> > > > +                atomic_dec_and_test() --> refcount_dec_and_test()
> > > > +                atomic_sub_and_test() --> refcount_sub_and_test()
> > > > +                no atomic counterpart --> refcount_dec_if_one()
> > > > +                atomic_add_unless(&var, -1, 1) --> refcount_dec_not_one(&var)
> > > > +
> > > > +Memory ordering guarantees changes:
> > > > +                fully ordered --> RELEASE ordering + control dependency
> > > >
> > > > I think that's against the expected refcount guarantees. When I
> > > > privatize an  atomic_dec_and_test I would expect that not only stores,
> > > > but also loads act on a quiescent object. But loads can hoist outside
> > > > of the control dependency.
> > > >
> > > > Consider the following example, is it the case that the BUG_ON can still fire?
> > > >
> > > > struct X {
> > > >   refcount_t rc; // == 2
> > > >   int done1, done2; // == 0
> > > > };
> > > >
> > > > // thread 1:
> > > > x->done1 = 1;
> > > > if (refcount_dec_and_test(&x->rc))
> > > >   BUG_ON(!x->done2);
> > > >
> > > > // thread 2:
> > > > x->done2 = 1;
> > > > if (refcount_dec_and_test(&x->rc))
> > > >   BUG_ON(!x->done1);
> >
> > I'm the one responsible for that refcount_t ordering.
> >
> > The rationale for REL+CTRL is that for the final put we want to ensure
> > all prior load/store are complete, because any later access could be a
> > UAF; consider:
> >
> >
> > P0()
> > {
> >       x->foo=0;
> >       if (refcount_dec_and_test(&x->rc))
> >               free(x);
> > }
> >
> > P1()
> > {
> >       x->bar=1;
> >       if (refcount_dec_and_test(&->rc))
> >               free(x);
> > }
> >
> >
> > without release, if would be possible for either (foo,bar) store to
> > happen after the free().
> >
> > Additionally we also need the CTRL to ensure that the actual free()
> > happens _after_ the dec_and_test, freeing early would be bad.
> >
> > But after these two requirements, the basic refcounting works.
> >
> > > The refcount_dec_and_test guarantees look too weak to me, see the
> > > example above. Shouldn't refcount_dec_and_test returning true give the
> > > object in fully quiescent state? Why only control dependency? Loads
> > > can hoist across control dependency, no?
> >
> > Yes, loads can escape like you say.
> >
> > Any additional ordering; like the one you have above; are not strictly
> > required for the proper functioning of the refcount. Rather, you rely on
> > additional ordering and will need to provide this explicitly:
> >
> >
> >       if (refcount_dec_and_text(&x->rc)) {
> >               /*
> >                * Comment that explains what we order against....
> >                */
> >               smp_mb__after_atomic();
> >               BUG_ON(!x->done*);
> >               free(x);
> >       }
> >
> >
> > Also; these patches explicitly mention that refcount_t is weaker,
> > specifically to make people aware of this difference.
> >
> > A full smp_mb() (or two) would also be much more expensive on a number
> > of platforms and in the vast majority of the cases it is not required.
>
> How about adding smp_rmb() into refcount_dec_and_test()?  That would
> give acq+rel semantics, which seems to be what people will expect.  And
> it wouldn't be nearly as expensive as a full smp_mb().

I would expect a ref count api to provide acquire on the last reference release.
Also, even if the code is correct when first submitted and the
developer has done the proper analysis, no acquire will be quite
fragile during subsequent changes: adding a load to a function that is
meant to "own" the object (e.g. a destructor) can subtly break things.
This does not affect x86, so mostly likely won't be noticed right
away, and even when noticed it can cost man-months to debug episodic
misbehaviors.
From the performance perspective the last release happens less often
than non-last releases, which already contain a barrier, and release
of the last reference usually contains more work (multiple kfree's,
scheduler calls, etc), so it should not be a deal breaker. From safe
API design perspective we can do it the other way around: provide a
safe default and an opt-in version without barrier and longer name for
cases where people do know what they are doing and each bit of
performance really matters.

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

On Mon, Jan 21, 2019 at 3:07 PM Reshetova, Elena
<elena.reshetova@intel.com> wrote:
>
>  > Just to check, has this been tested with CONFIG_REFCOUNT_FULL and
> > > something poking kcov?
> > >
> > > Given lib/refcount.c is instrumented, the refcount_*() calls will
> > > recurse back into the kcov code. It looks like that's fine, given these
> > > are only manipulated in setup/teardown paths, but it would be nice to be
> > > sure.
> >
> > A simple program using KCOV is available here:
> > https://elixir.bootlin.com/linux/v5.0-rc3/source/Documentation/dev-
> > tools/kcov.rst#L42
> > or here (it's like strace but collects and prints KCOV coverage):
> > https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c
> >
>
> No, this one hasn't gone via any particular testing apart
> the tests that zero day runs automatically (like boot tests, etc.) since normally
> it is hard for me to know how exactly to test a particular component in a meaningful
> way.
>
> But I can try to test with the above example now, if it helps!

We really need automated tests but I wan't able to figure out how to
write/run kernel tests. Sorry about that. Need to try again. I've
filed https://bugzilla.kernel.org/show_bug.cgi?id=202363 for this.

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Peter Zijlstra]

On Mon, Jan 21, 2019 at 11:05:03AM -0500, Alan Stern wrote:
> On Mon, 21 Jan 2019, Peter Zijlstra wrote:

> > Any additional ordering; like the one you have above; are not strictly
> > required for the proper functioning of the refcount. Rather, you rely on
> > additional ordering and will need to provide this explicitly:
> > 
> > 
> > 	if (refcount_dec_and_text(&x->rc)) {
> > 		/*
> > 		 * Comment that explains what we order against....
> > 		 */
> > 		smp_mb__after_atomic();
> > 		BUG_ON(!x->done*);
> > 		free(x);
> > 	}
> > 
> > 
> > Also; these patches explicitly mention that refcount_t is weaker,
> > specifically to make people aware of this difference.
> > 
> > A full smp_mb() (or two) would also be much more expensive on a number
> > of platforms and in the vast majority of the cases it is not required.
> 
> How about adding smp_rmb() into refcount_dec_and_test()?  That would
> give acq+rel semantics, which seems to be what people will expect.  And
> it wouldn't be nearly as expensive as a full smp_mb().

Yes, that's a very good suggestion.

I suppose we can add smp_acquire__after_ctrl_dep() on the true branch.
Then it reall does become rel_acq.

A wee something like so (I couldn't find an arm64 refcount, even though
I have distinct memories of talk about it).

This isn't compiled, and obviously needs comment/documentation updates
to go along with it.

---
 arch/x86/include/asm/refcount.h | 9 ++++++++-
 lib/refcount.c                  | 7 ++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
index dbaed55c1c24..6f7a1eb345b4 100644
--- a/arch/x86/include/asm/refcount.h
+++ b/arch/x86/include/asm/refcount.h
@@ -74,9 +74,16 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
 
 static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
 {
-	return GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
+	bool ret = GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
 					REFCOUNT_CHECK_LT_ZERO,
 					r->refs.counter, e, "cx");
+
+	if (ret) {
+		smp_acquire__after_ctrl_dep();
+		return true;
+	}
+
+	return false;
 }
 
 static __always_inline __must_check
diff --git a/lib/refcount.c b/lib/refcount.c
index ebcf8cd49e05..8276ad349d48 100644
--- a/lib/refcount.c
+++ b/lib/refcount.c
@@ -190,7 +190,12 @@ bool refcount_sub_and_test_checked(unsigned int i, refcount_t *r)
 
 	} while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
 
-	return !new;
+	if (!new) {
+		smp_acquire__after_ctrl_dep();
+		return true;
+	}
+
+	return false;
 }
 EXPORT_SYMBOL(refcount_sub_and_test_checked);
 

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Kees Cook]

On Tue, Jan 22, 2019 at 10:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Jan 21, 2019 at 11:05:03AM -0500, Alan Stern wrote:
> > On Mon, 21 Jan 2019, Peter Zijlstra wrote:
>
> > > Any additional ordering; like the one you have above; are not strictly
> > > required for the proper functioning of the refcount. Rather, you rely on
> > > additional ordering and will need to provide this explicitly:
> > >
> > >
> > >     if (refcount_dec_and_text(&x->rc)) {
> > >             /*
> > >              * Comment that explains what we order against....
> > >              */
> > >             smp_mb__after_atomic();
> > >             BUG_ON(!x->done*);
> > >             free(x);
> > >     }
> > >
> > >
> > > Also; these patches explicitly mention that refcount_t is weaker,
> > > specifically to make people aware of this difference.
> > >
> > > A full smp_mb() (or two) would also be much more expensive on a number
> > > of platforms and in the vast majority of the cases it is not required.
> >
> > How about adding smp_rmb() into refcount_dec_and_test()?  That would
> > give acq+rel semantics, which seems to be what people will expect.  And
> > it wouldn't be nearly as expensive as a full smp_mb().
>
> Yes, that's a very good suggestion.
>
> I suppose we can add smp_acquire__after_ctrl_dep() on the true branch.
> Then it reall does become rel_acq.
>
> A wee something like so (I couldn't find an arm64 refcount, even though
> I have distinct memories of talk about it).

In the end, arm and arm64 chose to use REFCOUNT_FULL unconditionally,
so there's no arch-specific implementation.

> This isn't compiled, and obviously needs comment/documentation updates
> to go along with it.

Elena, can you do the doc updates?

>
> ---
>  arch/x86/include/asm/refcount.h | 9 ++++++++-
>  lib/refcount.c                  | 7 ++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
> index dbaed55c1c24..6f7a1eb345b4 100644
> --- a/arch/x86/include/asm/refcount.h
> +++ b/arch/x86/include/asm/refcount.h
> @@ -74,9 +74,16 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
>
>  static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
>  {
> -       return GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> +       bool ret = GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
>                                         REFCOUNT_CHECK_LT_ZERO,
>                                         r->refs.counter, e, "cx");
> +
> +       if (ret) {
> +               smp_acquire__after_ctrl_dep();
> +               return true;
> +       }
> +
> +       return false;
>  }
>
>  static __always_inline __must_check
> diff --git a/lib/refcount.c b/lib/refcount.c
> index ebcf8cd49e05..8276ad349d48 100644
> --- a/lib/refcount.c
> +++ b/lib/refcount.c
> @@ -190,7 +190,12 @@ bool refcount_sub_and_test_checked(unsigned int i, refcount_t *r)
>
>         } while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
>
> -       return !new;
> +       if (!new) {
> +               smp_acquire__after_ctrl_dep();
> +               return true;
> +       }
> +
> +       return false;
>  }
>  EXPORT_SYMBOL(refcount_sub_and_test_checked);
>

Thanks for this!

-- 
Kees Cook

RE: [PATCH] kcov: convert kcov.refcount to refcount_t

[Reshetova, Elena]

> > I suppose we can add smp_acquire__after_ctrl_dep() on the true branch.
> > Then it reall does become rel_acq.
> >
> > A wee something like so (I couldn't find an arm64 refcount, even though
> > I have distinct memories of talk about it).
> 
> In the end, arm and arm64 chose to use REFCOUNT_FULL unconditionally,
> so there's no arch-specific implementation.
> 
> > This isn't compiled, and obviously needs comment/documentation updates
> > to go along with it.
> 
> Elena, can you do the doc updates?

Well, doc updates should go with below patch + and additional testing...

Peter, should I create a full version from below with doc/comments
updates, run the whole thing via zero day and send to you if it all looks ok?

Best Regards,
Elena.

> >
> > ---
> >  arch/x86/include/asm/refcount.h | 9 ++++++++-
> >  lib/refcount.c                  | 7 ++++++-
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
> > index dbaed55c1c24..6f7a1eb345b4 100644
> > --- a/arch/x86/include/asm/refcount.h
> > +++ b/arch/x86/include/asm/refcount.h
> > @@ -74,9 +74,16 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
> >
> >  static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
> >  {
> > -       return GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> > +       bool ret = GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> >                                         REFCOUNT_CHECK_LT_ZERO,
> >                                         r->refs.counter, e, "cx");
> > +
> > +       if (ret) {
> > +               smp_acquire__after_ctrl_dep();
> > +               return true;
> > +       }
> > +
> > +       return false;
> >  }
> >
> >  static __always_inline __must_check
> > diff --git a/lib/refcount.c b/lib/refcount.c
> > index ebcf8cd49e05..8276ad349d48 100644
> > --- a/lib/refcount.c
> > +++ b/lib/refcount.c
> > @@ -190,7 +190,12 @@ bool refcount_sub_and_test_checked(unsigned int i,
> refcount_t *r)
> >
> >         } while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
> >
> > -       return !new;
> > +       if (!new) {
> > +               smp_acquire__after_ctrl_dep();
> > +               return true;
> > +       }
> > +
> > +       return false;
> >  }
> >  EXPORT_SYMBOL(refcount_sub_and_test_checked);
> >
> 
> Thanks for this!
> 
> --
> Kees Cook

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Peter Zijlstra]

On Fri, Jan 25, 2019 at 09:02:42AM +0000, Reshetova, Elena wrote:
> > > I suppose we can add smp_acquire__after_ctrl_dep() on the true branch.
> > > Then it reall does become rel_acq.
> > >
> > > A wee something like so (I couldn't find an arm64 refcount, even though
> > > I have distinct memories of talk about it).
> > 
> > In the end, arm and arm64 chose to use REFCOUNT_FULL unconditionally,
> > so there's no arch-specific implementation.
> > 
> > > This isn't compiled, and obviously needs comment/documentation updates
> > > to go along with it.
> > 
> > Elena, can you do the doc updates?
> 
> Well, doc updates should go with below patch + and additional testing...
> 
> Peter, should I create a full version from below with doc/comments
> updates, run the whole thing via zero day and send to you if it all looks ok?

If you have to time to do so, yes please!

RE: [PATCH] kcov: convert kcov.refcount to refcount_t

[Reshetova, Elena]

> On Mon, Jan 21, 2019 at 11:05:03AM -0500, Alan Stern wrote:
> > On Mon, 21 Jan 2019, Peter Zijlstra wrote:
> 
> > > Any additional ordering; like the one you have above; are not strictly
> > > required for the proper functioning of the refcount. Rather, you rely on
> > > additional ordering and will need to provide this explicitly:
> > >
> > >
> > > 	if (refcount_dec_and_text(&x->rc)) {
> > > 		/*
> > > 		 * Comment that explains what we order against....
> > > 		 */
> > > 		smp_mb__after_atomic();
> > > 		BUG_ON(!x->done*);
> > > 		free(x);
> > > 	}
> > >
> > >
> > > Also; these patches explicitly mention that refcount_t is weaker,
> > > specifically to make people aware of this difference.
> > >
> > > A full smp_mb() (or two) would also be much more expensive on a number
> > > of platforms and in the vast majority of the cases it is not required.
> >
> > How about adding smp_rmb() into refcount_dec_and_test()?  That would
> > give acq+rel semantics, which seems to be what people will expect.  And
> > it wouldn't be nearly as expensive as a full smp_mb().
> 
> Yes, that's a very good suggestion.
> 
> I suppose we can add smp_acquire__after_ctrl_dep() on the true branch.
> Then it reall does become rel_acq.
> 
> A wee something like so (I couldn't find an arm64 refcount, even though
> I have distinct memories of talk about it).
> 
> This isn't compiled, and obviously needs comment/documentation updates
> to go along with it.
> 
> ---
>  arch/x86/include/asm/refcount.h | 9 ++++++++-
>  lib/refcount.c                  | 7 ++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
> index dbaed55c1c24..6f7a1eb345b4 100644
> --- a/arch/x86/include/asm/refcount.h
> +++ b/arch/x86/include/asm/refcount.h
> @@ -74,9 +74,16 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
> 
>  static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
>  {
> -	return GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> +	bool ret = GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> 
> 	REFCOUNT_CHECK_LT_ZERO,
>  					r-
> >refs.counter, e, "cx");
> +
> +	if (ret) {
> +		smp_acquire__after_ctrl_dep();
> +		return true;
> +	}
> +
> +	return false;
>  }

Actually as I started to do this, any reason why the change here only for dec_and_test and not
for sub_and _test also? Should not the arch. specific logic follow the generic?

Best Regards,
Elena. 

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

On Sun, Jan 27, 2019 at 7:41 PM Reshetova, Elena
<elena.reshetova@intel.com> wrote:
>
> > On Mon, Jan 21, 2019 at 11:05:03AM -0500, Alan Stern wrote:
> > > On Mon, 21 Jan 2019, Peter Zijlstra wrote:
> >
> > > > Any additional ordering; like the one you have above; are not strictly
> > > > required for the proper functioning of the refcount. Rather, you rely on
> > > > additional ordering and will need to provide this explicitly:
> > > >
> > > >
> > > >   if (refcount_dec_and_text(&x->rc)) {
> > > >           /*
> > > >            * Comment that explains what we order against....
> > > >            */
> > > >           smp_mb__after_atomic();
> > > >           BUG_ON(!x->done*);
> > > >           free(x);
> > > >   }
> > > >
> > > >
> > > > Also; these patches explicitly mention that refcount_t is weaker,
> > > > specifically to make people aware of this difference.
> > > >
> > > > A full smp_mb() (or two) would also be much more expensive on a number
> > > > of platforms and in the vast majority of the cases it is not required.
> > >
> > > How about adding smp_rmb() into refcount_dec_and_test()?  That would
> > > give acq+rel semantics, which seems to be what people will expect.  And
> > >it wouldn't be nearly as expensive as a full smp_mb().
> >
> > Yes, that's a very good suggestion.
> >
> > I suppose we can add smp_acquire__after_ctrl_dep() on the true branch.
> > Then it reall does become rel_acq.
> >
> > A wee something like so (I couldn't find an arm64 refcount, even though
> > I have distinct memories of talk about it).
> >
> > This isn't compiled, and obviously needs comment/documentation updates
> > to go along with it.
> >
> > ---
> >  arch/x86/include/asm/refcount.h | 9 ++++++++-
> >  lib/refcount.c                  | 7 ++++++-
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
> > index dbaed55c1c24..6f7a1eb345b4 100644
> > --- a/arch/x86/include/asm/refcount.h
> > +++ b/arch/x86/include/asm/refcount.h
> > @@ -74,9 +74,16 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
> >
> >  static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
> >  {
> > -     return GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> > +     bool ret = GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> >
> >       REFCOUNT_CHECK_LT_ZERO,
> >                                       r-
> > >refs.counter, e, "cx");
> > +
> > +     if (ret) {
> > +             smp_acquire__after_ctrl_dep();
> > +             return true;
> > +     }
> > +
> > +     return false;
> >  }
>
> Actually as I started to do this, any reason why the change here only for dec_and_test and not
> for sub_and _test also? Should not the arch. specific logic follow the generic?

I would say these should be exactly the same wrt semantics.
dec_and_test is just syntactic sugar for 1 decrement. If we change
dec_and_test, we should change sub_and_test the same way.

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Peter Zijlstra]

On Sun, Jan 27, 2019 at 06:41:38PM +0000, Reshetova, Elena wrote:
> > On Mon, Jan 21, 2019 at 11:05:03AM -0500, Alan Stern wrote:
> > > On Mon, 21 Jan 2019, Peter Zijlstra wrote:

> > Yes, that's a very good suggestion.
> > 
> > I suppose we can add smp_acquire__after_ctrl_dep() on the true branch.
> > Then it reall does become rel_acq.
> > 
> > A wee something like so (I couldn't find an arm64 refcount, even though
> > I have distinct memories of talk about it).
> > 
> > This isn't compiled, and obviously needs comment/documentation updates
> > to go along with it.
> > 
> > ---
> >  arch/x86/include/asm/refcount.h | 9 ++++++++-
> >  lib/refcount.c                  | 7 ++++++-
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/refcount.h b/arch/x86/include/asm/refcount.h
> > index dbaed55c1c24..6f7a1eb345b4 100644
> > --- a/arch/x86/include/asm/refcount.h
> > +++ b/arch/x86/include/asm/refcount.h
> > @@ -74,9 +74,16 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
> > 
> >  static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r)
> >  {
> > -	return GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> > +	bool ret = GEN_UNARY_SUFFIXED_RMWcc(LOCK_PREFIX "decl",
> > 
> > 	REFCOUNT_CHECK_LT_ZERO,
> >  					r-
> > >refs.counter, e, "cx");
> > +
> > +	if (ret) {
> > +		smp_acquire__after_ctrl_dep();
> > +		return true;
> > +	}
> > +
> > +	return false;
> >  }
> 
> Actually as I started to do this, any reason why the change here only
> for dec_and_test and not for sub_and _test also? Should not the arch.
> specific logic follow the generic?

Yes, sub_and_test also needs it; I simply overlooked it.

RE: [PATCH] kcov: convert kcov.refcount to refcount_t

[Reshetova, Elena]

 > Just to check, has this been tested with CONFIG_REFCOUNT_FULL and
> > something poking kcov?
> >
> > Given lib/refcount.c is instrumented, the refcount_*() calls will
> > recurse back into the kcov code. It looks like that's fine, given these
> > are only manipulated in setup/teardown paths, but it would be nice to be
> > sure.
> 
> A simple program using KCOV is available here:
> https://elixir.bootlin.com/linux/v5.0-rc3/source/Documentation/dev-
> tools/kcov.rst#L42
> or here (it's like strace but collects and prints KCOV coverage):
> https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c
> 

Ok, so I finally got to compile kcov in and try the first test program 
and it works fine as far as I can see: runs, prints results, and no WARNs anywhere
visible with regards to refcount_t. 

I did my test on 4.20 with CONFIG_REFCOUNT_FULL=y 
since I have serious issues getting 5.0 running as it is even from
the stable branch, but unless kcov underwent some serious changes since December,
it should not affect. 

Is it ok now for merging this change? The stricter mem ordering on dec_and_test
hopefully will also lands soon. 

Best Regards,
Elena.

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

On Thu, Jan 31, 2019 at 11:04 AM Reshetova, Elena
<elena.reshetova@intel.com> wrote:
>
>  > Just to check, has this been tested with CONFIG_REFCOUNT_FULL and
> > > something poking kcov?
> > >
> > > Given lib/refcount.c is instrumented, the refcount_*() calls will
> > > recurse back into the kcov code. It looks like that's fine, given these
> > > are only manipulated in setup/teardown paths, but it would be nice to be
> > > sure.
> >
> > A simple program using KCOV is available here:
> > https://elixir.bootlin.com/linux/v5.0-rc3/source/Documentation/dev-
> > tools/kcov.rst#L42
> > or here (it's like strace but collects and prints KCOV coverage):
> > https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c
> >
>
> Ok, so I finally got to compile kcov in and try the first test program
> and it works fine as far as I can see: runs, prints results, and no WARNs anywhere
> visible with regards to refcount_t.
>
> I did my test on 4.20 with CONFIG_REFCOUNT_FULL=y
> since I have serious issues getting 5.0 running as it is even from
> the stable branch, but unless kcov underwent some serious changes since December,
> it should not affect.

There were no changes that should affect this part.

Reviewed-by: Dmitry Vyukov <dvyukov@google.com>


> Is it ok now for merging this change? The stricter mem ordering on dec_and_test
> hopefully will also lands soon.
>
> Best Regards,
> Elena.
>

RE: [PATCH] kcov: convert kcov.refcount to refcount_t

[Reshetova, Elena]

> On Thu, Jan 31, 2019 at 11:04 AM Reshetova, Elena
> <elena.reshetova@intel.com> wrote:
> >
> >  > Just to check, has this been tested with CONFIG_REFCOUNT_FULL and
> > > > something poking kcov?
> > > >
> > > > Given lib/refcount.c is instrumented, the refcount_*() calls will
> > > > recurse back into the kcov code. It looks like that's fine, given these
> > > > are only manipulated in setup/teardown paths, but it would be nice to be
> > > > sure.
> > >
> > > A simple program using KCOV is available here:
> > > https://elixir.bootlin.com/linux/v5.0-rc3/source/Documentation/dev-
> > > tools/kcov.rst#L42
> > > or here (it's like strace but collects and prints KCOV coverage):
> > > https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c
> > >
> >
> > Ok, so I finally got to compile kcov in and try the first test program
> > and it works fine as far as I can see: runs, prints results, and no WARNs anywhere
> > visible with regards to refcount_t.
> >
> > I did my test on 4.20 with CONFIG_REFCOUNT_FULL=y
> > since I have serious issues getting 5.0 running as it is even from
> > the stable branch, but unless kcov underwent some serious changes since
> December,
> > it should not affect.
> 
> There were no changes that should affect this part.
> 
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>


Thank you! Will you be able to take this change forward as for 
other normal kcov changes? 

Best Regards,
Elena.

Re: [PATCH] kcov: convert kcov.refcount to refcount_t

[Dmitry Vyukov]

On Thu, Jan 31, 2019 at 11:09 AM Reshetova, Elena
<elena.reshetova@intel.com> wrote:
>
> > On Thu, Jan 31, 2019 at 11:04 AM Reshetova, Elena
> > <elena.reshetova@intel.com> wrote:
> > >
> > >  > Just to check, has this been tested with CONFIG_REFCOUNT_FULL and
> > > > > something poking kcov?
> > > > >
> > > > > Given lib/refcount.c is instrumented, the refcount_*() calls will
> > > > > recurse back into the kcov code. It looks like that's fine, given these
> > > > > are only manipulated in setup/teardown paths, but it would be nice to be
> > > > > sure.
> > > >
> > > > A simple program using KCOV is available here:
> > > > https://elixir.bootlin.com/linux/v5.0-rc3/source/Documentation/dev-
> > > > tools/kcov.rst#L42
> > > > or here (it's like strace but collects and prints KCOV coverage):
> > > > https://github.com/google/syzkaller/blob/master/tools/kcovtrace/kcovtrace.c
> > > >
> > >
> > > Ok, so I finally got to compile kcov in and try the first test program
> > > and it works fine as far as I can see: runs, prints results, and no WARNs anywhere
> > > visible with regards to refcount_t.
> > >
> > > I did my test on 4.20 with CONFIG_REFCOUNT_FULL=y
> > > since I have serious issues getting 5.0 running as it is even from
> > > the stable branch, but unless kcov underwent some serious changes since
> > December,
> > > it should not affect.
> >
> > There were no changes that should affect this part.
> >
> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>
>
> Thank you! Will you be able to take this change forward as for
> other normal kcov changes?

Andrew, please take this patch to mm tree.

+linux-mm mailing list for proper mm patch tracking
I am not a maintainer, all other KCOV patches went through mm tree.