On Tue, 28 Apr 2020 17:50:11 +0800
Jason Wang [off-list ref] wrote:
We tried to reserve space for vnet header before
xdp.data_hard_start. But this is useless since the packet could be
modified by XDP which may invalidate the information stored in the
header and there's no way for XDP to know the existence of the vnet
header currently.
I think this is wrong. We can reserve space for vnet header before
xdp.data_hard_start, and it will be safe and cannot be modified by XDP
as BPF program cannot access data before xdp.data_hard_start.
quoted hunk ↗ jump to hunk
So let's just not reserve space for vnet header in this case.
Signed-off-by: Jason Wang <redacted>
---
drivers/net/virtio_net.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2fe7a3188282..9bdaf2425e6e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -681,8 +681,8 @@ static struct sk_buff *receive_small(struct net_device *dev,
page = xdp_page;
}
- xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
- xdp.data = xdp.data_hard_start + xdp_headroom;
+ xdp.data_hard_start = buf + VIRTNET_RX_PAD;
+ xdp.data = xdp.data_hard_start + xdp_headroom + vi->hdr_len;;
I think this is wrong. You are exposing the vi header info, which will
be overwritten when code creates an xdp_frame. I find it very fragile,
as later in the code the vi header info is copied, but only if xdp_prog
is not loaded, so in principle it's okay, but when someone later
figures out that we want to look at this area, we will be in trouble
(and I expect this will be needed when we work towards multi-buffer
XDP).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer