On Mon, Apr 8, 2024 at 2:41 PM Jason Wang <jasowang@xxxxxxxxxx> wrote:
>
> On Mon, Apr 1, 2024 at 11:38 AM Liang Chen <liangchen.linux@xxxxxxxxx> wrote:
> >
> > On Thu, Feb 29, 2024 at 4:37 PM Liang Chen <liangchen.linux@xxxxxxxxx> wrote:
> > >
> > > On Tue, Feb 27, 2024 at 4:42 AM John Fastabend <john.fastabend@xxxxxxxxx> wrote:
> > > >
> > > > Jason Wang wrote:
> > > > > On Fri, Feb 23, 2024 at 9:42 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
> > > > > > > On Fri, 2024-02-09 at 18:39 +0800, Liang Chen wrote:
> > > > > > > > On Wed, Feb 7, 2024 at 10:27 PM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, 2024-02-07 at 10:54 +0800, Liang Chen wrote:
> > > > > > > > > > On Tue, Feb 6, 2024 at 6:44 PM Paolo Abeni <pabeni@xxxxxxxxxx> 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 <hawk@xxxxxxxxxx> 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.linux@xxxxxxxxx/
> > > > > > > > > > > > . 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_start data
> > > > > > > > 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?
> > >
> > > Thank you for raising the concern. I am not quite sure if the XDP
> > > program is considered buggy, as it is agnostic to the layout of the
> > > inline header.
> > > Let's say an XDP program calls bpf_xdp_adjust_meta to adjust data_meta
> > > to point to the inline header and overwrites it without even knowing
> > > of its existence. Later, when the XDP program invokes the kfunc to
> > > retrieve the hash, incorrect data would be returned. In this case, the
> > > XDP program seems to be doing everything legally but ends up with the
> > > wrong hash data.
> > >
> > > Thanks,
> > > Liang
> > >
> >
> > I haven’t received any feedback yet, so I’m under the impression that
> > the XDP program is still considered buggy in the scenario mentioned
> > above, and the overall behavior is as designed from XDP perspective.
> > Looking up the intel igc driver, it also does not bother with this
> > particular aspect.
>
> So let's post a new version with all the detailed explanations as above and see?
Sure. Thanks!
>
> >
> > Given this context, we don't need to be concerned about the hash value
> > being overwritten. So if there aren't any objections, I plan to remove
> > the preservation of the hash value in the next iteration.
> >
> > Thanks,
> > Liang
>
> Thanks
>
> >
> > > >
> > > > I like seeing more use cases for the hints though.
> > > >
> > > > Thanks!
> > > > John
> >
>
