[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20191024130502.GA11335@andrea.guest.corp.microsoft.com>
Date: Thu, 24 Oct 2019 15:05:02 +0200
From: Andrea Parri <parri.andrea@...il.com>
To: Dmitry Vyukov <dvyukov@...gle.com>
Cc: Christian Brauner <christian.brauner@...ntu.com>,
Will Deacon <will@...nel.org>,
LKML <linux-kernel@...r.kernel.org>, bsingharora@...il.com,
Marco Elver <elver@...gle.com>,
stable <stable@...r.kernel.org>,
syzbot <syzbot+c5d03165a1bd1dead0c1@...kaller.appspotmail.com>,
syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: [PATCH v6] taskstats: fix data-race
On Thu, Oct 24, 2019 at 01:51:20PM +0200, Dmitry Vyukov wrote:
> On Thu, Oct 24, 2019 at 1:32 PM Andrea Parri <parri.andrea@...il.com> wrote:
> >
> > > How these later loads can be completely independent of the pointer
> > > value? They need to obtain the pointer value from somewhere. And this
> > > can only be done by loaded it. And if a thread loads a pointer and
> > > then dereferences that pointer, that's a data/address dependency and
> > > we assume this is now covered by READ_ONCE.
> >
> > The "dependency" I was considering here is a dependency _between the
> > load of sig->stats in taskstats_tgid_alloc() and the (program-order)
> > later loads of *(sig->stats) in taskstats_exit(). Roughly speaking,
> > such a dependency should correspond to a dependency chain at the asm
> > or registers level from the first load to the later loads; e.g., in:
> >
> > Thread [register r0 contains the address of sig->stats]
> >
> > A: LOAD r1,[r0] // LOAD_ACQUIRE sig->stats
> > ...
> > B: LOAD r2,[r0] // LOAD *(sig->stats)
> > C: LOAD r3,[r2]
> >
> > there would be no such dependency from A to C. Compare, e.g., with:
> >
> > Thread [register r0 contains the address of sig->stats]
> >
> > A: LOAD r1,[r0] // LOAD_ACQUIRE sig->stats
> > ...
> > C: LOAD r3,[r1] // LOAD *(sig->stats)
> >
> > AFAICT, there's no guarantee that the compilers will generate such a
> > dependency from the code under discussion.
>
> Fixing this by making A ACQUIRE looks like somewhat weird code pattern
> to me (though correct). B is what loads the address used to read
> indirect data, so B ought to be ACQUIRE (or LOAD-DEPENDS which we get
> from READ_ONCE).
>
> What you are suggesting is:
>
> addr = ptr.load(memory_order_acquire);
> if (addr) {
> addr = ptr.load(memory_order_relaxed);
> data = *addr;
> }
>
> whereas the canonical/non-convoluted form of this pattern is:
>
> addr = ptr.load(memory_order_consume);
> if (addr)
> data = *addr;
No, I'd rather be suggesting:
addr = ptr.load(memory_order_acquire);
if (addr)
data = *addr;
since I'd not expect any form of encouragement to rely on "consume" or
on "READ_ONCE() + true-address-dependency" from myself. ;-)
IAC, v6 looks more like:
addr = ptr.load(memory_order_consume);
if (!!addr)
*ptr = 1;
data = *ptr;
to me (hence my comments/questions ...).
Thanks,
Andrea
Powered by blists - more mailing lists