Message-ID: <c2fc1cc2-b0e4-40de-9f0c-0e383c0fe1b3@hetzner-cloud.de>
Date: Wed, 26 Feb 2025 18:14:18 +0100
From: Marcus Wichelmann <marcus.wichelmann@...zner-cloud.de>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
 linux-kselftest@...r.kernel.org
Cc: jasowang@...hat.com, andrew+netdev@...n.ch, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, andrii@...nel.org,
 eddyz87@...il.com, mykolal@...com, ast@...nel.org, daniel@...earbox.net,
 martin.lau@...ux.dev, song@...nel.org, yonghong.song@...ux.dev,
 john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me,
 haoluo@...gle.com, jolsa@...nel.org, shuah@...nel.org, hawk@...nel.org
Subject: Re: [PATCH bpf-next v3 4/6] selftests/bpf: refactor
 xdp_context_functional test and bpf program

Am 25.02.25 um 19:32 schrieb Willem de Bruijn:
> Marcus Wichelmann wrote:
>> The existing XDP metadata test works by creating a veth pair and
>> attaching XDP & TC programs that drop the packet when the condition of
>> the test isn't fulfilled. The test then pings through the veth pair and
>> succeeds when the ping comes through.
>>
>> While this test works great for a veth pair, it is hard to replicate for
>> tap devices to test the XDP metadata support of them. A similar test for
>> the tun driver would either involve logic to reply to the ping request,
>> or would have to capture the packet to check if it was dropped or not.
>>
>> To make the testing of other drivers easier while still maximizing code
>> reuse, this commit refactors the existing xdp_context_functional test to
>> use a test_result map. Instead of conditionally passing or dropping the
>> packet, the TC program is changed to copy the received metadata into the
>> value of that single-entry array map. Tests can then verify that the map
>> value matches the expectation.
>>
>> This testing logic is easy to adapt to other network drivers as the only
>> remaining requirement is that there is some way to send a custom
>> Ethernet packet through it that triggers the XDP & TC programs.
>>
>> The payload of the Ethernet packet is used as the test data that is
>> expected to be passed as metadata from the XDP to the TC program and
>> written to the map. It has a fixed size of 32 bytes which is a
>> reasonalbe size that should be supported by both drivers. Additional
>> packet headers are not necessary for the test and were therefore skipped
>> to keep the testing code short.
>>
>> Signed-off-by: Marcus Wichelmann <marcus.wichelmann@...zner-cloud.de>
> 
>> -	SYS(close, "ip addr add " RX_ADDR "/24 dev " RX_NAME);
> 
> Why remove the Rx and Tx addresses?

Because the testing methodology is changed from pinging and IP address
to sending a custom packet and handling it in XDP instead, the IP addresses
are no longer relevant for this test.
I removed them to reduce the testing code to only the setup tasks that are
needed for the test to do its job.

I can add a note about this to the commit message.

>> [...]
>>   
>> -	tc_prog = bpf_object__find_program_by_name(skel->obj, "ing_cls");
>> -	if (!ASSERT_OK_PTR(tc_prog, "open ing_cls prog"))
>> -		goto close;
>> -
>> -	tc_opts.prog_fd = bpf_program__fd(tc_prog);
>> +	tc_opts.prog_fd = bpf_program__fd(skel->progs.ing_cls);
> 
> This refactor seems irrelevant to the goal of this patch? Same below.

Fair point. These were drive-by cleanups to make the testing code less
verbose. But you're right, they are not relevant and just inflate the diff.
I'll remove them from the patch.

>> [...]
>> @@ -179,9 +231,18 @@ void test_xdp_context_functional(void)
>>   	if (!ASSERT_OK_PTR(nstoken, "setns tx_ns"))
>>   		goto close;
>>   
>> -	SYS(close, "ip addr add " TX_ADDR "/24 dev " TX_NAME);
>> +	SYS(close, "ip link set dev " TX_NAME " arp off");
>>   	SYS(close, "ip link set dev " TX_NAME " up");
> 
>>   SEC("tc")
>>   int ing_cls(struct __sk_buff *ctx)
>>   {
>> -	__u8 *data, *data_meta, *data_end;
>> -	__u32 diff = 0;
>> +	void *data_meta = (void *)(unsigned long)ctx->data_meta;
>> +	void *data = (void *)(unsigned long)ctx->data;
>>   
>> -	data_meta = ctx_ptr(ctx, data_meta);
>> -	data_end  = ctx_ptr(ctx, data_end);
>> -	data      = ctx_ptr(ctx, data);
> 
> Similarly: why these changes?
> 
> In general, please minimize your patch to the core purpose. Avoid
> including "cleanups" or other such refactors. Or justify the choice
> explicitly in the commit message.

I had rewritten the function bodies of these two BPF programs from
scratch because the functionality changed significantly. That's where
this comes from.
I'll try to reuse more of the existing code to make the diff shorter.

>> -
>> -	if (data + ETH_ALEN > data_end ||
>> -	    data_meta + round_up(ETH_ALEN, 4) > data)
>> +	if (data_meta + META_SIZE > data)
>>   		return TC_ACT_SHOT;
>>   
>> -	diff |= ((__u32 *)data_meta)[0] ^ ((__u32 *)data)[0];
>> -	diff |= ((__u16 *)data_meta)[2] ^ ((__u16 *)data)[2];
>> +	int key = 0;
>> +
>> +	bpf_map_update_elem(&test_result, &key, data_meta, BPF_ANY);
>>   
>> -	return diff ? TC_ACT_SHOT : TC_ACT_OK;
>> +	return TC_ACT_SHOT;
>>   }
>>   
>>   SEC("xdp")
>>   int ing_xdp(struct xdp_md *ctx)
>>   {
>> -	__u8 *data, *data_meta, *data_end;
>> -	int ret;
>> -
>> -	ret = bpf_xdp_adjust_meta(ctx, -round_up(ETH_ALEN, 4));
>> +	int ret = bpf_xdp_adjust_meta(ctx, -META_SIZE);
>>   	if (ret < 0)
>>   		return XDP_DROP;
>>   
>> -	data_meta = ctx_ptr(ctx, data_meta);
>> -	data_end  = ctx_ptr(ctx, data_end);
>> -	data      = ctx_ptr(ctx, data);
>> +	void *data_meta = (void *)(unsigned long)ctx->data_meta;
>> +	void *data = (void *)(unsigned long)ctx->data;
>> +	void *data_end = (void *)(unsigned long)ctx->data_end;
>>   
>> -	if (data + ETH_ALEN > data_end ||
>> -	    data_meta + round_up(ETH_ALEN, 4) > data)
>> +	void *payload = data + sizeof(struct ethhdr);
>> +
>> +	if (data_meta + META_SIZE > data || payload + META_SIZE > data_end)
>>   		return XDP_DROP;
>>   
>> -	__builtin_memcpy(data_meta, data, ETH_ALEN);
>> +	__builtin_memcpy(data_meta, payload, META_SIZE);
>> +
>>   	return XDP_PASS;
>>   }
>>   
>> -- 
>> 2.43.0
>>

Powered by blists - more mailing lists