linux-kernel - Re: [PATCH bpf-next 2/2] selftests/bpf: Enhance XDP Rx Metadata Handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aGVYdjDKNBdWdrQ8@mini-arch>
Date: Wed, 2 Jul 2025 09:04:06 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: "Song, Yoong Siang" <yoong.siang.song@...el.com>
Cc: "David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, Jonathan Corbet <corbet@....net>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Stanislav Fomichev <sdf@...ichev.me>,
	Andrii Nakryiko <andrii@...nel.org>,
	Martin KaFai Lau <martin.lau@...ux.dev>,
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
	Yonghong Song <yonghong.song@...ux.dev>,
	KP Singh <kpsingh@...nel.org>, Hao Luo <haoluo@...gle.com>,
	Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
	Shuah Khan <shuah@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Enhance XDP Rx Metadata
 Handling

On 07/02, Song, Yoong Siang wrote:
> On Wednesday, July 2, 2025 11:19 PM, Stanislav Fomichev <stfomichev@...il.com> wrote:
> >On 07/02, Song, Yoong Siang wrote:
> >> On Wednesday, July 2, 2025 10:23 AM, Song, Yoong Siang
> ><yoong.siang.song@...el.com> wrote:
> >> >On Wednesday, July 2, 2025 12:31 AM, Stanislav Fomichev
> ><stfomichev@...il.com>
> >> >wrote:
> >> >>On 07/01, Song Yoong Siang wrote:
> >> >>> Introduce the XDP_METADATA_SIZE macro to ensure that user applications can
> >> >>> consistently retrieve the correct location of struct xdp_meta.
> >> >>>
> >> >>> Prior to this commit, the XDP program adjusted the data_meta backward by
> >> >>> the size of struct xdp_meta, while the user application retrieved the data
> >> >>> by calculating backward from the data pointer. This approach only worked if
> >> >>> xdp_buff->data_meta was equal to xdp_buff->data before calling
> >> >>> bpf_xdp_adjust_meta.
> >> >>>
> >> >>> With the introduction of XDP_METADATA_SIZE, both the XDP program and user
> >> >>> application now calculate and identify the location of struct xdp_meta from
> >> >>> the data pointer. This ensures the implementation remains functional even
> >> >>> when there is device-reserved metadata, making the tests more portable
> >> >>> across different NICs.
> >> >>>
> >> >>> Signed-off-by: Song Yoong Siang <yoong.siang.song@...el.com>
> >> >>> ---
> >> >>>  tools/testing/selftests/bpf/prog_tests/xdp_metadata.c |  2 +-
> >> >>>  tools/testing/selftests/bpf/progs/xdp_hw_metadata.c   | 10 +++++++++-
> >> >>>  tools/testing/selftests/bpf/progs/xdp_metadata.c      |  8 +++++++-
> >> >>>  tools/testing/selftests/bpf/xdp_hw_metadata.c         |  2 +-
> >> >>>  tools/testing/selftests/bpf/xdp_metadata.h            |  7 +++++++
> >> >>>  5 files changed, 25 insertions(+), 4 deletions(-)
> >> >>>
> >> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> >> >>b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> >> >>> index 19f92affc2da..8d6c2633698b 100644
> >> >>> --- a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> >> >>> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> >> >>> @@ -302,7 +302,7 @@ static int verify_xsk_metadata(struct xsk *xsk, bool
> >> >>sent_from_af_xdp)
> >> >>>
> >> >>>  	/* custom metadata */
> >> >>>
> >> >>> -	meta = data - sizeof(struct xdp_meta);
> >> >>> +	meta = data - XDP_METADATA_SIZE;
> >> >>>
> >> >>>  	if (!ASSERT_NEQ(meta->rx_timestamp, 0, "rx_timestamp"))
> >> >>>  		return -1;
> >> >>> diff --git a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> >> >>b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> >> >>> index 330ece2eabdb..72242ac1cdcd 100644
> >> >>> --- a/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> >> >>> +++ b/tools/testing/selftests/bpf/progs/xdp_hw_metadata.c
> >> >>> @@ -27,6 +27,7 @@ extern int bpf_xdp_metadata_rx_vlan_tag(const struct
> >> >>xdp_md *ctx,
> >> >>>  SEC("xdp.frags")
> >> >>>  int rx(struct xdp_md *ctx)
> >> >>>  {
> >> >>> +	int metalen_used, metalen_to_adjust;
> >> >>>  	void *data, *data_meta, *data_end;
> >> >>>  	struct ipv6hdr *ip6h = NULL;
> >> >>>  	struct udphdr *udp = NULL;
> >> >>> @@ -72,7 +73,14 @@ int rx(struct xdp_md *ctx)
> >> >>>  		return XDP_PASS;
> >> >>>  	}
> >> >>>
> >> >>> -	err = bpf_xdp_adjust_meta(ctx, -(int)sizeof(struct xdp_meta));
> >> >>
> >> >>[..]
> >> >>
> >> >>> +	metalen_used = ctx->data - ctx->data_meta;
> >> >>
> >> >>Is the intent here to query how much metadata has been consumed/reserved
> >> >>by the driver?
> >> >Yes.
> >> >
> >> >>Looking at IGC it has the following code/comment:
> >> >>
> >> >>	bi->xdp->data += IGC_TS_HDR_LEN;
> >> >>
> >> >>	/* HW timestamp has been copied into local variable. Metadata
> >> >>	 * length when XDP program is called should be 0.
> >> >>	 */
> >> >>	bi->xdp->data_meta += IGC_TS_HDR_LEN;
> >> >>
> >> >>Are you sure that metadata size is correctly exposed to the bpf program?
> >> >You are right, the current igc driver didn't expose the metadata size correctly.
> >> >I submitted [1] to fix it.
> >> >
> >> >[1] https://patchwork.ozlabs.org/project/intel-wired-
> >> >lan/patch/20250701080955.3273137-1-yoong.siang.song@...el.com/
> >> >
> >> >>
> >> >>My assumptions was that we should just unconditionally do
> >bpf_xdp_adjust_meta
> >> >>with -XDP_METADATA_SIZE and that should be good enough.
> >> >
> >> >The checking is just for precautions. No problem if directly adjust the meta
> >> >unconditionally.
> >> >That will save processing time for each packet as well.
> >> >I will remove the checking and submit v2.
> >> >
> >> >Thanks & Regards
> >> >Siang
> >> >
> >>
> >> Hi Stanislav Fomichev,
> >>
> >> I submitted v2. But after that, I think twice. IMHO,
> >> err = bpf_xdp_adjust_meta(ctx, (int)(ctx->data - ctx->data_meta -
> >XDP_METADATA_SIZE));
> >> is better than
> >> err = bpf_xdp_adjust_meta(ctx, -(int)XDP_METADATA_SIZE);
> >> because it is more robust.
> >>
> >> Any thoughts?
> >
> >My preference is on keeping everything as is and converting to
> >-(int)XDP_METADATA_SIZE. Making IGC properly expose (temporary) metadata len
> >is a user visible change, not sure we have a good justification?
> 
> Thank you for your feedback. I agree that we don't have a strong justification
> for making the metadata length user-visible at this time. I concur with your
> preference to keep everything as is and proceed with -(int)XDP_METADATA_SIZE.
> 
> Btw, do you think whether my first patch which changes the documentation is
> still needed or not?

Yes, the documentation is super useful, let's keep it!