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  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Mon, 2 Dec 2019 13:31:37 +0100
From:   Magnus Karlsson <>
To:     Maxim Mikityanskiy <>
Cc:     Magnus Karlsson <>,
        "" <>,
        "" <>,
        "" <>,
        "" <>,
        "" <>,
        "" <>
Subject: Re: [PATCH bpf] xsk: add missing memory barrier in xskq_has_addrs()

On Mon, Dec 2, 2019 at 10:30 AM Maxim Mikityanskiy <> wrote:
> On 2019-11-29 11:51, Magnus Karlsson wrote:
> > The rings in AF_XDP between user space and kernel space have the
> > following semantics:
> >
> > producer                         consumer
> >
> > if (LOAD ->consumer) {           LOAD ->producer
> >                     (A)           smp_rmb()       (C)
> >     STORE $data                   LOAD $data
> >     smp_wmb()       (B)           smp_mb()        (D)
> >     STORE ->producer              STORE ->consumer
> > }
> >
> > The consumer function xskq_has_addrs() below loads the producer
> > pointer and updates the locally cached copy of it. However, it does
> > not issue the smp_rmb() operation required by the lockless ring. This
> > would have been ok had the function not updated the locally cached
> > copy, as that could not have resulted in new data being read from the
> > ring. But as it updates the local producer pointer, a subsequent peek
> > operation, such as xskq_peek_addr(), might load data from the ring
> > without issuing the required smp_rmb() memory barrier.
> Thanks for paying attention to it, but I don't think it can really
> happen. xskq_has_addrs only updates prod_tail, but xskq_peek_addr
> doesn't use prod_tail, it reads from cons_tail to cons_head, and every
> cons_head update has the necessary smp_rmb.

You are correct, it cannot happen. I am working on a 10 part patch set
that simplifies the rings and was staring blindly at that. In that
patch set it can happen since I only have two cached pointers instead
of four so there is a dependency, but not in the current code. I will
include this barrier in my patch set at the appropriate place. Thanks
for looking into this Maxim.

Please drop this patch.


Powered by blists - more mailing lists