Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Sat, 25 Nov 2023 00:16:53 +0900
From:   Yewon Choi <woni9911@gmail.com>
To:     Magnus Karlsson <magnus.karlsson@gmail.com>
Cc:     =?iso-8859-1?Q?Bj=F6rn_T=F6pel?= <bjorn@kernel.org>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Jonathan Lemon <jonathan.lemon@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        netdev@vger.kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, threeearcat@gmail.com
Subject: Re: xdp/xsk.c: missing read memory barrier in xsk_poll()
Message-ID: <20231124151651.GA26062@libra05>
References: <20231124070005.GA10393@libra05>
 <CAJ8uoz1s_TqemsQSsu4=pH147d9M1y-cy5G1VCLkM9g3pFj93w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJ8uoz1s_TqemsQSsu4=pH147d9M1y-cy5G1VCLkM9g3pFj93w@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Fri, Nov 24, 2023 at 02:50:04PM +0100, Magnus Karlsson wrote:
> On Fri, 24 Nov 2023 at 08:00, Yewon Choi <woni9911@gmail.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@intel.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;
> 

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