netdev - Re: [PATCH] veth: Fix race with AF_XDP exposing old or uninitialized descriptors

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]

Message-ID: <e6b0414dbc7e97857fee5936ed04efca81b1d472.camel@redhat.com>
Date:   Thu, 22 Dec 2022 11:18:03 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Shawn Bohrer <sbohrer@...udflare.com>, magnus.karlsson@...il.com
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, bjorn@...nel.org,
        kernel-team@...udflare.com, davem@...emloft.net
Subject: Re: [PATCH] veth: Fix race with AF_XDP exposing old or
 uninitialized descriptors

On Tue, 2022-12-20 at 12:59 -0600, Shawn Bohrer wrote:
> When AF_XDP is used on on a veth interface the RX ring is updated in two
> steps.  veth_xdp_rcv() removes packet descriptors from the FILL ring
> fills them and places them in the RX ring updating the cached_prod
> pointer.  Later xdp_do_flush() syncs the RX ring prod pointer with the
> cached_prod pointer allowing user-space to see the recently filled in
> descriptors.  The rings are intended to be SPSC, however the existing
> order in veth_poll allows the xdp_do_flush() to run concurrently with
> another CPU creating a race condition that allows user-space to see old
> or uninitialized descriptors in the RX ring.  This bug has been observed
> in production systems.
> 
> To summarize, we are expecting this ordering:
> 
> CPU 0 __xsk_rcv_zc()
> CPU 0 __xsk_map_flush()
> CPU 2 __xsk_rcv_zc()
> CPU 2 __xsk_map_flush()
> 
> But we are seeing this order:
> 
> CPU 0 __xsk_rcv_zc()
> CPU 2 __xsk_rcv_zc()
> CPU 0 __xsk_map_flush()
> CPU 2 __xsk_map_flush()
> 
> This occurs because we rely on NAPI to ensure that only one napi_poll
> handler is running at a time for the given veth receive queue.
> napi_schedule_prep() will prevent multiple instances from getting
> scheduled. However calling napi_complete_done() signals that this
> napi_poll is complete and allows subsequent calls to
> napi_schedule_prep() and __napi_schedule() to succeed in scheduling a
> concurrent napi_poll before the xdp_do_flush() has been called.  For the
> veth driver a concurrent call to napi_schedule_prep() and
> __napi_schedule() can occur on a different CPU because the veth xmit
> path can additionally schedule a napi_poll creating the race.

The above looks like a generic problem that other drivers could hit.
Perhaps it could be worthy updating the xdp_do_flush() doc text to
explicitly mention it must be called before napi_complete_done().

(in a separate, net-next patch)

Thanks!

Paolo

> 
> The fix as suggested by Magnus Karlsson, is to simply move the
> xdp_do_flush() call before napi_complete_done().  This syncs the
> producer ring pointers before another instance of napi_poll can be
> scheduled on another CPU.  It will also slightly improve performance by
> moving the flush closer to when the descriptors were placed in the
> RX ring.
> 
> Fixes: d1396004dd86 ("veth: Add XDP TX and REDIRECT")
> Suggested-by: Magnus Karlsson <magnus.karlsson@...il.com>
> Signed-off-by: Shawn Bohrer <sbohrer@...udflare.com>
> ---
>  drivers/net/veth.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index ac7c0653695f..dfc7d87fad59 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -974,6 +974,9 @@ static int veth_poll(struct napi_struct *napi, int budget)
>  	xdp_set_return_frame_no_direct();
>  	done = veth_xdp_rcv(rq, budget, &bq, &stats);
>  
> +	if (stats.xdp_redirect > 0)
> +		xdp_do_flush();
> +
>  	if (done < budget && napi_complete_done(napi, done)) {
>  		/* Write rx_notify_masked before reading ptr_ring */
>  		smp_store_mb(rq->rx_notify_masked, false);
> @@ -987,8 +990,6 @@ static int veth_poll(struct napi_struct *napi, int budget)
>  
>  	if (stats.xdp_tx > 0)
>  		veth_xdp_flush(rq, &bq);
> -	if (stats.xdp_redirect > 0)
> -		xdp_do_flush();
>  	xdp_clear_return_frame_no_direct();
>  
>  	return done;