linux-kernel - Re: [PATCH v6] taskstats: fix data-race

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 Oct 2019 13:31:55 +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

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

> Or these later loads of the pointer can also race with the store? If
> so, I think they also need to use READ_ONCE (rather than turn this earlier
> pointer load into acquire).

AFAICT, _if the LOAD_ACQUIRE reads from the mentioned STORE_RELEASE,
then the former must induce enough synchronization to eliminate data
races (as well as any undesired re-ordering).

TBH, I am not familiar enough with the underlying logic of this code
to say whether that "if .. reads from .." pre-condition holds by the
time those *(sig->stats) execute.

Thanks,
  Andrea