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

[Dmitry Vyukov <dvyukov@google.com>]

 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
> > > >