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: <YDwYzYVIDQABINyy@lore-laptop-rh>
Date:   Sun, 28 Feb 2021 23:27:25 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To:     Shay Agroskin <shayagr@...zon.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, 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,
        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.

ack, will do in v3

> 
> > -		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 ?

it is just an optimization to avoid the for loop instruction if sent = bq->count

Regards,
Lorenzo

> 
> > +	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
> 

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)