On Tue, 28 Apr 2020 17:50:11 +0800 Jason Wang <jasowang@xxxxxxxxxx> 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. > So let's just not reserve space for vnet header in this case. > > Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx> > --- > 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
