netdev - Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST

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]

Date: Tue, 20 Jun 2023 11:24:30 +0800
From: Heng Qi <hengqi@...ux.alibaba.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org,
	Jason Wang <jasowang@...hat.com>,
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and
 _F_GUEST_CSUM

On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote:
> > We are now re-probing the csum related fields and  trying
> > to have XDP and RX hw checksum capabilities coexist on the
> > XDP path. For the benefit of:
> > 1. RX hw checksum capability can be used if XDP is loaded.
> > 2. Avoid packet loss when loading XDP in the vm-vm scenario.
> > 
> > Signed-off-by: Heng Qi <hengqi@...ux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 07b4801d689c..25b486ab74db 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  	struct net_device *dev = vi->dev;
> >  	struct sk_buff *skb;
> >  	struct virtio_net_hdr_mrg_rxbuf *hdr;
> > +	__u8 flags;
> >  
> >  	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> >  		pr_debug("%s: short packet %i\n", dev->name, len);
> > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  		return;
> >  	}
> >  
> > +	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
> > +
> >  	if (vi->mergeable_rx_bufs)
> >  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> >  					stats);
> 
> what's going on here?

Hi, Michael.

Is your question about the function of this code?
1. If yes,
this sentence saves the flags value in virtio-net-hdr in advance
before entering the XDP processing logic, so that it can be used to
judge further logic after XDP processing.

If _NEEDS_CSUM is included in flags before XDP processing, then after
XDP processing we need to re-probe the csum fields and calculate the
pseudo-header checksum.

2. If not,
do you mean that mergeable and small modes should be distinguished for
save actions?
The answer is that we don't need it. The information in virtio-net-hdr in
the current small mode is also forcibly converted into the
virtio_net_hdr_mrg_rxbuf structure, which is consistent with the
virtio_net_hdr structure in terms of code layout, and the results are
consistent.

If you think this is semantically wrong, then we need a bugfix patch.
The simplest example is in receive_small_xdp():
"
unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom;
...
struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset; "


Thanks.

> 
> > @@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  	if (unlikely(!skb))
> >  		return;
> >  
> > -	hdr = skb_vnet_hdr(skb);
> > -	if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> > -		virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> > -
> > -	if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > -		skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +	if (unlikely(vi->xdp_enabled)) {
> > +		if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) {
> > +			pr_debug("%s: errors occurred in flow dissector setting csum",
> > +				 dev->name);
> > +			goto frame_err;
> > +		}
> >  
> > -	if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> > -				  virtio_is_little_endian(vi->vdev))) {
> > -		net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> > -				     dev->name, hdr->hdr.gso_type,
> > -				     hdr->hdr.gso_size);
> > -		goto frame_err;
> > +	} else {
> > +		hdr = skb_vnet_hdr(skb);
> > +		if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> > +			virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> > +
> > +		if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +
> > +		if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> > +					  virtio_is_little_endian(vi->vdev))) {
> > +			net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> > +					     dev->name, hdr->hdr.gso_type,
> > +					     hdr->hdr.gso_size);
> > +			goto frame_err;
> > +		}
> >  	}
> >  
> >  	skb_record_rx_queue(skb, vq2rxq(rq->vq));
> > -- 
> > 2.19.1.6.gb485710b