Received-SPF: pass (google.com: domain of linux-kernel+bounces-170894-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <327aafc5-2bea-4cc2-9ada-9320146b5e28@intel.com>
Date: Tue, 7 May 2024 10:47:03 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net] selftests/bpf: fix pointer arithmetic in
 test_xdp_do_redirect
To: Michal Schmidt <mschmidt@redhat.com>
CC: Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann
	<daniel@iogearbox.net>, "David S. Miller" <davem@davemloft.net>, "Jakub
 Kicinski" <kuba@kernel.org>, Jesper Dangaard Brouer <hawk@kernel.org>, "John
 Fastabend" <john.fastabend@gmail.com>, Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <martin.lau@linux.dev>, Eduard Zingerman
	<eddyz87@gmail.com>, Song Liu <song@kernel.org>, Yonghong Song
	<yonghong.song@linux.dev>, KP Singh <kpsingh@kernel.org>, Stanislav Fomichev
	<sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
	Mykola Lysenko <mykolal@fb.com>, Shuah Khan <shuah@kernel.org>,
	=?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>,
	<netdev@vger.kernel.org>, <bpf@vger.kernel.org>,
	<linux-kselftest@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240506145023.214248-1-mschmidt@redhat.com>
From: Alexander Lobakin <aleksander.lobakin@intel.com>
Content-Language: en-US
In-Reply-To: <20240506145023.214248-1-mschmidt@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NWEySzBPZGxESkRHRm1EbU9JeDJreEZLM2JWN3dTS25xbjIvekF1K1d4cXRK?=
 =?utf-8?B?YW5oUkZRRXBKYTRHRTZraGxrT0FQdllxNTRZN2wrbm4wb2xDTFJURWttbHo0?=
 =?utf-8?B?eXZhbEIyYnM2c2F4UlU0VXV4YVprRitYdlg0eEhwRHhRK3JnTFozZFlMZ0lo?=
 =?utf-8?B?RGJKMGxjNHB0T0VLSDBoZzNJZEsvWVMvaURuY0xEVHIrb2JZbmFQWXY1a1NF?=
 =?utf-8?B?dFJFZUpVT1RuNFJmSEZiSFJYWnNIQ1JiaUh3c1RNTjN4U1hKQ0pwL1ZOekMr?=
 =?utf-8?B?NzFxVnhEa3hGY09tYmt0NVhUbHZmWHVsTFI0bFdlblkvTVl5MzJua3pNYjRJ?=
 =?utf-8?B?WUoxOEw2YkdsNjFkbWlLVzBXODkzVjJESlNSZ1ZGZU9OWDJxMWJOdEE1QUh5?=
 =?utf-8?B?MDVQWjNwai9GSEZ4dFJwNy9UM0xLd0gycTRKRGJwSjROZHd2eU1ZYjdzVkYz?=
 =?utf-8?B?VGNGU0pjbCtiZ25WbGFIa3luU1FiVUNzMXNYd1h0OXprUlB4ZjZ4MHhpNGdu?=
 =?utf-8?B?UUxQeWVWeVFoa3c1RFVLL2h5dnhuRTh0N2RuN1hCWG9McWp4Zk41SEVReFZY?=
 =?utf-8?B?dzg3SXJKS3p3MElzYUF6M1Jvb2hVaG5WVzlrK1VxUFlvbE1zL0tQdFJKOFg5?=
 =?utf-8?B?UzJHb3dGUmVmVkNxQ2piT1dHRWI3Y05KZXQwZU5IK1BLazc2eGJ4bk1QWWhw?=
 =?utf-8?B?Y2RZN3p1aXNzck9jZmxKaU5PeGQ4YmJ1TUlUdjV4VVFJbloycGNFSm4ySDBX?=
 =?utf-8?B?WW5ZMzVnSXA4Y1VES3VXb1JCYjJtRkM0Q3VFS2hMQWM0Tk9nZkZDL011cmpo?=
 =?utf-8?B?QzRFKzEwU0RFeXNmQnhtWlRjNTZWT1A5WVZqMkFWNmV1Z0pudmx2SmhRTlMx?=
 =?utf-8?B?NmJUeWc4SlI5NlgzTzFtUDc4VHY4SUEzamZYYTN6R0U2UTB5a2I2REQ3Z3M5?=
 =?utf-8?B?N3I4U29LTjAyMUU2SFJLa1RLN2hvSWRNakg1cWppL01Vem1vMUF2ZDdqc01j?=
 =?utf-8?B?OThJMFc0VEdiU0FzS1c5bEIzYkxtOHlyNXNhS3RFMytRWTZMdUpvNDdLNG96?=
 =?utf-8?B?NUVrRitiSGQwR2Z0S2RSdSsyWWt2c2Y2bTkxbGxVa01KTTZkSjFOdFBIYXFC?=
 =?utf-8?B?blE4TTZseDlLN2g4Nm53dkdvMTNMSnlMcUhrVitQWVRPaUlKYmRzR3J5NE0v?=
 =?utf-8?B?RUNCSGV3MEE4am1YYzdjcURYY2h2aUlyam9FWGg1RS85dFlpQjZVZDFJc1Jr?=
 =?utf-8?B?eGdBV0YrUDFMM0w3OFEzeG9UeFN5ZFc3YXVjYmU4YUNCUXRzOWtVTXV3dWYw?=
 =?utf-8?B?WkhtN0lDM2hDMDdpRk10NTBmc0xVUzhqWkxKUk0yellicDZxYVJqQWZYNVBm?=
 =?utf-8?B?L2swakNSOXMyby8zQTN0OEtJRkpJcFFDL1grY3hLNVVMYko3TGpCeWtYTmRw?=
 =?utf-8?B?d0F6UStMQk9EMkdTczdjUkpmenZTaithaVZPVkhEOXJVMWVBMUtqTVlwS0do?=
 =?utf-8?B?azNBWGJ5YTI5cGNBb1VlbkxTdEN2Qk8rS1RjWEhrSXc4S3N5YU00NHFlK1Rs?=
 =?utf-8?B?dzQvRUhrdWtvWkloK1BWcTVIL0psRmM1cmd4NWxHc2E5ZHJUc0dRaXhmdmxY?=
 =?utf-8?B?NVFXdjZIbkZzTFg1NVcxRnBrbklSVmE4eklVR3FCV3g5d3JuRjlUQTVXRVJo?=
 =?utf-8?B?NThPajkwYTJ6aS9qRTc3ZWY4WEVPWnNKYk43a2wrdEJYSXhMcVAwYUZDR1lV?=
 =?utf-8?B?cUNSZjhrS3h3MGdhR1E5cCsrdEVSMmNjbm93Q3FKeDlDMERnTUJ5c0ZTZ1RD?=
 =?utf-8?B?T0ZvTVFhVkdzVXlsUFJLSUEvcUJUQVk3bE14bTdRbjIxWis2b1ZkRm5oUzJN?=
 =?utf-8?B?akpMRmdwM0pXWDFsaTgxZUc1WVFhYXpTcFUveHI0OXRhR2xaVjJvN05xWUhz?=
 =?utf-8?B?MVZ6bXRSOG1tWmM2VUZwa3VxNmwwY1BIcENSa3ZMV2ViOEhNNjh3aDlUQnhh?=
 =?utf-8?B?Qlo5ZmE5a2Fwd3hSak5ibndRVlR1KzdaY2ZhMW54Q1NaV0lQKzR1aHphRFMz?=
 =?utf-8?B?U0Zhci9vbDJHSmJxQ3pOSnk3Sld4c0NoUmtiY2E5T3pzK1h4dktoR2hseGtY?=
 =?utf-8?B?dmVUMThQVU4rL0hNbVlOVDEyQ0dBeEx1MXZ3UGRmaWR5dXZIQmVOQlhQU1B3?=
 =?utf-8?B?ZVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 51cd10ee-5865-4b13-f090-08dc6e7256ba
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB8718.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 08:47:34.2691
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vRiQCI2wIxxu/50cpyS+R726gZo01OWNWpVbWc+BW6BGLLlJMfDicwvzJox+n1ZEuhfHu4dg4QdK60Nz//Izz5XvhUuDpjRLTYOop+oVXsM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7313
X-OriginatorOrg: intel.com

