Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191009114809.8643-1-christian.brauner@ubuntu.com>
 <20191021113327.22365-1-christian.brauner@ubuntu.com> <20191023121603.GA16344@andrea.guest.corp.microsoft.com>
 <CACT4Y+Y86HFnQGHyxv+f32tKDJXnRxmL7jQ3tGxVcksvtK3L7Q@mail.gmail.com>
 <20191024113155.GA7406@andrea.guest.corp.microsoft.com> <CACT4Y+Z2-mm6Qk0cecJdiA5B_VsQ1v8k2z+RWrDQv6dTNFXFog@mail.gmail.com>
 <20191024130502.GA11335@andrea.guest.corp.microsoft.com> <CACT4Y+ahUr11pQQ7=dw80Abj5owUPnPdufbMYvsKLM6iDg5QQg@mail.gmail.com>
 <20191024132136.jknzt7rgjssgv5b6@wittgenstein>
In-Reply-To: <20191024132136.jknzt7rgjssgv5b6@wittgenstein>
From:   Dmitry Vyukov <dvyukov@google.com>
Date:   Thu, 24 Oct 2019 15:34:19 +0200
Message-ID: <CACT4Y+bgs3P9v_MqG4BpDM2ZC06kdUJbqKobH=rAXxwv6cBeRA@mail.gmail.com>
Subject: Re: [PATCH v6] taskstats: fix data-race
To:     Christian Brauner <christian.brauner@ubuntu.com>
Cc:     Andrea Parri <parri.andrea@gmail.com>,
        Will Deacon <will@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, bsingharora@gmail.com,
        Marco Elver <elver@google.com>,
        stable <stable@vger.kernel.org>,
        syzbot <syzbot+c5d03165a1bd1dead0c1@syzkaller.appspotmail.com>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Oct 24, 2019 at 3:21 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Thu, Oct 24, 2019 at 03:13:48PM +0200, Dmitry Vyukov wrote:
> > On Thu, Oct 24, 2019 at 3:05 PM Andrea Parri <parri.andrea@gmail.com> wrote:
> > >
> > > 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@gmail.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.  ;-)
> >
> > But why? I think kernel contains lots of such cases and it seems to be
> > officially documented by the LKMM:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/explanation.txt
> > address dependencies and ppo
>
> You mean this section:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/explanation.txt#n955
> and specifically:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/explanation.txt#n982
> ?

Yes, and also this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/explanation.txt#n450