From: Liang Chen <liangchen.linux@gmail.com>
To: Jason Wang <jasowang@redhat.com>
Cc: John Fastabend <john.fastabend@gmail.com>,
Xuan Zhuo <xuanzhuo@linux.alibaba.com>,
Paolo Abeni <pabeni@redhat.com>,
Jesper Dangaard Brouer <hawk@kernel.org>,
mst@redhat.com, hengqi@linux.alibaba.com, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, netdev@vger.kernel.org,
virtualization@lists.linux.dev, linux-kernel@vger.kernel.org,
bpf@vger.kernel.org, daniel@iogearbox.net, ast@kernel.org
Subject: Re: [PATCH net-next v5] virtio_net: Support RX hash XDP hint
Date: Thu, 11 Apr 2024 14:09:30 +0800 [thread overview]
Message-ID: <CAKhg4tJZ5FePt+UfcEvDoYtKCUVAX=vo_XUJkF4bzYBhiKmjgQ@mail.gmail.com> (raw)
In-Reply-To: <CACGkMEsfW-j=W8fdTofUsF2a9rRujCs8TH6wkFvZkpLc68ZEEg@mail.gmail.com>
On Mon, Apr 8, 2024 at 2:41 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Apr 1, 2024 at 11:38 AM Liang Chen <liangchen.linux@gmail.com> wrote:
> >
> > On Thu, Feb 29, 2024 at 4:37 PM Liang Chen <liangchen.linux@gmail.com> wrote:
> > >
> > > On Tue, Feb 27, 2024 at 4:42 AM John Fastabend <john.fastabend@gmail.com> wrote:
> > > >
> > > > Jason Wang wrote:
> > > > > On Fri, Feb 23, 2024 at 9:42 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Fri, 09 Feb 2024 13:57:25 +0100, Paolo Abeni <pabeni@redhat.com> 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@redhat.com> 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@redhat.com> 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@kernel.org> 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@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_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
> >
>
prev parent reply other threads:[~2024-04-11 6:09 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-02 12:11 [PATCH net-next v5] virtio_net: Support RX hash XDP hint Liang Chen
2024-02-02 16:19 ` Jesper Dangaard Brouer
2024-02-03 2:56 ` Liang Chen
2024-02-06 10:43 ` Paolo Abeni
2024-02-07 2:54 ` Liang Chen
2024-02-07 14:27 ` Paolo Abeni
2024-02-09 10:39 ` Liang Chen
2024-02-09 12:57 ` Paolo Abeni
2024-02-22 20:09 ` Michael S. Tsirkin
2024-02-23 1:37 ` Xuan Zhuo
2024-02-26 4:59 ` Jason Wang
2024-02-26 20:42 ` John Fastabend
2024-02-29 8:37 ` Liang Chen
2024-04-01 3:38 ` Liang Chen
2024-04-08 6:41 ` Jason Wang
2024-04-11 6:09 ` Liang Chen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKhg4tJZ5FePt+UfcEvDoYtKCUVAX=vo_XUJkF4bzYBhiKmjgQ@mail.gmail.com' \
--to=liangchen.linux@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=hawk@kernel.org \
--cc=hengqi@linux.alibaba.com \
--cc=jasowang@redhat.com \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=virtualization@lists.linux.dev \
--cc=xuanzhuo@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).