netdev - Re: [PATCH v2 bpf-next] bpf: devmap: move drop error path to devmap for XDP

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: <pj41zly2f8wfq6.fsf@u68c7b5b1d2d758.ant.amazon.com>
Date:   Sun, 28 Feb 2021 14:15:29 +0200
From:   Shay Agroskin <shayagr@...zon.com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
CC:     <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
        <davem@...emloft.net>, <kuba@...nel.org>, <ast@...nel.org>,
        <daniel@...earbox.net>, <brouer@...hat.com>, <toke@...hat.com>,
        <freysteinn.alfredsson@....se>, <lorenzo.bianconi@...hat.com>,
        <john.fastabend@...il.com>, <jasowang@...hat.com>,
        <mst@...hat.com>, <thomas.petazzoni@...tlin.com>,
        <mw@...ihalf.com>, <linux@...linux.org.uk>,
        <ilias.apalodimas@...aro.org>, <netanel@...zon.com>,
        <akiyano@...zon.com>, <michael.chan@...adcom.com>,
        <madalin.bucur@....com>, <ioana.ciornei@....com>,
        <jesse.brandeburg@...el.com>, <anthony.l.nguyen@...el.com>,
        <saeedm@...dia.com>, <grygorii.strashko@...com>,
        <ecree.xilinx@...il.com>
Subject: Re: [PATCH v2 bpf-next] bpf: devmap: move drop error path to devmap
 for XDP_REDIRECT


Lorenzo Bianconi <lorenzo@...nel.org> writes:

> ...
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c 
> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 102f2c91fdb8..7ad0557dedbd 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> ...
> 
> @@ -339,8 +337,8 @@ static int ena_xdp_xmit(struct net_device 
> *dev, int n,
>  			struct xdp_frame **frames, u32 flags)
>  {
>  	struct ena_adapter *adapter = netdev_priv(dev);
> -	int qid, i, err, drops = 0;
>  	struct ena_ring *xdp_ring;
> +	int qid, i, nxmit = 0;
>  
>  	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
>  		return -EINVAL;
> @@ -360,12 +358,12 @@ static int ena_xdp_xmit(struct net_device 
> *dev, int n,
>  	spin_lock(&xdp_ring->xdp_tx_lock);
>  
>  	for (i = 0; i < n; i++) {
> -		err = ena_xdp_xmit_frame(xdp_ring, dev, frames[i], 
> 0);
>  		/* The descriptor is freed by ena_xdp_xmit_frame 
>  in case
>  		 * of an error.
>  		 */

Thanks a lot for the patch. It's a good idea. Do you mind removing 
the comment here as well ? ena_xdp_xmit_frame() no longer frees 
the frame in case of an error after this patch.

> -		if (err)
> -			drops++;
> +		if (ena_xdp_xmit_frame(xdp_ring, dev, frames[i], 
> 0))
> +			break;
> +		nxmit++;
>  	}
>  
>  	/* Ring doorbell to make device aware of the packets */
> @@ -378,7 +376,7 @@ static int ena_xdp_xmit(struct net_device 
> *dev, int n,
>  	spin_unlock(&xdp_ring->xdp_tx_lock);
>  
>  	/* Return number of packets sent */
> -	return n - drops;
> +	return nxmit;
>  }
> ...
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 85d9d1b72a33..9f158b3862df 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -344,29 +344,26 @@ static void bq_xmit_all(struct 
> xdp_dev_bulk_queue *bq, u32 flags)
>  
>  	sent = dev->netdev_ops->ndo_xdp_xmit(dev, bq->count, 
>  bq->q, flags);
>  	if (sent < 0) {
> +		/* If ndo_xdp_xmit fails with an errno, no frames 
> have
> +		 * been xmit'ed.
> +		 */
>  		err = sent;
>  		sent = 0;
> -		goto error;
>  	}
> +
>  	drops = bq->count - sent;
> -out:
> -	bq->count = 0;
> +	if (unlikely(drops > 0)) {
> +		/* If not all frames have been transmitted, it is 
> our
> +		 * responsibility to free them
> +		 */
> +		for (i = sent; i < bq->count; i++)
> +			xdp_return_frame_rx_napi(bq->q[i]);
> +	}

Wouldn't the logic above be the same even w/o the 'if' condition ?

>  
> +	bq->count = 0;
>  	trace_xdp_devmap_xmit(bq->dev_rx, dev, sent, drops, err);
>  	bq->dev_rx = NULL;
>  	__list_del_clearprev(&bq->flush_node);
> -	return;
> -error:
> -	/* If ndo_xdp_xmit fails with an errno, no frames have 
> been
> -	 * xmit'ed and it's our responsibility to them free all.
> -	 */
> -	for (i = 0; i < bq->count; i++) {
> -		struct xdp_frame *xdpf = bq->q[i];
> -
> -		xdp_return_frame_rx_napi(xdpf);
> -		drops++;
> -	}
> -	goto out;
>  }
>  
>  /* __dev_flush is called from xdp_do_flush() which _must_ be 
>  signaled

Thanks, Shay