The RSS hash report is a feature that's part of the virtio specification.
Currently, virtio backends like qemu and vdpa (mlx5) support it(potentially
vhost). While the capability to obtain the RSS hash has been enabled in the
normal path, it's currently missing in the XDP path.
Changes from v1:
- introduce a wrapper structure to preserve virtio header
Liang Chen (3):
virtio_net: Preserve virtio header before XDP program execution
virtio_net: Add missing virtio header in skb for XDP_PASS
virtio_net: Support RX hash XDP hint
drivers/net/virtio_net.c | 102 ++++++++++++++++++++++++++++++++++-----
1 file changed, 90 insertions(+), 12 deletions(-)
--
2.40.1
The xdp program may overwrite the inline virtio header. To ensure the
integrity of the virtio header, it is saved in a data structure that
wraps both the xdp_buff and the header before running the xdp program.
Signed-off-by: Liang Chen <[email protected]>
---
drivers/net/virtio_net.c | 43 +++++++++++++++++++++++++++++-----------
1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d7ce4a1011ea..b56828804e5f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -349,6 +349,11 @@ struct virtio_net_common_hdr {
};
};
+struct virtnet_xdp_buff {
+ struct xdp_buff xdp;
+ struct virtio_net_common_hdr hdr;
+};
+
static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
static bool is_xdp_frame(void *ptr)
@@ -1199,9 +1204,10 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
unsigned int headroom = vi->hdr_len + header_offset;
struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
struct page *page = virt_to_head_page(buf);
+ struct virtnet_xdp_buff virtnet_xdp;
struct page *xdp_page;
+ struct xdp_buff *xdp;
unsigned int buflen;
- struct xdp_buff xdp;
struct sk_buff *skb;
unsigned int metasize = 0;
u32 act;
@@ -1233,17 +1239,23 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
page = xdp_page;
}
- xdp_init_buff(&xdp, buflen, &rq->xdp_rxq);
- xdp_prepare_buff(&xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
+ xdp = &virtnet_xdp.xdp;
+ xdp_init_buff(xdp, buflen, &rq->xdp_rxq);
+ xdp_prepare_buff(xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
xdp_headroom, len, true);
- act = virtnet_xdp_handler(xdp_prog, &xdp, dev, xdp_xmit, stats);
+ /* Copy out the virtio header, as it may be overwritten by the
+ * xdp program.
+ */
+ memcpy(&virtnet_xdp.hdr, hdr, vi->hdr_len);
+
+ act = virtnet_xdp_handler(xdp_prog, xdp, dev, xdp_xmit, stats);
switch (act) {
case XDP_PASS:
/* Recalculate length in case bpf program changed it */
- len = xdp.data_end - xdp.data;
- metasize = xdp.data - xdp.data_meta;
+ len = xdp->data_end - xdp->data;
+ metasize = xdp->data - xdp->data_meta;
break;
case XDP_TX:
@@ -1254,7 +1266,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
goto err_xdp;
}
- skb = virtnet_build_skb(buf, buflen, xdp.data - buf, len);
+ skb = virtnet_build_skb(buf, buflen, xdp->data - buf, len);
if (unlikely(!skb))
goto err;
@@ -1591,10 +1603,11 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
struct page *page = virt_to_head_page(buf);
int offset = buf - page_address(page);
+ struct virtnet_xdp_buff virtnet_xdp;
unsigned int xdp_frags_truesz = 0;
struct sk_buff *head_skb;
unsigned int frame_sz;
- struct xdp_buff xdp;
+ struct xdp_buff *xdp;
void *data;
u32 act;
int err;
@@ -1604,16 +1617,22 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
if (unlikely(!data))
goto err_xdp;
- err = virtnet_build_xdp_buff_mrg(dev, vi, rq, &xdp, data, len, frame_sz,
+ xdp = &virtnet_xdp.xdp;
+ err = virtnet_build_xdp_buff_mrg(dev, vi, rq, xdp, data, len, frame_sz,
&num_buf, &xdp_frags_truesz, stats);
if (unlikely(err))
goto err_xdp;
- act = virtnet_xdp_handler(xdp_prog, &xdp, dev, xdp_xmit, stats);
+ /* Copy out the virtio header, as it may be overwritten by the
+ * xdp program.
+ */
+ memcpy(&virtnet_xdp.hdr, hdr, vi->hdr_len);
+
+ act = virtnet_xdp_handler(xdp_prog, xdp, dev, xdp_xmit, stats);
switch (act) {
case XDP_PASS:
- head_skb = build_skb_from_xdp_buff(dev, vi, &xdp, xdp_frags_truesz);
+ head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
if (unlikely(!head_skb))
break;
return head_skb;
@@ -1626,7 +1645,7 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
break;
}
- put_xdp_frags(&xdp);
+ put_xdp_frags(xdp);
err_xdp:
put_page(page);
--
2.40.1
For the XDP_PASS scenario of the XDP path, the skb constructed with
xdp_buff does not include the virtio header. Adding the virtio header
information back when creating the skb.
Signed-off-by: Liang Chen <[email protected]>
---
drivers/net/virtio_net.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b56828804e5f..2de46eb4c661 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1270,6 +1270,9 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
if (unlikely(!skb))
goto err;
+ /* Store the original virtio header for subsequent use by the driver. */
+ memcpy(skb_vnet_common_hdr(skb), &virtnet_xdp.hdr, vi->hdr_len);
+
if (metasize)
skb_metadata_set(skb, metasize);
@@ -1635,6 +1638,9 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
if (unlikely(!head_skb))
break;
+ /* Store the original virtio header for subsequent use by the driver. */
+ memcpy(skb_vnet_common_hdr(head_skb), &virtnet_xdp.hdr, vi->hdr_len);
+
return head_skb;
case XDP_TX:
--
2.40.1
The RSS hash report is a feature that's part of the virtio specification.
Currently, virtio backends like qemu, vdpa (mlx5), and potentially vhost
(still a work in progress as per [1]) support this feature. While the
capability to obtain the RSS hash has been enabled in the normal path,
it's currently missing in the XDP path. Therefore, we are introducing XDP
hints through kfuncs to allow XDP programs to access the RSS hash.
1.
https://lore.kernel.org/all/[email protected]/#r
Signed-off-by: Liang Chen <[email protected]>
---
drivers/net/virtio_net.c | 53 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2de46eb4c661..ed6a4aa3fe04 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -4604,6 +4604,58 @@ static void virtnet_set_big_packets(struct virtnet_info *vi, const int mtu)
}
}
+static int virtnet_xdp_rx_hash(const struct xdp_md *_ctx, u32 *hash,
+ enum xdp_rss_hash_type *rss_type)
+{
+ const struct virtnet_xdp_buff *virtnet_xdp = (void *)_ctx;
+ struct virtio_net_hdr_v1_hash *hdr_hash;
+
+ if (!(virtnet_xdp->xdp.rxq->dev->features & NETIF_F_RXHASH))
+ return -ENODATA;
+
+ hdr_hash = (struct virtio_net_hdr_v1_hash *)(&virtnet_xdp->hdr);
+
+ switch (__le16_to_cpu(hdr_hash->hash_report)) {
+ case VIRTIO_NET_HASH_REPORT_TCPv4:
+ *rss_type = XDP_RSS_TYPE_L4_IPV4_TCP;
+ break;
+ case VIRTIO_NET_HASH_REPORT_UDPv4:
+ *rss_type = XDP_RSS_TYPE_L4_IPV4_UDP;
+ break;
+ case VIRTIO_NET_HASH_REPORT_TCPv6:
+ *rss_type = XDP_RSS_TYPE_L4_IPV6_TCP;
+ break;
+ case VIRTIO_NET_HASH_REPORT_UDPv6:
+ *rss_type = XDP_RSS_TYPE_L4_IPV6_UDP;
+ break;
+ case VIRTIO_NET_HASH_REPORT_TCPv6_EX:
+ *rss_type = XDP_RSS_TYPE_L4_IPV6_TCP_EX;
+ break;
+ case VIRTIO_NET_HASH_REPORT_UDPv6_EX:
+ *rss_type = XDP_RSS_TYPE_L4_IPV6_UDP_EX;
+ break;
+ case VIRTIO_NET_HASH_REPORT_IPv4:
+ *rss_type = XDP_RSS_TYPE_L3_IPV4;
+ break;
+ case VIRTIO_NET_HASH_REPORT_IPv6:
+ *rss_type = XDP_RSS_TYPE_L3_IPV6;
+ break;
+ case VIRTIO_NET_HASH_REPORT_IPv6_EX:
+ *rss_type = XDP_RSS_TYPE_L3_IPV6_EX;
+ break;
+ case VIRTIO_NET_HASH_REPORT_NONE:
+ default:
+ *rss_type = XDP_RSS_TYPE_NONE;
+ }
+
+ *hash = __le32_to_cpu(hdr_hash->hash_value);
+ return 0;
+}
+
+static const struct xdp_metadata_ops virtnet_xdp_metadata_ops = {
+ .xmo_rx_hash = virtnet_xdp_rx_hash,
+};
+
static int virtnet_probe(struct virtio_device *vdev)
{
int i, err = -ENOMEM;
@@ -4729,6 +4781,7 @@ static int virtnet_probe(struct virtio_device *vdev)
VIRTIO_NET_RSS_HASH_TYPE_UDP_EX);
dev->hw_features |= NETIF_F_RXHASH;
+ dev->xdp_metadata_ops = &virtnet_xdp_metadata_ops;
}
if (vi->has_rss_hash_report)
--
2.40.1
On Wed, 24 Jan 2024 16:57:20 +0800, Liang Chen <[email protected]> wrote:
> For the XDP_PASS scenario of the XDP path, the skb constructed with
> xdp_buff does not include the virtio header. Adding the virtio header
> information back when creating the skb.
>
> Signed-off-by: Liang Chen <[email protected]>
> ---
> drivers/net/virtio_net.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b56828804e5f..2de46eb4c661 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1270,6 +1270,9 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> if (unlikely(!skb))
> goto err;
>
> + /* Store the original virtio header for subsequent use by the driver. */
> + memcpy(skb_vnet_common_hdr(skb), &virtnet_xdp.hdr, vi->hdr_len);
About this, a spec is waiting for voting.
This may change the logic of the csum offset and so on.
Please not do this.
Thanks.
> +
> if (metasize)
> skb_metadata_set(skb, metasize);
>
> @@ -1635,6 +1638,9 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
> if (unlikely(!head_skb))
> break;
> + /* Store the original virtio header for subsequent use by the driver. */
> + memcpy(skb_vnet_common_hdr(head_skb), &virtnet_xdp.hdr, vi->hdr_len);
> +
> return head_skb;
>
> case XDP_TX:
> --
> 2.40.1
>
On Wed, 24 Jan 2024 16:57:19 +0800, Liang Chen <[email protected]> wrote:
> The xdp program may overwrite the inline virtio header. To ensure the
> integrity of the virtio header, it is saved in a data structure that
> wraps both the xdp_buff and the header before running the xdp program.
>
> Signed-off-by: Liang Chen <[email protected]>
> ---
> drivers/net/virtio_net.c | 43 +++++++++++++++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d7ce4a1011ea..b56828804e5f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -349,6 +349,11 @@ struct virtio_net_common_hdr {
> };
> };
>
> +struct virtnet_xdp_buff {
> + struct xdp_buff xdp;
> + struct virtio_net_common_hdr hdr;
Not all items of the hdr are useful, we can just save the useful items.
> +};
> +
> static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
>
> static bool is_xdp_frame(void *ptr)
> @@ -1199,9 +1204,10 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> unsigned int headroom = vi->hdr_len + header_offset;
> struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
> struct page *page = virt_to_head_page(buf);
> + struct virtnet_xdp_buff virtnet_xdp;
> struct page *xdp_page;
> + struct xdp_buff *xdp;
> unsigned int buflen;
> - struct xdp_buff xdp;
> struct sk_buff *skb;
> unsigned int metasize = 0;
> u32 act;
> @@ -1233,17 +1239,23 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> page = xdp_page;
> }
>
> - xdp_init_buff(&xdp, buflen, &rq->xdp_rxq);
> - xdp_prepare_buff(&xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
> + xdp = &virtnet_xdp.xdp;
> + xdp_init_buff(xdp, buflen, &rq->xdp_rxq);
> + xdp_prepare_buff(xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
> xdp_headroom, len, true);
>
> - act = virtnet_xdp_handler(xdp_prog, &xdp, dev, xdp_xmit, stats);
> + /* Copy out the virtio header, as it may be overwritten by the
> + * xdp program.
> + */
> + memcpy(&virtnet_xdp.hdr, hdr, vi->hdr_len);
Can we put this into virtnet_xdp_handler?
And just do that when the hash is negotiated.
> +
> + act = virtnet_xdp_handler(xdp_prog, xdp, dev, xdp_xmit, stats);
>
> switch (act) {
> case XDP_PASS:
> /* Recalculate length in case bpf program changed it */
> - len = xdp.data_end - xdp.data;
> - metasize = xdp.data - xdp.data_meta;
> + len = xdp->data_end - xdp->data;
> + metasize = xdp->data - xdp->data_meta;
> break;
>
> case XDP_TX:
> @@ -1254,7 +1266,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> goto err_xdp;
> }
>
> - skb = virtnet_build_skb(buf, buflen, xdp.data - buf, len);
> + skb = virtnet_build_skb(buf, buflen, xdp->data - buf, len);
> if (unlikely(!skb))
> goto err;
>
> @@ -1591,10 +1603,11 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> struct page *page = virt_to_head_page(buf);
> int offset = buf - page_address(page);
> + struct virtnet_xdp_buff virtnet_xdp;
> unsigned int xdp_frags_truesz = 0;
> struct sk_buff *head_skb;
> unsigned int frame_sz;
> - struct xdp_buff xdp;
> + struct xdp_buff *xdp;
> void *data;
> u32 act;
> int err;
> @@ -1604,16 +1617,22 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> if (unlikely(!data))
> goto err_xdp;
>
> - err = virtnet_build_xdp_buff_mrg(dev, vi, rq, &xdp, data, len, frame_sz,
> + xdp = &virtnet_xdp.xdp;
> + err = virtnet_build_xdp_buff_mrg(dev, vi, rq, xdp, data, len, frame_sz,
> &num_buf, &xdp_frags_truesz, stats);
> if (unlikely(err))
> goto err_xdp;
>
> - act = virtnet_xdp_handler(xdp_prog, &xdp, dev, xdp_xmit, stats);
> + /* Copy out the virtio header, as it may be overwritten by the
> + * xdp program.
> + */
> + memcpy(&virtnet_xdp.hdr, hdr, vi->hdr_len);
> +
> + act = virtnet_xdp_handler(xdp_prog, xdp, dev, xdp_xmit, stats);
>
> switch (act) {
> case XDP_PASS:
> - head_skb = build_skb_from_xdp_buff(dev, vi, &xdp, xdp_frags_truesz);
> + head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
> if (unlikely(!head_skb))
> break;
> return head_skb;
> @@ -1626,7 +1645,7 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> break;
> }
>
> - put_xdp_frags(&xdp);
> + put_xdp_frags(xdp);
>
> err_xdp:
> put_page(page);
> --
> 2.40.1
>
在 2024/1/24 下午4:57, Liang Chen 写道:
> For the XDP_PASS scenario of the XDP path, the skb constructed with
> xdp_buff does not include the virtio header. Adding the virtio header
> information back when creating the skb.
>
> Signed-off-by: Liang Chen <[email protected]>
> ---
> drivers/net/virtio_net.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b56828804e5f..2de46eb4c661 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1270,6 +1270,9 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> if (unlikely(!skb))
> goto err;
>
> + /* Store the original virtio header for subsequent use by the driver. */
> + memcpy(skb_vnet_common_hdr(skb), &virtnet_xdp.hdr, vi->hdr_len);
If xdp push or xdp pull modifies xdp_buff, will the original header
still apply to the modified data?
Thanks,
Heng
> +
> if (metasize)
> skb_metadata_set(skb, metasize);
>
> @@ -1635,6 +1638,9 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
> if (unlikely(!head_skb))
> break;
> + /* Store the original virtio header for subsequent use by the driver. */
> + memcpy(skb_vnet_common_hdr(head_skb), &virtnet_xdp.hdr, vi->hdr_len);
> +
> return head_skb;
>
> case XDP_TX:
On Wed, Jan 24, 2024 at 5:16 PM Xuan Zhuo <[email protected]> wrote:
>
> On Wed, 24 Jan 2024 16:57:20 +0800, Liang Chen <[email protected]> wrote:
> > For the XDP_PASS scenario of the XDP path, the skb constructed with
> > xdp_buff does not include the virtio header. Adding the virtio header
> > information back when creating the skb.
> >
> > Signed-off-by: Liang Chen <[email protected]>
> > ---
> > drivers/net/virtio_net.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index b56828804e5f..2de46eb4c661 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1270,6 +1270,9 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> > if (unlikely(!skb))
> > goto err;
> >
> > + /* Store the original virtio header for subsequent use by the driver. */
> > + memcpy(skb_vnet_common_hdr(skb), &virtnet_xdp.hdr, vi->hdr_len);
>
> About this, a spec is waiting for voting.
>
A pointer?
> This may change the logic of the csum offset and so on.
Btw, doesn't it need a new feature bit?
Thanks
>
> Please not do this.
>
> Thanks.
>
>
> > +
> > if (metasize)
> > skb_metadata_set(skb, metasize);
> >
> > @@ -1635,6 +1638,9 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> > head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
> > if (unlikely(!head_skb))
> > break;
> > + /* Store the original virtio header for subsequent use by the driver. */
> > + memcpy(skb_vnet_common_hdr(head_skb), &virtnet_xdp.hdr, vi->hdr_len);
> > +
> > return head_skb;
> >
> > case XDP_TX:
> > --
> > 2.40.1
> >
>
On Thu, 25 Jan 2024 11:48:18 +0800, Jason Wang <[email protected]> wrote:
> On Wed, Jan 24, 2024 at 5:16 PM Xuan Zhuo <[email protected]> wrote:
> >
> > On Wed, 24 Jan 2024 16:57:20 +0800, Liang Chen <[email protected]> wrote:
> > > For the XDP_PASS scenario of the XDP path, the skb constructed with
> > > xdp_buff does not include the virtio header. Adding the virtio header
> > > information back when creating the skb.
> > >
> > > Signed-off-by: Liang Chen <[email protected]>
> > > ---
> > > drivers/net/virtio_net.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index b56828804e5f..2de46eb4c661 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -1270,6 +1270,9 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> > > if (unlikely(!skb))
> > > goto err;
> > >
> > > + /* Store the original virtio header for subsequent use by the driver. */
> > > + memcpy(skb_vnet_common_hdr(skb), &virtnet_xdp.hdr, vi->hdr_len);
> >
> > About this, a spec is waiting for voting.
> >
>
> A pointer?
Sorry.
http://lore.kernel.org/all/[email protected]
>
> > This may change the logic of the csum offset and so on.
>
> Btw, doesn't it need a new feature bit?
Yes.
But this patch set should not include this.
We may do the similar thing, but the commit log will
include more info about the spec.
Thanks.
>
> Thanks
>
> >
> > Please not do this.
> >
> > Thanks.
> >
> >
> > > +
> > > if (metasize)
> > > skb_metadata_set(skb, metasize);
> > >
> > > @@ -1635,6 +1638,9 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> > > head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
> > > if (unlikely(!head_skb))
> > > break;
> > > + /* Store the original virtio header for subsequent use by the driver. */
> > > + memcpy(skb_vnet_common_hdr(head_skb), &virtnet_xdp.hdr, vi->hdr_len);
> > > +
> > > return head_skb;
> > >
> > > case XDP_TX:
> > > --
> > > 2.40.1
> > >
> >
>
On Wed, Jan 24, 2024 at 7:04 PM Heng Qi <[email protected]> wrote:
>
>
>
> 在 2024/1/24 下午4:57, Liang Chen 写道:
> > For the XDP_PASS scenario of the XDP path, the skb constructed with
> > xdp_buff does not include the virtio header. Adding the virtio header
> > information back when creating the skb.
> >
> > Signed-off-by: Liang Chen <[email protected]>
> > ---
> > drivers/net/virtio_net.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index b56828804e5f..2de46eb4c661 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1270,6 +1270,9 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> > if (unlikely(!skb))
> > goto err;
> >
> > + /* Store the original virtio header for subsequent use by the driver. */
> > + memcpy(skb_vnet_common_hdr(skb), &virtnet_xdp.hdr, vi->hdr_len);
>
> If xdp push or xdp pull modifies xdp_buff, will the original header
> still apply to the modified data?
>
No, it would be an issue then. Anyway, this patch will be dropped in v3. Thanks.
> Thanks,
> Heng
>
> > +
> > if (metasize)
> > skb_metadata_set(skb, metasize);
> >
> > @@ -1635,6 +1638,9 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> > head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
> > if (unlikely(!head_skb))
> > break;
> > + /* Store the original virtio header for subsequent use by the driver. */
> > + memcpy(skb_vnet_common_hdr(head_skb), &virtnet_xdp.hdr, vi->hdr_len);
> > +
> > return head_skb;
> >
> > case XDP_TX:
>
On Wed, Jan 24, 2024 at 5:16 PM Xuan Zhuo <[email protected]> wrote:
>
> On Wed, 24 Jan 2024 16:57:20 +0800, Liang Chen <[email protected]> wrote:
> > For the XDP_PASS scenario of the XDP path, the skb constructed with
> > xdp_buff does not include the virtio header. Adding the virtio header
> > information back when creating the skb.
> >
> > Signed-off-by: Liang Chen <[email protected]>
> > ---
> > drivers/net/virtio_net.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index b56828804e5f..2de46eb4c661 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1270,6 +1270,9 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> > if (unlikely(!skb))
> > goto err;
> >
> > + /* Store the original virtio header for subsequent use by the driver. */
> > + memcpy(skb_vnet_common_hdr(skb), &virtnet_xdp.hdr, vi->hdr_len);
>
> About this, a spec is waiting for voting.
>
> This may change the logic of the csum offset and so on.
>
> Please not do this.
>
Sure. This will be dropped in v3. Thanks.
> Thanks.
>
>
> > +
> > if (metasize)
> > skb_metadata_set(skb, metasize);
> >
> > @@ -1635,6 +1638,9 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> > head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
> > if (unlikely(!head_skb))
> > break;
> > + /* Store the original virtio header for subsequent use by the driver. */
> > + memcpy(skb_vnet_common_hdr(head_skb), &virtnet_xdp.hdr, vi->hdr_len);
> > +
> > return head_skb;
> >
> > case XDP_TX:
> > --
> > 2.40.1
> >
On Wed, Jan 24, 2024 at 5:08 PM Xuan Zhuo <[email protected]> wrote:
>
> On Wed, 24 Jan 2024 16:57:19 +0800, Liang Chen <[email protected]> wrote:
> > The xdp program may overwrite the inline virtio header. To ensure the
> > integrity of the virtio header, it is saved in a data structure that
> > wraps both the xdp_buff and the header before running the xdp program.
> >
> > Signed-off-by: Liang Chen <[email protected]>
> > ---
> > drivers/net/virtio_net.c | 43 +++++++++++++++++++++++++++++-----------
> > 1 file changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index d7ce4a1011ea..b56828804e5f 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -349,6 +349,11 @@ struct virtio_net_common_hdr {
> > };
> > };
> >
> > +struct virtnet_xdp_buff {
> > + struct xdp_buff xdp;
> > + struct virtio_net_common_hdr hdr;
>
> Not all items of the hdr are useful, we can just save the useful items.
>
Sure. Will do in v3. Thanks.
> > +};
> > +
> > static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
> >
> > static bool is_xdp_frame(void *ptr)
> > @@ -1199,9 +1204,10 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> > unsigned int headroom = vi->hdr_len + header_offset;
> > struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset;
> > struct page *page = virt_to_head_page(buf);
> > + struct virtnet_xdp_buff virtnet_xdp;
> > struct page *xdp_page;
> > + struct xdp_buff *xdp;
> > unsigned int buflen;
> > - struct xdp_buff xdp;
> > struct sk_buff *skb;
> > unsigned int metasize = 0;
> > u32 act;
> > @@ -1233,17 +1239,23 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> > page = xdp_page;
> > }
> >
> > - xdp_init_buff(&xdp, buflen, &rq->xdp_rxq);
> > - xdp_prepare_buff(&xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
> > + xdp = &virtnet_xdp.xdp;
> > + xdp_init_buff(xdp, buflen, &rq->xdp_rxq);
> > + xdp_prepare_buff(xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
> > xdp_headroom, len, true);
> >
> > - act = virtnet_xdp_handler(xdp_prog, &xdp, dev, xdp_xmit, stats);
> > + /* Copy out the virtio header, as it may be overwritten by the
> > + * xdp program.
> > + */
> > + memcpy(&virtnet_xdp.hdr, hdr, vi->hdr_len);
>
> Can we put this into virtnet_xdp_handler?
>
> And just do that when the hash is negotiated.
>
> > +
> > + act = virtnet_xdp_handler(xdp_prog, xdp, dev, xdp_xmit, stats);
> >
> > switch (act) {
> > case XDP_PASS:
> > /* Recalculate length in case bpf program changed it */
> > - len = xdp.data_end - xdp.data;
> > - metasize = xdp.data - xdp.data_meta;
> > + len = xdp->data_end - xdp->data;
> > + metasize = xdp->data - xdp->data_meta;
> > break;
> >
> > case XDP_TX:
> > @@ -1254,7 +1266,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> > goto err_xdp;
> > }
> >
> > - skb = virtnet_build_skb(buf, buflen, xdp.data - buf, len);
> > + skb = virtnet_build_skb(buf, buflen, xdp->data - buf, len);
> > if (unlikely(!skb))
> > goto err;
> >
> > @@ -1591,10 +1603,11 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> > int num_buf = virtio16_to_cpu(vi->vdev, hdr->num_buffers);
> > struct page *page = virt_to_head_page(buf);
> > int offset = buf - page_address(page);
> > + struct virtnet_xdp_buff virtnet_xdp;
> > unsigned int xdp_frags_truesz = 0;
> > struct sk_buff *head_skb;
> > unsigned int frame_sz;
> > - struct xdp_buff xdp;
> > + struct xdp_buff *xdp;
> > void *data;
> > u32 act;
> > int err;
> > @@ -1604,16 +1617,22 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> > if (unlikely(!data))
> > goto err_xdp;
> >
> > - err = virtnet_build_xdp_buff_mrg(dev, vi, rq, &xdp, data, len, frame_sz,
> > + xdp = &virtnet_xdp.xdp;
> > + err = virtnet_build_xdp_buff_mrg(dev, vi, rq, xdp, data, len, frame_sz,
> > &num_buf, &xdp_frags_truesz, stats);
> > if (unlikely(err))
> > goto err_xdp;
> >
> > - act = virtnet_xdp_handler(xdp_prog, &xdp, dev, xdp_xmit, stats);
> > + /* Copy out the virtio header, as it may be overwritten by the
> > + * xdp program.
> > + */
> > + memcpy(&virtnet_xdp.hdr, hdr, vi->hdr_len);
> > +
> > + act = virtnet_xdp_handler(xdp_prog, xdp, dev, xdp_xmit, stats);
> >
> > switch (act) {
> > case XDP_PASS:
> > - head_skb = build_skb_from_xdp_buff(dev, vi, &xdp, xdp_frags_truesz);
> > + head_skb = build_skb_from_xdp_buff(dev, vi, xdp, xdp_frags_truesz);
> > if (unlikely(!head_skb))
> > break;
> > return head_skb;
> > @@ -1626,7 +1645,7 @@ static struct sk_buff *receive_mergeable_xdp(struct net_device *dev,
> > break;
> > }
> >
> > - put_xdp_frags(&xdp);
> > + put_xdp_frags(xdp);
> >
> > err_xdp:
> > put_page(page);
> > --
> > 2.40.1
> >