| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 21 Jan 2019 13:29:11 +0100
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Andrea Parri <andrea.parri@...rulasolutions.com>
Cc: Elena Reshetova <elena.reshetova@...el.com>,
Kees Cook <keescook@...omium.org>,
Peter Zijlstra <peterz@...radead.org>,
Alan Stern <stern@...land.harvard.edu>,
"Paul E. McKenney" <paulmck@...ux.ibm.com>,
Will Deacon <will.deacon@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
Andrey Ryabinin <aryabinin@...tuozzo.com>,
Anders Roxell <anders.roxell@...aro.org>,
Mark Rutland <mark.rutland@....com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kcov: convert kcov.refcount to refcount_t
On Mon, Jan 21, 2019 at 12:45 PM Andrea Parri
<andrea.parri@...rulasolutions.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@...omium.org>
> > > > Reviewed-by: David Windsor <dwindsor@...il.com>
> > > > Reviewed-by: Hans Liljestrand <ishkamiel@...il.com>
> > > > Signed-off-by: Elena Reshetova <elena.reshetova@...el.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
> > > >
Powered by blists - more mailing lists