Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Mon, Apr 8, 2024 at 2:41 PM Jason Wang wrote:
>
> On Mon, Apr 1, 2024 at 11:38 AM Liang Chen wrote:
> >
> > On Thu, Feb 29, 2024 at 4:37 PM Liang Chen
> > wrote:
> > >
> > > On Tue, Feb 27, 2024 at 4:42 AM John Fastabend
> > > wrote:
> > > >
> > > > Jason Wang wrote:
> > > > > On Fri, Feb 23, 2024 at 9:42 AM Xuan Zhuo
> > > > > wrote:
> > > > > >
> > > > > > On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni
> > > > > > wrote:
> > > > > > > On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > > > > > > > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > > > > > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > > > > > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > > > > > > > > [...]
> > > > > > > > > > > > > > @@ -1033,6 +1039,16 @@ static void
> > > > > > > > > > > > > > put_xdp_frags(struct xdp_buff *xdp)
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > +static void virtnet_xdp_save_rx_hash(struct
> > > > > > > > > > > > > > virtnet_xdp_buff *virtnet_xdp,
> > > > > > > > > > > > > > + struct
> > > > > > > > > > > > > > net_device *dev,
> > > > > > > > > > > > > > + struct
> > > > > > > > > > > > > > virtio_net_hdr_v1_hash *hdr_hash)
> > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > > > > > > > > + virtnet_xdp->hash_value =
> > > > > > > > > > > > > > hdr_hash->hash_value;
> > > > > > > > > > > > > > + virtnet_xdp->hash_report =
> > > > > > > > > > > > > > hdr_hash->hash_report;
> > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > +
> > > > > > > > > > > > >
> > > > > > > > > > > > > Would it be possible to store a pointer to hdr_hash
> > > > > > > > > > > > > in virtnet_xdp_buff,
> > > > > > > > > > > > > with the purpose of delaying extracting this, until
> > > > > > > > > > > > > and only if XDP
> > > > > > > > > > > > > bpf_prog calls the kfunc?
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > That seems to be the way v1 works,
> > > > > > > > > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > > > > > > > > . But it was pointed out that the inline header may be
> > > > > > > > > > > > overwritten by
> > > > > > > > > > > > the xdp prog, so the hash is copied out to maintain its
> > > > > > > > > > > > integrity.
> > > > > > > > > > >
> > > > > > > > > > > Why? isn't XDP supposed to get write access only to the
> > > > > > > > > > > pkt
> > > > > > > > > > > contents/buffer?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Normally, an XDP program accesses only the packet data.
> > > > > > > > > > However,
> > > > > > > > > > there's also an XDP RX Metadata area, referenced by the
> > > > > > > > > > data_meta
> > > > > > > > > > pointer. This pointer can be adjusted with
> > > > > > > > > > bpf_xdp_adjust_meta to
> > > > > > > > > > point somewhere ahead of the data buffer, thereby granting
> > > > > > > > > > the XDP
> > > > > > > > > > program access to the virtio header located immediately
> > > > > > > > > > before the
> > > > > > > > >
> > > > > > > > > AFAICS bpf_xdp_adjust_meta() does not allow moving the
> > > > > > > > > meta_data before
> > > > > > > > > xdp->data_hard_start:
> > > > > > > > >
> > > > > > > > > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> > > > > > > > >
> > > > > > > > > and virtio net set such field after the virtio_net_hdr:
> > > > > > > > >
> > > > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > > > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> > > > > > > > >
> > > > > > > > > I don't see how the virtio hdr could be touched? Possibly
> > > > > > > > > even more
> > > > > > > > > important: if such thing is possible, I think is should be
> > > > > > > > > somewhat
> > > > > > > > > denied (for the same reason an H/W nic should prevent XDP from
> > > > > > > > > modifying its own buffer descriptor).
> > > > > > > >
> > > > > > > > Thank you for highlighting this concern. The header layout
> > > > > > > > differs
> > > > > > > > slightly between small and mergeable mode. Taking 'mergeable
> > > > > > > > mode' as
> > > > > > > > an example, after calling xdp_prepare_buff the layout of
> > > > > > > > xdp_buff
> > > > > > > > would be as depicted in the diagram below,
> > > > > > > >
> > > > > > > >
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Mon, Apr 1, 2024 at 11:38 AM Liang Chen wrote:
>
> On Thu, Feb 29, 2024 at 4:37 PM Liang Chen wrote:
> >
> > On Tue, Feb 27, 2024 at 4:42 AM John Fastabend
> > wrote:
> > >
> > > Jason Wang wrote:
> > > > On Fri, Feb 23, 2024 at 9:42 AM Xuan Zhuo
> > > > wrote:
> > > > >
> > > > > On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni
> > > > > wrote:
> > > > > > On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > > > > > > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > > > > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > > > > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > > > > > > > [...]
> > > > > > > > > > > > > @@ -1033,6 +1039,16 @@ static void
> > > > > > > > > > > > > put_xdp_frags(struct xdp_buff *xdp)
> > > > > > > > > > > > > }
> > > > > > > > > > > > > }
> > > > > > > > > > > > >
> > > > > > > > > > > > > +static void virtnet_xdp_save_rx_hash(struct
> > > > > > > > > > > > > virtnet_xdp_buff *virtnet_xdp,
> > > > > > > > > > > > > + struct net_device
> > > > > > > > > > > > > *dev,
> > > > > > > > > > > > > + struct
> > > > > > > > > > > > > virtio_net_hdr_v1_hash *hdr_hash)
> > > > > > > > > > > > > +{
> > > > > > > > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > > > > > > > + virtnet_xdp->hash_value =
> > > > > > > > > > > > > hdr_hash->hash_value;
> > > > > > > > > > > > > + virtnet_xdp->hash_report =
> > > > > > > > > > > > > hdr_hash->hash_report;
> > > > > > > > > > > > > + }
> > > > > > > > > > > > > +}
> > > > > > > > > > > > > +
> > > > > > > > > > > >
> > > > > > > > > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > > > > > > > > virtnet_xdp_buff,
> > > > > > > > > > > > with the purpose of delaying extracting this, until and
> > > > > > > > > > > > only if XDP
> > > > > > > > > > > > bpf_prog calls the kfunc?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > That seems to be the way v1 works,
> > > > > > > > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > > > > > > > . But it was pointed out that the inline header may be
> > > > > > > > > > > overwritten by
> > > > > > > > > > > the xdp prog, so the hash is copied out to maintain its
> > > > > > > > > > > integrity.
> > > > > > > > > >
> > > > > > > > > > Why? isn't XDP supposed to get write access only to the pkt
> > > > > > > > > > contents/buffer?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Normally, an XDP program accesses only the packet data.
> > > > > > > > > However,
> > > > > > > > > there's also an XDP RX Metadata area, referenced by the
> > > > > > > > > data_meta
> > > > > > > > > pointer. This pointer can be adjusted with
> > > > > > > > > bpf_xdp_adjust_meta to
> > > > > > > > > point somewhere ahead of the data buffer, thereby granting
> > > > > > > > > the XDP
> > > > > > > > > program access to the virtio header located immediately
> > > > > > > > > before the
> > > > > > > >
> > > > > > > > AFAICS bpf_xdp_adjust_meta() does not allow moving the
> > > > > > > > meta_data before
> > > > > > > > xdp->data_hard_start:
> > > > > > > >
> > > > > > > > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> > > > > > > >
> > > > > > > > and virtio net set such field after the virtio_net_hdr:
> > > > > > > >
> > > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> > > > > > > >
> > > > > > > > I don't see how the virtio hdr could be touched? Possibly even
> > > > > > > > more
> > > > > > > > important: if such thing is possible, I think is should be
> > > > > > > > somewhat
> > > > > > > > denied (for the same reason an H/W nic should prevent XDP from
> > > > > > > > modifying its own buffer descriptor).
> > > > > > >
> > > > > > > Thank you for highlighting this concern. The header layout differs
> > > > > > > slightly between small and mergeable mode. Taking 'mergeable
> > > > > > > mode' as
> > > > > > > an example, after calling xdp_prepare_buff the layout of xdp_buff
> > > > > > > would be as depicted in the diagram below,
> > > > > > >
> > > > > > > buf
> > > > > > >|
> > > > > > >v
> > > > > > > +--+--+-+
> > > > > > > | xdp headroom | virtio header| packet |
> > > > > > > | (256 bytes) | (20 bytes) | content |
> > > > > > >
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Thu, Feb 29, 2024 at 4:37 PM Liang Chen wrote:
>
> On Tue, Feb 27, 2024 at 4:42 AM John Fastabend
> wrote:
> >
> > Jason Wang wrote:
> > > On Fri, Feb 23, 2024 at 9:42 AM Xuan Zhuo
> > > wrote:
> > > >
> > > > On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni
> > > > wrote:
> > > > > On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > > > > > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni
> > > > > > wrote:
> > > > > > >
> > > > > > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > > > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > > > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > > > > > > wrote:
> > > > > > > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > > > > > > [...]
> > > > > > > > > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct
> > > > > > > > > > > > xdp_buff *xdp)
> > > > > > > > > > > > }
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > +static void virtnet_xdp_save_rx_hash(struct
> > > > > > > > > > > > virtnet_xdp_buff *virtnet_xdp,
> > > > > > > > > > > > + struct net_device
> > > > > > > > > > > > *dev,
> > > > > > > > > > > > + struct
> > > > > > > > > > > > virtio_net_hdr_v1_hash *hdr_hash)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > > > > > > + virtnet_xdp->hash_value =
> > > > > > > > > > > > hdr_hash->hash_value;
> > > > > > > > > > > > + virtnet_xdp->hash_report =
> > > > > > > > > > > > hdr_hash->hash_report;
> > > > > > > > > > > > + }
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > >
> > > > > > > > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > > > > > > > virtnet_xdp_buff,
> > > > > > > > > > > with the purpose of delaying extracting this, until and
> > > > > > > > > > > only if XDP
> > > > > > > > > > > bpf_prog calls the kfunc?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > That seems to be the way v1 works,
> > > > > > > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > > > > > > . But it was pointed out that the inline header may be
> > > > > > > > > > overwritten by
> > > > > > > > > > the xdp prog, so the hash is copied out to maintain its
> > > > > > > > > > integrity.
> > > > > > > > >
> > > > > > > > > Why? isn't XDP supposed to get write access only to the pkt
> > > > > > > > > contents/buffer?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Normally, an XDP program accesses only the packet data. However,
> > > > > > > > there's also an XDP RX Metadata area, referenced by the
> > > > > > > > data_meta
> > > > > > > > pointer. This pointer can be adjusted with bpf_xdp_adjust_meta
> > > > > > > > to
> > > > > > > > point somewhere ahead of the data buffer, thereby granting the
> > > > > > > > XDP
> > > > > > > > program access to the virtio header located immediately before
> > > > > > > > the
> > > > > > >
> > > > > > > AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data
> > > > > > > before
> > > > > > > xdp->data_hard_start:
> > > > > > >
> > > > > > > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> > > > > > >
> > > > > > > and virtio net set such field after the virtio_net_hdr:
> > > > > > >
> > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> > > > > > >
> > > > > > > I don't see how the virtio hdr could be touched? Possibly even
> > > > > > > more
> > > > > > > important: if such thing is possible, I think is should be
> > > > > > > somewhat
> > > > > > > denied (for the same reason an H/W nic should prevent XDP from
> > > > > > > modifying its own buffer descriptor).
> > > > > >
> > > > > > Thank you for highlighting this concern. The header layout differs
> > > > > > slightly between small and mergeable mode. Taking 'mergeable mode'
> > > > > > as
> > > > > > an example, after calling xdp_prepare_buff the layout of xdp_buff
> > > > > > would be as depicted in the diagram below,
> > > > > >
> > > > > > buf
> > > > > >|
> > > > > >v
> > > > > > +--+--+-+
> > > > > > | xdp headroom | virtio header| packet |
> > > > > > | (256 bytes) | (20 bytes) | content |
> > > > > > +--+--+-+
> > > > > > ^ ^
> > > > > > | |
> > > > > > data_hard_startdata
> > > > > > data_meta
> > > > > >
> > >
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Tue, Feb 27, 2024 at 4:42 AM John Fastabend wrote:
>
> Jason Wang wrote:
> > On Fri, Feb 23, 2024 at 9:42 AM Xuan Zhuo
> > wrote:
> > >
> > > On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni wrote:
> > > > On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > > > > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni wrote:
> > > > > >
> > > > > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > > > > > wrote:
> > > > > > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > > > > > [...]
> > > > > > > > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct
> > > > > > > > > > > xdp_buff *xdp)
> > > > > > > > > > > }
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > +static void virtnet_xdp_save_rx_hash(struct
> > > > > > > > > > > virtnet_xdp_buff *virtnet_xdp,
> > > > > > > > > > > + struct net_device *dev,
> > > > > > > > > > > + struct
> > > > > > > > > > > virtio_net_hdr_v1_hash *hdr_hash)
> > > > > > > > > > > +{
> > > > > > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > > > > > + virtnet_xdp->hash_value =
> > > > > > > > > > > hdr_hash->hash_value;
> > > > > > > > > > > + virtnet_xdp->hash_report =
> > > > > > > > > > > hdr_hash->hash_report;
> > > > > > > > > > > + }
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > >
> > > > > > > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > > > > > > virtnet_xdp_buff,
> > > > > > > > > > with the purpose of delaying extracting this, until and
> > > > > > > > > > only if XDP
> > > > > > > > > > bpf_prog calls the kfunc?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > That seems to be the way v1 works,
> > > > > > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > > > > > . But it was pointed out that the inline header may be
> > > > > > > > > overwritten by
> > > > > > > > > the xdp prog, so the hash is copied out to maintain its
> > > > > > > > > integrity.
> > > > > > > >
> > > > > > > > Why? isn't XDP supposed to get write access only to the pkt
> > > > > > > > contents/buffer?
> > > > > > > >
> > > > > > >
> > > > > > > Normally, an XDP program accesses only the packet data. However,
> > > > > > > there's also an XDP RX Metadata area, referenced by the data_meta
> > > > > > > pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
> > > > > > > point somewhere ahead of the data buffer, thereby granting the XDP
> > > > > > > program access to the virtio header located immediately before the
> > > > > >
> > > > > > AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data
> > > > > > before
> > > > > > xdp->data_hard_start:
> > > > > >
> > > > > > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> > > > > >
> > > > > > and virtio net set such field after the virtio_net_hdr:
> > > > > >
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> > > > > >
> > > > > > I don't see how the virtio hdr could be touched? Possibly even more
> > > > > > important: if such thing is possible, I think is should be somewhat
> > > > > > denied (for the same reason an H/W nic should prevent XDP from
> > > > > > modifying its own buffer descriptor).
> > > > >
> > > > > Thank you for highlighting this concern. The header layout differs
> > > > > slightly between small and mergeable mode. Taking 'mergeable mode' as
> > > > > an example, after calling xdp_prepare_buff the layout of xdp_buff
> > > > > would be as depicted in the diagram below,
> > > > >
> > > > > buf
> > > > >|
> > > > >v
> > > > > +--+--+-+
> > > > > | xdp headroom | virtio header| packet |
> > > > > | (256 bytes) | (20 bytes) | content |
> > > > > +--+--+-+
> > > > > ^ ^
> > > > > | |
> > > > > data_hard_startdata
> > > > > data_meta
> > > > >
> > > > > If 'bpf_xdp_adjust_meta' repositions the 'data_meta' pointer a little
> > > > > towards 'data_hard_start', it would point to the inline header, thus
> > > > > potentially allowing the XDP program to access the inline header.
>
> Fairly late to the thread sorry. Given above layout does it make sense to
> just delay extraction to the kfunc as suggested above? Sure the XDP program
> could smash the
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
Jason Wang wrote:
> On Fri, Feb 23, 2024 at 9:42 AM Xuan Zhuo wrote:
> >
> > On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni wrote:
> > > On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > > > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni wrote:
> > > > >
> > > > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni
> > > > > > wrote:
> > > > > > >
> > > > > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > > > > wrote:
> > > > > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > > > > [...]
> > > > > > > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct
> > > > > > > > > > xdp_buff *xdp)
> > > > > > > > > > }
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +static void virtnet_xdp_save_rx_hash(struct
> > > > > > > > > > virtnet_xdp_buff *virtnet_xdp,
> > > > > > > > > > + struct net_device *dev,
> > > > > > > > > > + struct
> > > > > > > > > > virtio_net_hdr_v1_hash *hdr_hash)
> > > > > > > > > > +{
> > > > > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > > > > + virtnet_xdp->hash_value =
> > > > > > > > > > hdr_hash->hash_value;
> > > > > > > > > > + virtnet_xdp->hash_report =
> > > > > > > > > > hdr_hash->hash_report;
> > > > > > > > > > + }
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > > > > > virtnet_xdp_buff,
> > > > > > > > > with the purpose of delaying extracting this, until and only
> > > > > > > > > if XDP
> > > > > > > > > bpf_prog calls the kfunc?
> > > > > > > > >
> > > > > > > >
> > > > > > > > That seems to be the way v1 works,
> > > > > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > > > > . But it was pointed out that the inline header may be
> > > > > > > > overwritten by
> > > > > > > > the xdp prog, so the hash is copied out to maintain its
> > > > > > > > integrity.
> > > > > > >
> > > > > > > Why? isn't XDP supposed to get write access only to the pkt
> > > > > > > contents/buffer?
> > > > > > >
> > > > > >
> > > > > > Normally, an XDP program accesses only the packet data. However,
> > > > > > there's also an XDP RX Metadata area, referenced by the data_meta
> > > > > > pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
> > > > > > point somewhere ahead of the data buffer, thereby granting the XDP
> > > > > > program access to the virtio header located immediately before the
> > > > >
> > > > > AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data
> > > > > before
> > > > > xdp->data_hard_start:
> > > > >
> > > > > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> > > > >
> > > > > and virtio net set such field after the virtio_net_hdr:
> > > > >
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> > > > >
> > > > > I don't see how the virtio hdr could be touched? Possibly even more
> > > > > important: if such thing is possible, I think is should be somewhat
> > > > > denied (for the same reason an H/W nic should prevent XDP from
> > > > > modifying its own buffer descriptor).
> > > >
> > > > Thank you for highlighting this concern. The header layout differs
> > > > slightly between small and mergeable mode. Taking 'mergeable mode' as
> > > > an example, after calling xdp_prepare_buff the layout of xdp_buff
> > > > would be as depicted in the diagram below,
> > > >
> > > > buf
> > > >|
> > > >v
> > > > +--+--+-+
> > > > | xdp headroom | virtio header| packet |
> > > > | (256 bytes) | (20 bytes) | content |
> > > > +--+--+-+
> > > > ^ ^
> > > > | |
> > > > data_hard_startdata
> > > > data_meta
> > > >
> > > > If 'bpf_xdp_adjust_meta' repositions the 'data_meta' pointer a little
> > > > towards 'data_hard_start', it would point to the inline header, thus
> > > > potentially allowing the XDP program to access the inline header.
Fairly late to the thread sorry. Given above layout does it make sense to
just delay extraction to the kfunc as suggested above? Sure the XDP program
could smash the entry in virtio header, but this is already the case for
anything else there. A program writing over the virtio header is likely
buggy anyways. Worse that might happen is bad rss values and mappings?
I like seeing more use cases for the hints though.
Thanks!
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Fri, Feb 23, 2024 at 9:42 AM Xuan Zhuo wrote:
>
> On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni wrote:
> > On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni wrote:
> > > >
> > > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni wrote:
> > > > > >
> > > > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > > > wrote:
> > > > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > > > [...]
> > > > > > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct
> > > > > > > > > xdp_buff *xdp)
> > > > > > > > > }
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff
> > > > > > > > > *virtnet_xdp,
> > > > > > > > > + struct net_device *dev,
> > > > > > > > > + struct
> > > > > > > > > virtio_net_hdr_v1_hash *hdr_hash)
> > > > > > > > > +{
> > > > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > > > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > > > > > > > > + virtnet_xdp->hash_report =
> > > > > > > > > hdr_hash->hash_report;
> > > > > > > > > + }
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > >
> > > > > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > > > > virtnet_xdp_buff,
> > > > > > > > with the purpose of delaying extracting this, until and only if
> > > > > > > > XDP
> > > > > > > > bpf_prog calls the kfunc?
> > > > > > > >
> > > > > > >
> > > > > > > That seems to be the way v1 works,
> > > > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > > > . But it was pointed out that the inline header may be
> > > > > > > overwritten by
> > > > > > > the xdp prog, so the hash is copied out to maintain its integrity.
> > > > > >
> > > > > > Why? isn't XDP supposed to get write access only to the pkt
> > > > > > contents/buffer?
> > > > > >
> > > > >
> > > > > Normally, an XDP program accesses only the packet data. However,
> > > > > there's also an XDP RX Metadata area, referenced by the data_meta
> > > > > pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
> > > > > point somewhere ahead of the data buffer, thereby granting the XDP
> > > > > program access to the virtio header located immediately before the
> > > >
> > > > AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data before
> > > > xdp->data_hard_start:
> > > >
> > > > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> > > >
> > > > and virtio net set such field after the virtio_net_hdr:
> > > >
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> > > >
> > > > I don't see how the virtio hdr could be touched? Possibly even more
> > > > important: if such thing is possible, I think is should be somewhat
> > > > denied (for the same reason an H/W nic should prevent XDP from
> > > > modifying its own buffer descriptor).
> > >
> > > Thank you for highlighting this concern. The header layout differs
> > > slightly between small and mergeable mode. Taking 'mergeable mode' as
> > > an example, after calling xdp_prepare_buff the layout of xdp_buff
> > > would be as depicted in the diagram below,
> > >
> > > buf
> > >|
> > >v
> > > +--+--+-+
> > > | xdp headroom | virtio header| packet |
> > > | (256 bytes) | (20 bytes) | content |
> > > +--+--+-+
> > > ^ ^
> > > | |
> > > data_hard_startdata
> > > data_meta
> > >
> > > If 'bpf_xdp_adjust_meta' repositions the 'data_meta' pointer a little
> > > towards 'data_hard_start', it would point to the inline header, thus
> > > potentially allowing the XDP program to access the inline header.
> >
> > I see. That layout was completely unexpected to me.
> >
> > AFAICS the virtio_net driver tries to avoid accessing/using the
> > virtio_net_hdr after the XDP program execution, so nothing tragic
> > should happen.
> >
> > @Michael, @Jason, I guess the above is like that by design? Isn't it a
> > bit fragile?
Yes.
>
> YES. We process it carefully. That brings some troubles, we hope to put the
> virtio-net header to the vring desc like other NICs. But that is a big
> project.
Yes, and we still need to support the "legacy" layout.
>
> I think this patch is ok, this can be merged to net-next firstly.
+1
Thanks
>
> Thanks.
>
>
> >
> > Thanks!
> >
> > Paolo
> >
>
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni wrote:
> On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni wrote:
> > >
> > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni wrote:
> > > > >
> > > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > > wrote:
> > > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > > [...]
> > > > > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct
> > > > > > > > xdp_buff *xdp)
> > > > > > > > }
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff
> > > > > > > > *virtnet_xdp,
> > > > > > > > + struct net_device *dev,
> > > > > > > > + struct
> > > > > > > > virtio_net_hdr_v1_hash *hdr_hash)
> > > > > > > > +{
> > > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > > > > > > > + virtnet_xdp->hash_report = hdr_hash->hash_report;
> > > > > > > > + }
> > > > > > > > +}
> > > > > > > > +
> > > > > > >
> > > > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > > > virtnet_xdp_buff,
> > > > > > > with the purpose of delaying extracting this, until and only if
> > > > > > > XDP
> > > > > > > bpf_prog calls the kfunc?
> > > > > > >
> > > > > >
> > > > > > That seems to be the way v1 works,
> > > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > > . But it was pointed out that the inline header may be overwritten
> > > > > > by
> > > > > > the xdp prog, so the hash is copied out to maintain its integrity.
> > > > >
> > > > > Why? isn't XDP supposed to get write access only to the pkt
> > > > > contents/buffer?
> > > > >
> > > >
> > > > Normally, an XDP program accesses only the packet data. However,
> > > > there's also an XDP RX Metadata area, referenced by the data_meta
> > > > pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
> > > > point somewhere ahead of the data buffer, thereby granting the XDP
> > > > program access to the virtio header located immediately before the
> > >
> > > AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data before
> > > xdp->data_hard_start:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> > >
> > > and virtio net set such field after the virtio_net_hdr:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> > >
> > > I don't see how the virtio hdr could be touched? Possibly even more
> > > important: if such thing is possible, I think is should be somewhat
> > > denied (for the same reason an H/W nic should prevent XDP from
> > > modifying its own buffer descriptor).
> >
> > Thank you for highlighting this concern. The header layout differs
> > slightly between small and mergeable mode. Taking 'mergeable mode' as
> > an example, after calling xdp_prepare_buff the layout of xdp_buff
> > would be as depicted in the diagram below,
> >
> > buf
> >|
> >v
> > +--+--+-+
> > | xdp headroom | virtio header| packet |
> > | (256 bytes) | (20 bytes) | content |
> > +--+--+-+
> > ^ ^
> > | |
> > data_hard_startdata
> > data_meta
> >
> > If 'bpf_xdp_adjust_meta' repositions the 'data_meta' pointer a little
> > towards 'data_hard_start', it would point to the inline header, thus
> > potentially allowing the XDP program to access the inline header.
>
> I see. That layout was completely unexpected to me.
>
> AFAICS the virtio_net driver tries to avoid accessing/using the
> virtio_net_hdr after the XDP program execution, so nothing tragic
> should happen.
>
> @Michael, @Jason, I guess the above is like that by design? Isn't it a
> bit fragile?
YES. We process it carefully. That brings some troubles, we hope to put the
virtio-net header to the vring desc like other NICs. But that is a big project.
I think this patch is ok, this can be merged to net-next firstly.
Thanks.
>
> Thanks!
>
> Paolo
>
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Fri, Feb 09, 2024 at 01:57:25PM +0100, Paolo Abeni wrote:
> On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni wrote:
> > >
> > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni wrote:
> > > > >
> > > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > > wrote:
> > > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > > [...]
> > > > > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct
> > > > > > > > xdp_buff *xdp)
> > > > > > > > }
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff
> > > > > > > > *virtnet_xdp,
> > > > > > > > + struct net_device *dev,
> > > > > > > > + struct
> > > > > > > > virtio_net_hdr_v1_hash *hdr_hash)
> > > > > > > > +{
> > > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > > > > > > > + virtnet_xdp->hash_report = hdr_hash->hash_report;
> > > > > > > > + }
> > > > > > > > +}
> > > > > > > > +
> > > > > > >
> > > > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > > > virtnet_xdp_buff,
> > > > > > > with the purpose of delaying extracting this, until and only if
> > > > > > > XDP
> > > > > > > bpf_prog calls the kfunc?
> > > > > > >
> > > > > >
> > > > > > That seems to be the way v1 works,
> > > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > > . But it was pointed out that the inline header may be overwritten
> > > > > > by
> > > > > > the xdp prog, so the hash is copied out to maintain its integrity.
> > > > >
> > > > > Why? isn't XDP supposed to get write access only to the pkt
> > > > > contents/buffer?
> > > > >
> > > >
> > > > Normally, an XDP program accesses only the packet data. However,
> > > > there's also an XDP RX Metadata area, referenced by the data_meta
> > > > pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
> > > > point somewhere ahead of the data buffer, thereby granting the XDP
> > > > program access to the virtio header located immediately before the
> > >
> > > AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data before
> > > xdp->data_hard_start:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> > >
> > > and virtio net set such field after the virtio_net_hdr:
> > >
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> > >
> > > I don't see how the virtio hdr could be touched? Possibly even more
> > > important: if such thing is possible, I think is should be somewhat
> > > denied (for the same reason an H/W nic should prevent XDP from
> > > modifying its own buffer descriptor).
> >
> > Thank you for highlighting this concern. The header layout differs
> > slightly between small and mergeable mode. Taking 'mergeable mode' as
> > an example, after calling xdp_prepare_buff the layout of xdp_buff
> > would be as depicted in the diagram below,
> >
> > buf
> >|
> >v
> > +--+--+-+
> > | xdp headroom | virtio header| packet |
> > | (256 bytes) | (20 bytes) | content |
> > +--+--+-+
> > ^ ^
> > | |
> > data_hard_startdata
> > data_meta
> >
> > If 'bpf_xdp_adjust_meta' repositions the 'data_meta' pointer a little
> > towards 'data_hard_start', it would point to the inline header, thus
> > potentially allowing the XDP program to access the inline header.
>
> I see. That layout was completely unexpected to me.
>
> AFAICS the virtio_net driver tries to avoid accessing/using the
> virtio_net_hdr after the XDP program execution, so nothing tragic
> should happen.
>
> @Michael, @Jason, I guess the above is like that by design? Isn't it a
> bit fragile?
>
> Thanks!
>
> Paolo
I agree it is all a bit fragile, not sure how to do better without extra
copies though ...
--
MST
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni wrote:
> >
> > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni wrote:
> > > >
> > > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > > wrote:
> > > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > > [...]
> > > > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct xdp_buff
> > > > > > > *xdp)
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff
> > > > > > > *virtnet_xdp,
> > > > > > > + struct net_device *dev,
> > > > > > > + struct virtio_net_hdr_v1_hash
> > > > > > > *hdr_hash)
> > > > > > > +{
> > > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > > > > > > + virtnet_xdp->hash_report = hdr_hash->hash_report;
> > > > > > > + }
> > > > > > > +}
> > > > > > > +
> > > > > >
> > > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > > virtnet_xdp_buff,
> > > > > > with the purpose of delaying extracting this, until and only if XDP
> > > > > > bpf_prog calls the kfunc?
> > > > > >
> > > > >
> > > > > That seems to be the way v1 works,
> > > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > > . But it was pointed out that the inline header may be overwritten by
> > > > > the xdp prog, so the hash is copied out to maintain its integrity.
> > > >
> > > > Why? isn't XDP supposed to get write access only to the pkt
> > > > contents/buffer?
> > > >
> > >
> > > Normally, an XDP program accesses only the packet data. However,
> > > there's also an XDP RX Metadata area, referenced by the data_meta
> > > pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
> > > point somewhere ahead of the data buffer, thereby granting the XDP
> > > program access to the virtio header located immediately before the
> >
> > AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data before
> > xdp->data_hard_start:
> >
> > https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
> >
> > and virtio net set such field after the virtio_net_hdr:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
> >
> > I don't see how the virtio hdr could be touched? Possibly even more
> > important: if such thing is possible, I think is should be somewhat
> > denied (for the same reason an H/W nic should prevent XDP from
> > modifying its own buffer descriptor).
>
> Thank you for highlighting this concern. The header layout differs
> slightly between small and mergeable mode. Taking 'mergeable mode' as
> an example, after calling xdp_prepare_buff the layout of xdp_buff
> would be as depicted in the diagram below,
>
> buf
>|
>v
> +--+--+-+
> | xdp headroom | virtio header| packet |
> | (256 bytes) | (20 bytes) | content |
> +--+--+-+
> ^ ^
> | |
> data_hard_startdata
> data_meta
>
> If 'bpf_xdp_adjust_meta' repositions the 'data_meta' pointer a little
> towards 'data_hard_start', it would point to the inline header, thus
> potentially allowing the XDP program to access the inline header.
I see. That layout was completely unexpected to me.
AFAICS the virtio_net driver tries to avoid accessing/using the
virtio_net_hdr after the XDP program execution, so nothing tragic
should happen.
@Michael, @Jason, I guess the above is like that by design? Isn't it a
bit fragile?
Thanks!
Paolo
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni wrote:
>
> On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni wrote:
> > >
> > > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > > wrote:
> > > > > On 02/02/2024 13.11, Liang Chen wrote:
> > > [...]
> > > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct xdp_buff
> > > > > > *xdp)
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff
> > > > > > *virtnet_xdp,
> > > > > > + struct net_device *dev,
> > > > > > + struct virtio_net_hdr_v1_hash
> > > > > > *hdr_hash)
> > > > > > +{
> > > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > > > > > + virtnet_xdp->hash_report = hdr_hash->hash_report;
> > > > > > + }
> > > > > > +}
> > > > > > +
> > > > >
> > > > > Would it be possible to store a pointer to hdr_hash in
> > > > > virtnet_xdp_buff,
> > > > > with the purpose of delaying extracting this, until and only if XDP
> > > > > bpf_prog calls the kfunc?
> > > > >
> > > >
> > > > That seems to be the way v1 works,
> > > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > > . But it was pointed out that the inline header may be overwritten by
> > > > the xdp prog, so the hash is copied out to maintain its integrity.
> > >
> > > Why? isn't XDP supposed to get write access only to the pkt
> > > contents/buffer?
> > >
> >
> > Normally, an XDP program accesses only the packet data. However,
> > there's also an XDP RX Metadata area, referenced by the data_meta
> > pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
> > point somewhere ahead of the data buffer, thereby granting the XDP
> > program access to the virtio header located immediately before the
>
> AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data before
> xdp->data_hard_start:
>
> https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
>
> and virtio net set such field after the virtio_net_hdr:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
> https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
>
> I don't see how the virtio hdr could be touched? Possibly even more
> important: if such thing is possible, I think is should be somewhat
> denied (for the same reason an H/W nic should prevent XDP from
> modifying its own buffer descriptor).
Thank you for highlighting this concern. The header layout differs
slightly between small and mergeable mode. Taking 'mergeable mode' as
an example, after calling xdp_prepare_buff the layout of xdp_buff
would be as depicted in the diagram below,
buf
|
v
+--+--+-+
| xdp headroom | virtio header| packet |
| (256 bytes) | (20 bytes) | content |
+--+--+-+
^ ^
| |
data_hard_startdata
data_meta
If 'bpf_xdp_adjust_meta' repositions the 'data_meta' pointer a little
towards 'data_hard_start', it would point to the inline header, thus
potentially allowing the XDP program to access the inline header.
We will take a closer look on how to prevent the inline header from
being altered, possibly by borrowing some ideas from other
xdp_metadata_ops implementation.
Thanks,
Liang
>
> Cheers,
>
> Paolo
>
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni wrote:
> >
> > On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > > wrote:
> > > > On 02/02/2024 13.11, Liang Chen wrote:
> > [...]
> > > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct xdp_buff *xdp)
> > > > > }
> > > > > }
> > > > >
> > > > > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff
> > > > > *virtnet_xdp,
> > > > > + struct net_device *dev,
> > > > > + struct virtio_net_hdr_v1_hash
> > > > > *hdr_hash)
> > > > > +{
> > > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > > > > + virtnet_xdp->hash_report = hdr_hash->hash_report;
> > > > > + }
> > > > > +}
> > > > > +
> > > >
> > > > Would it be possible to store a pointer to hdr_hash in virtnet_xdp_buff,
> > > > with the purpose of delaying extracting this, until and only if XDP
> > > > bpf_prog calls the kfunc?
> > > >
> > >
> > > That seems to be the way v1 works,
> > > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > > . But it was pointed out that the inline header may be overwritten by
> > > the xdp prog, so the hash is copied out to maintain its integrity.
> >
> > Why? isn't XDP supposed to get write access only to the pkt
> > contents/buffer?
> >
>
> Normally, an XDP program accesses only the packet data. However,
> there's also an XDP RX Metadata area, referenced by the data_meta
> pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
> point somewhere ahead of the data buffer, thereby granting the XDP
> program access to the virtio header located immediately before the
AFAICS bpf_xdp_adjust_meta() does not allow moving the meta_data before
xdp->data_hard_start:
https://elixir.bootlin.com/linux/latest/source/net/core/filter.c#L4210
and virtio net set such field after the virtio_net_hdr:
https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1218
https://elixir.bootlin.com/linux/latest/source/drivers/net/virtio_net.c#L1420
I don't see how the virtio hdr could be touched? Possibly even more
important: if such thing is possible, I think is should be somewhat
denied (for the same reason an H/W nic should prevent XDP from
modifying its own buffer descriptor).
Cheers,
Paolo
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni wrote:
>
> On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> > On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> > wrote:
> > > On 02/02/2024 13.11, Liang Chen wrote:
> [...]
> > > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct xdp_buff *xdp)
> > > > }
> > > > }
> > > >
> > > > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff
> > > > *virtnet_xdp,
> > > > + struct net_device *dev,
> > > > + struct virtio_net_hdr_v1_hash
> > > > *hdr_hash)
> > > > +{
> > > > + if (dev->features & NETIF_F_RXHASH) {
> > > > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > > > + virtnet_xdp->hash_report = hdr_hash->hash_report;
> > > > + }
> > > > +}
> > > > +
> > >
> > > Would it be possible to store a pointer to hdr_hash in virtnet_xdp_buff,
> > > with the purpose of delaying extracting this, until and only if XDP
> > > bpf_prog calls the kfunc?
> > >
> >
> > That seems to be the way v1 works,
> > https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> > . But it was pointed out that the inline header may be overwritten by
> > the xdp prog, so the hash is copied out to maintain its integrity.
>
> Why? isn't XDP supposed to get write access only to the pkt
> contents/buffer?
>
Normally, an XDP program accesses only the packet data. However,
there's also an XDP RX Metadata area, referenced by the data_meta
pointer. This pointer can be adjusted with bpf_xdp_adjust_meta to
point somewhere ahead of the data buffer, thereby granting the XDP
program access to the virtio header located immediately before the
packet data.
Thanks,
Liang
> if the XDP program can really change the virtio_net_hdr, that looks
> potentially dangerous/bug prone regardless of this patch.
>
> Thanks,
>
> Paolo
>
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Sat, 2024-02-03 at 10:56 +0800, Liang Chen wrote:
> On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer
> wrote:
> > On 02/02/2024 13.11, Liang Chen wrote:
[...]
> > > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct xdp_buff *xdp)
> > > }
> > > }
> > >
> > > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff
> > > *virtnet_xdp,
> > > + struct net_device *dev,
> > > + struct virtio_net_hdr_v1_hash
> > > *hdr_hash)
> > > +{
> > > + if (dev->features & NETIF_F_RXHASH) {
> > > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > > + virtnet_xdp->hash_report = hdr_hash->hash_report;
> > > + }
> > > +}
> > > +
> >
> > Would it be possible to store a pointer to hdr_hash in virtnet_xdp_buff,
> > with the purpose of delaying extracting this, until and only if XDP
> > bpf_prog calls the kfunc?
> >
>
> That seems to be the way v1 works,
> https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
> . But it was pointed out that the inline header may be overwritten by
> the xdp prog, so the hash is copied out to maintain its integrity.
Why? isn't XDP supposed to get write access only to the pkt
contents/buffer?
if the XDP program can really change the virtio_net_hdr, that looks
potentially dangerous/bug prone regardless of this patch.
Thanks,
Paolo
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On Sat, Feb 3, 2024 at 12:20 AM Jesper Dangaard Brouer wrote:
>
>
>
> On 02/02/2024 13.11, Liang Chen wrote:
> > 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/20231015141644.260646-1-akihiko.od...@daynix.com/#r
> >
> > Signed-off-by: Liang Chen
> > Reviewed-by: Xuan Zhuo
> > Acked-by: Jason Wang
> > ---
> >Changes from v4:
> > - cc complete list of maintainers
> > ---
> > drivers/net/virtio_net.c | 98 +++-
> > 1 file changed, 86 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index d7ce4a1011ea..7ce666c86ee0 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -349,6 +349,12 @@ struct virtio_net_common_hdr {
> > };
> > };
> >
> > +struct virtnet_xdp_buff {
> > + struct xdp_buff xdp;
> > + __le32 hash_value;
> > + __le16 hash_report;
> > +};
> > +
> > static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
> >
> > static bool is_xdp_frame(void *ptr)
> > @@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct xdp_buff *xdp)
> > }
> > }
> >
> > +static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff *virtnet_xdp,
> > + struct net_device *dev,
> > + struct virtio_net_hdr_v1_hash *hdr_hash)
> > +{
> > + if (dev->features & NETIF_F_RXHASH) {
> > + virtnet_xdp->hash_value = hdr_hash->hash_value;
> > + virtnet_xdp->hash_report = hdr_hash->hash_report;
> > + }
> > +}
> > +
>
> Would it be possible to store a pointer to hdr_hash in virtnet_xdp_buff,
> with the purpose of delaying extracting this, until and only if XDP
> bpf_prog calls the kfunc?
>
That seems to be the way v1 works,
https://lore.kernel.org/all/20240122102256.261374-1-liangchen.li...@gmail.com/
. But it was pointed out that the inline header may be overwritten by
the xdp prog, so the hash is copied out to maintain its integrity.
Thanks,
Liang
>
>
> > static int virtnet_xdp_handler(struct bpf_prog *xdp_prog, struct xdp_buff
> > *xdp,
> > struct net_device *dev,
> > unsigned int *xdp_xmit,
> > @@ -1199,9 +1215,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 +1250,20 @@ static struct sk_buff *receive_small_xdp(struct
> > net_device *dev,
> > page = xdp_page;
> > }
> >
> > - xdp_init_buff(, buflen, >xdp_rxq);
> > - xdp_prepare_buff(, buf + VIRTNET_RX_PAD + vi->hdr_len,
> > + xdp = _xdp.xdp;
> > + xdp_init_buff(xdp, buflen, >xdp_rxq);
> > + xdp_prepare_buff(xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
> >xdp_headroom, len, true);
> >
> > - act = virtnet_xdp_handler(xdp_prog, , dev, xdp_xmit, stats);
> > + virtnet_xdp_save_rx_hash(_xdp, dev, (void *)hdr);
> > +
> > + 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 +1274,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 +1611,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;
> >
Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
On 02/02/2024 13.11, Liang Chen wrote:
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/20231015141644.260646-1-akihiko.od...@daynix.com/#r
Signed-off-by: Liang Chen
Reviewed-by: Xuan Zhuo
Acked-by: Jason Wang
---
Changes from v4:
- cc complete list of maintainers
---
drivers/net/virtio_net.c | 98 +++-
1 file changed, 86 insertions(+), 12 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d7ce4a1011ea..7ce666c86ee0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -349,6 +349,12 @@ struct virtio_net_common_hdr {
};
};
+struct virtnet_xdp_buff {
+ struct xdp_buff xdp;
+ __le32 hash_value;
+ __le16 hash_report;
+};
+
static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
static bool is_xdp_frame(void *ptr)
@@ -1033,6 +1039,16 @@ static void put_xdp_frags(struct xdp_buff *xdp)
}
}
+static void virtnet_xdp_save_rx_hash(struct virtnet_xdp_buff *virtnet_xdp,
+struct net_device *dev,
+struct virtio_net_hdr_v1_hash *hdr_hash)
+{
+ if (dev->features & NETIF_F_RXHASH) {
+ virtnet_xdp->hash_value = hdr_hash->hash_value;
+ virtnet_xdp->hash_report = hdr_hash->hash_report;
+ }
+}
+
Would it be possible to store a pointer to hdr_hash in virtnet_xdp_buff,
with the purpose of delaying extracting this, until and only if XDP
bpf_prog calls the kfunc?
static int virtnet_xdp_handler(struct bpf_prog *xdp_prog, struct xdp_buff
*xdp,
struct net_device *dev,
unsigned int *xdp_xmit,
@@ -1199,9 +1215,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 +1250,20 @@ static struct sk_buff *receive_small_xdp(struct
net_device *dev,
page = xdp_page;
}
- xdp_init_buff(, buflen, >xdp_rxq);
- xdp_prepare_buff(, buf + VIRTNET_RX_PAD + vi->hdr_len,
+ xdp = _xdp.xdp;
+ xdp_init_buff(xdp, buflen, >xdp_rxq);
+ xdp_prepare_buff(xdp, buf + VIRTNET_RX_PAD + vi->hdr_len,
xdp_headroom, len, true);
- act = virtnet_xdp_handler(xdp_prog, , dev, xdp_xmit, stats);
+ virtnet_xdp_save_rx_hash(_xdp, dev, (void *)hdr);
+
+ 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 +1274,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 +1611,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 +1625,19 @@ 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, , data, len, frame_sz,
+ xdp = _xdp.xdp;
+ err = virtnet_build_xdp_buff_mrg(dev, vi, rq, xdp, data, len, frame_sz,
_buf, _frags_truesz, stats);
if (unlikely(err))
goto err_xdp;
- act =
