Re: xdp/xsk.c: missing read memory barrier in xsk_poll()

On Fri, Nov 24, 2023 at 02:50:04PM +0100, Magnus Karlsson wrote:

On Fri, 24 Nov 2023 at 08:00, Yewon Choi [off-list ref] wrote:

quoted

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 [off-list ref]
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;

I didn't grab that semantic fully, thank you for pointing it out.
As you suggested, it seems that the part right below skip_tx also should
be skipped.
Additionally, I think read ordering will be guaranteed by smp_rmb()
in xsk_check_common().

I'll write a patch after making sure it, just in case of my mistake.

Thank you for your reply.

Thank you for spotting this!

/Magnus

Best Regards,
Yewon Choi