netdev - Re: xdp/xsk.c: missing read memory barrier in xsk

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: Fri, 24 Nov 2023 14:50:04 +0100
From: Magnus Karlsson <magnus.karlsson@...il.com>
To: Yewon Choi <woni9911@...il.com>
Cc: Björn Töpel <bjorn@...nel.org>, 
	Magnus Karlsson <magnus.karlsson@...el.com>, 
	Maciej Fijalkowski <maciej.fijalkowski@...el.com>, Jonathan Lemon <jonathan.lemon@...il.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Jesper Dangaard Brouer <hawk@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>, netdev@...r.kernel.org, bpf@...r.kernel.org, 
	linux-kernel@...r.kernel.org, threeearcat@...il.com
Subject: Re: xdp/xsk.c: missing read memory barrier in xsk_poll()

On Fri, 24 Nov 2023 at 08:00, Yewon Choi <woni9911@...il.com> wrote:
>
> Hello,
>
> We found some possibility of missing read memory barrier in xsk_poll(),
> so we would like to ask to check it.
>
> commit e6762c8b adds two smp_rmb() in xsk_mmap(), which are paired with
> smp_wmb() in XDP_UMEM_REG and xsk_init_queue each. The later one is
> added in order to prevent reordering between reading of q and reading
> of q->ring.
> One example in simplied code is:
>
> xsk_mmap():
>         if (offset == XDP_PGOFF_RX_RING) {
>                 q = READ_ONCE(xs->rx);
>         }
>         ...
>         if (!q)
>                 return -EINVAL;
>
>         /* Matches the smp_wmb() in xsk_init_queue */
>         smp_rmb();
>         ...
>         return remap_vmalloc_range(vma, q->ring, 0);
>
> Also, the similar logic exists in xsk_poll() without smp_rmb().
>
> xsk_poll():
>         ...
>         if (xs->rx && !xskq_prod_is_empty(xs->rx))
>                 mask |= EPOLLIN | EPOLLRDNORM;
>         if (xs->tx && xsk_tx_writeable(xs))
>                 mask |= EPOLLOUT | EPOLLWRNORM;
>
> xskq_prod_is_empty():
>         return READ_ONCE(q->ring->consumer) && ...
>
> To be consistent, I think that smp_rmb() is needed between
> xs->rx and !xsq_prod_is_empty() and the same applies for xs->tx.
>
> Could you check this please?
> If a patch is needed, we will send them.

Yes, you are correct that the current code would need an smp_rmb().
However, an unbound socket should never be allowed to enter the
xsk_poll() code in the first place since it is pointless to poll a
socket that has not been bound. This error was introduced in the
commit below:

commit 1596dae2f17ec5c6e8c8f0e3fec78c5ae55c1e0b
Author: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Date:   Wed Feb 15 15:33:09 2023 +0100

    xsk: check IFF_UP earlier in Tx path

When an AF_XDP socket has been bound, it is guaranteed to have been
set up in the correct way and a memory barrier has already been
executed in the xsk_bind call. It would be great if you could submit a
patch, but I suggest that you do something like this instead of
introducing an smp_rmb():

    if (xsk_check_common(xs))
        goto out;
    :
    :

    if (xs->rx && !xskq_prod_is_empty(xs->rx))
        mask |= EPOLLIN | EPOLLRDNORM;
    if (xs->tx && xsk_tx_writeable(xs))
        mask |= EPOLLOUT | EPOLLWRNORM;

out:
    rcu_read_unlock();
    return mask;

Thank you for spotting this!

/Magnus

>
> Best Regards,
> Yewon Choi
>