netdev - Re: [PATCH bpf-next v2 20/20] selftests/bpf: check checksum level in xdp

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: <ZKatOMUMTfbhd5Y0@lincoln>
Date: Thu, 6 Jul 2023 14:02:00 +0200
From: Larysa Zaremba <larysa.zaremba@...el.com>
To: Jesper Dangaard Brouer <jbrouer@...hat.com>
CC: <bpf@...r.kernel.org>, <brouer@...hat.com>, <ast@...nel.org>,
	<daniel@...earbox.net>, <andrii@...nel.org>, <martin.lau@...ux.dev>,
	<song@...nel.org>, <yhs@...com>, <john.fastabend@...il.com>,
	<kpsingh@...nel.org>, <sdf@...gle.com>, <haoluo@...gle.com>,
	<jolsa@...nel.org>, David Ahern <dsahern@...il.com>, Jakub Kicinski
	<kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com>, Anatoly Burakov
	<anatoly.burakov@...el.com>, Alexander Lobakin <alexandr.lobakin@...el.com>,
	Magnus Karlsson <magnus.karlsson@...il.com>, Maryam Tahhan
	<mtahhan@...hat.com>, <xdp-hints@...-project.net>, <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 20/20] selftests/bpf: check checksum level in
 xdp_metadata

On Thu, Jul 06, 2023 at 12:25:10PM +0200, Jesper Dangaard Brouer wrote:
> 
> 
> On 03/07/2023 20.12, Larysa Zaremba wrote:
> > Verify, whether kfunc in xdp_metadata test correctly returns checksum level
> > of zero.
> > 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
> > ---
> >   tools/testing/selftests/bpf/prog_tests/xdp_metadata.c | 3 +++
> >   tools/testing/selftests/bpf/progs/xdp_metadata.c      | 7 +++++++
> >   2 files changed, 10 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> > index 50ac9f570bc5..6c71d712932e 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_metadata.c
> > @@ -228,6 +228,9 @@ static int verify_xsk_metadata(struct xsk *xsk)
> >   	if (!ASSERT_EQ(meta->rx_vlan_proto, VLAN_PID, "rx_vlan_proto"))
> >   		return -1;
> > +	if (!ASSERT_NEQ(meta->rx_csum_lvl, 0, "rx_csum_lvl"))
> > +		return -1;
> 
> Not-equal ("NEQ") to 0 feels weird here.
> Below you set meta->rx_csum_lvl=1 in case meta->rx_csum_lvl==0.
> 
> Thus, test can pass if meta->rx_csum_lvl happens to be a random value.
> We could set meta->rx_csum_lvl to 42 in case meta->rx_csum_lvl==0, and
> then use a ASSERT_EQ==42 to be more certain of the case we are testing are
> fulfilled.
>

I just copied the approach used for timestamp. I think you are right and I 
should have make the new code better.

ASSERT_NEQ(0) is also used for rx_hash. It would be a good idea to go and fix 
those too, but the patchset has already ballooned too much for me, so I would 
leave it for later.

With ASSERT_EQ for checksum level, I think comparing it to "1" should be enough. 
Do I guess correctly, the main problem with ASSERT_NEQ is uninitialized memory?
 
Another value that is less magical than 42 would be "4", because csum_level 
takes 2 bits, so it is the smallest value that does not correspod to 
any valid checksum level.
 
> 
> > +
> >   	xsk_ring_cons__release(&xsk->rx, 1);
> >   	refill_rx(xsk, comp_addr);
> > diff --git a/tools/testing/selftests/bpf/progs/xdp_metadata.c b/tools/testing/selftests/bpf/progs/xdp_metadata.c
> > index 382984a5d1c9..6f7223d581b7 100644
> > --- a/tools/testing/selftests/bpf/progs/xdp_metadata.c
> > +++ b/tools/testing/selftests/bpf/progs/xdp_metadata.c
> > @@ -26,6 +26,8 @@ extern int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, __u32 *hash,
> >   extern int bpf_xdp_metadata_rx_vlan_tag(const struct xdp_md *ctx,
> >   					__u16 *vlan_tag,
> >   					__be16 *vlan_proto) __ksym;
> > +extern int bpf_xdp_metadata_rx_csum_lvl(const struct xdp_md *ctx,
> > +					__u8 *csum_level) __ksym;
> >   SEC("xdp")
> >   int rx(struct xdp_md *ctx)
> > @@ -62,6 +64,11 @@ int rx(struct xdp_md *ctx)
> >   	bpf_xdp_metadata_rx_hash(ctx, &meta->rx_hash, &meta->rx_hash_type);
> >   	bpf_xdp_metadata_rx_vlan_tag(ctx, &meta->rx_vlan_tag, &meta->rx_vlan_proto);
> > +	/* Same as with timestamp, zero is expected */
> > +	ret = bpf_xdp_metadata_rx_csum_lvl(ctx, &meta->rx_csum_lvl);
> > +	if (!ret && meta->rx_csum_lvl == 0)
> > +		meta->rx_csum_lvl = 1;
> > +
> 
> IMHO it is more human-readable-code to rename "ret" variable "err".
> 
> I know you are just reusing variable "ret", so it's not really your fault.
> 
> 
> 
> >   	return bpf_redirect_map(&xsk, ctx->rx_queue_index, XDP_PASS);
> >   }
>