From: Michal Schmidt <mschmidt@redhat.com>
Date: Mon,  6 May 2024 16:50:22 +0200

> Cast operation has a higher precedence than addition. The code here
> wants to zero the 2nd half of the 64-bit metadata, but due to a pointer
> arithmetic mistake, it writes the zero at offset 16 instead.
> 
> Just adding parentheses around "data + 4" would fix this, but I think
> this will be slightly better readable with array syntax.
> 
> I was unable to test this with tools/testing/selftests/bpf/vmtest.sh,
> because my glibc is newer than glibc in the provided VM image.
> So I just checked the difference in the compiled code.
> objdump -S tools/testing/selftests/bpf/xdp_do_redirect.test.o:
>   -	*((__u32 *)data) = 0x42; /* metadata test value */
>   +	((__u32 *)data)[0] = 0x42; /* metadata test value */
>         be7:	48 8d 85 30 fc ff ff 	lea    -0x3d0(%rbp),%rax
>         bee:	c7 00 42 00 00 00    	movl   $0x42,(%rax)
>   -	*((__u32 *)data + 4) = 0;
>   +	((__u32 *)data)[1] = 0;
>         bf4:	48 8d 85 30 fc ff ff 	lea    -0x3d0(%rbp),%rax
>   -     bfb:	48 83 c0 10          	add    $0x10,%rax
>   +     bfb:	48 83 c0 04          	add    $0x4,%rax
>         bff:	c7 00 00 00 00 00    	movl   $0x0,(%rax)
> 
> Fixes: 5640b6d89434 ("selftests/bpf: fix "metadata marker" getting overwritten by the netstack")
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> index 498d3bdaa4b0..bad0ea167be7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_do_redirect.c
> @@ -107,8 +107,8 @@ void test_xdp_do_redirect(void)
>  			    .attach_point = BPF_TC_INGRESS);
>  
>  	memcpy(&data[sizeof(__u64)], &pkt_udp, sizeof(pkt_udp));
> -	*((__u32 *)data) = 0x42; /* metadata test value */
> -	*((__u32 *)data + 4) = 0;
> +	((__u32 *)data)[0] = 0x42; /* metadata test value */
> +	((__u32 *)data)[1] = 0;

Uff sorry. I was a bit in hurry to fix BPF CI and did this braino :z
I know pointer arithms obviously :D
I'd give a Rev-by, but it was applied already. Thanks!

>  
>  	skel = test_xdp_do_redirect__open();
>  	if (!ASSERT_OK_PTR(skel, "skel"))

Thanks,
Olek