Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b516b0dc-df40-8dae-8b3c-16048d2bfdb3@fb.com>
Date:   Sat, 16 Jul 2022 19:03:54 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.11.0
Subject: Re: [PATCH bpf-next 1/5] bpf: Unify memory address casting operation
 style
Content-Language: en-US
To:     Pu Lehui <pulehui@huawei.com>, bpf@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Quentin Monnet <quentin@isovalent.com>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Jean-Philippe Brucker <jean-philippe@linaro.org>
References: <20220716125108.1011206-1-pulehui@huawei.com>
 <20220716125108.1011206-2-pulehui@huawei.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <20220716125108.1011206-2-pulehui@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a0tkZFJzMUlLMTFkdVN6Qkh5TG15Z0dtS1Z2RGF2bkVXYmdtWDc0Rmhhc0pH?=
 =?utf-8?B?TGZWUjJaQkRGK2E2aUlNbi9keTZpVEIvUkJCNENlNjBPVHRjcExzeTFpSnFi?=
 =?utf-8?B?b0hKYmJSNWhBMVpPT1o3a3lMMTBVOStKSDYrbXNMOHVOK2hGZkhud2RkMGgw?=
 =?utf-8?B?aytxeDltR3RONVFWUU5EMnJxdVFybXNjMU9lbnNEelZSckRkdnZ4aitpR3Fp?=
 =?utf-8?B?V1I3d1ZZblZRZGh0ZUhpV0VoY2I5TEthbmZ6T3ZsMXNnVmtlYlRIL3gycnZX?=
 =?utf-8?B?eVlNQWJjV3hEMkZ0Y1QrdGpuVUJsbW9PRFFUT2NmRjgwS2pFNEpNVWl5SG1G?=
 =?utf-8?B?K09tUENTYnc5QzhVcU5YUTZLZHNyR0Rqc3h4c3hSdlhMR1VQWjVNSEZQb1JH?=
 =?utf-8?B?aEpNRi9jNDR4UVpyL1pLU1FUWitQUlA5NEZzaUREakYrRDBHbGlHTjI2bXNM?=
 =?utf-8?B?Wkd4QW0vT05YVk9TQ3l4MmdsUVloN2N2aldkdVZPV1BESmd1UVo3YnJ3MHBW?=
 =?utf-8?B?UEhYdjdPbjdrcGs3bWRVT1VxN0o3ZG9pUEJzbjczaFArc3JUd3lkVkhWaVBj?=
 =?utf-8?B?NkFERVhrU3hndkZ3RjE4VktFVUZmMVloRDI5cnNMZnNDYXVEdk8rd2hkUHlz?=
 =?utf-8?B?byt0SEJZWE16eWFGb2RuUHFpdlNPTlBGc3RDa0dwUVMySHJsVDYxSjVMOStR?=
 =?utf-8?B?R0thNTByWW52SHN3NVBXQTR6MDg5NEh0enBpRXVMNWtHSFBvY2E1S0NNMlZL?=
 =?utf-8?B?SzF0V0VNSDN0YXZUQm1JaFNhTi9ubnlrRTdHeDRtV1lYMVpBaXV2b1BWVVoy?=
 =?utf-8?B?ZkdmYXMrNGRRdG9zdlhFWmdBcUNZanBUUW5wTFBHZm9QaGlValZ4cW4vUTRt?=
 =?utf-8?B?aVRtejRYNFp1YTVvQ1RROGxNTGVScnJiZ01tTXB5alZaMlBZczU3T3R6V1hl?=
 =?utf-8?B?citZWmZBRDVQU0pma3JaYzNHNGt0YXFLZGdKTVluOHpuQ2lOSWJNSHluN081?=
 =?utf-8?B?dlI5QUVjTkMwbjlpaHZDaER4OFltOWV3d29scTVVZFZ2bkpaekxnTUlLYXgv?=
 =?utf-8?B?WGZad1BEZjF6NllCbGh0b2JEbnVOQWIvOWR5ZXVmSjkzNCs1WWpyTGJwQ2th?=
 =?utf-8?B?K1A0bFpId3Z4VnpFbHpSMDNpOUUvV0J2aWtoV2U4QzI5L1JNYWpBWllxTGVp?=
 =?utf-8?B?dG1BRUJEMHgvMEt0RkkvK2tLZysvaXNGSVd6MVhXR0xCQ1lsTUp2L09WNktE?=
 =?utf-8?B?RGVQTE5XYVZGT2RxNC9ZazU4R3pCZk1MUFFQSk5qQXdJMDN6Rzc1dFRBMVZU?=
 =?utf-8?B?ZkZ1cnJQR0FmS1dSQnNhNU5meUVrWG93c3FsdURHOVlNN0xFQ3ZCNDZGUXd0?=
 =?utf-8?B?MGZpM3hlbERFWExJVnFZSW1JYnlGR0c4d2t2U2NUNVFXZm84OHpROWw2a3k3?=
 =?utf-8?B?QndoTHJEdk1Zc3hPQUVWVHFhK29ZOHQyZTJtalE5OUxmSy90TXN0c3dEampH?=
 =?utf-8?B?WW9wMlhXQTUxN1o0azBkOHNJUC9yUyt2SkFQTGVYaitUcGFuZzY0QXdQUHpS?=
 =?utf-8?B?YkVRYVlWY0E5S1VZcHh1c2I5dXo5TDhXYnlyOWxJekhoN0tINE04QVlMVmlH?=
 =?utf-8?B?NmVFUWZhYkExT21YMkxUZC9ITXZaUjF1ZmNJdWVTdVczWXN5eGY2SXUwWnhK?=
 =?utf-8?B?SWdGaS90MENlNDZKMVVoTm13NDhZVnpGM3p6cnZRTVVPSGRZU1A5ZFB0Rk1S?=
 =?utf-8?B?bjNHVjRvMmN2RHIvN3pSVVE1Q1FHSUY0Ym9zMlQvc2ppeFhDdzNDTjZERVFr?=
 =?utf-8?B?bnM4SGdFckVVVnA4OFR5RVRodnNUL0tIeFRRaVpnVHBqYVFIckVVSG1obGZE?=
 =?utf-8?B?SEgwcmNlN0szQUd5dkdJMzVlb1VOUmZFMWlDalhqdGdwRU5RSmpONFJTb3Zx?=
 =?utf-8?B?emFGeFBZcHFnM0p1SHdYUU5tcEMzQlRZakQyVFdTL3VGcXJYVzRlWmljY2c3?=
 =?utf-8?B?SnAxUlpEOTY2a0h5QjhKSTVqTklhTHNRMTZ2K3pzZnFIdXU5SWlxSVE2MXJP?=
 =?utf-8?B?VWZaZFlsS3dtYWNDbTBKNGYyYVh3bi9ZT3FNYkRHR1lScUwrVW5wRGpoOEJv?=
 =?utf-8?B?RjkyNkFLaGhZNmJ4Zm5LbXI3OWpHeHdJL2psaG5Ja2pxK0FySHRkemJCRGpE?=
 =?utf-8?B?MGc9PQ==?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a818e143-4fc1-4f80-1f3f-08da67989baa
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jul 2022 02:03:57.1747
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cpxPW7Wv97thsvs6vcweac+ZhfCL13Ki/4CaSrmZFYmnhrLp5W2SVTMB6APj5JSe
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR15MB1404
X-Proofpoint-ORIG-GUID: hpjKZI1zFUN8A1rNfZzp_U3ry_m7sqiA
X-Proofpoint-GUID: hpjKZI1zFUN8A1rNfZzp_U3ry_m7sqiA
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-16_21,2022-07-15_01,2022-06-22_01
X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 7/16/22 5:51 AM, Pu Lehui wrote:
> Memory addresses are conceptually unsigned, (unsigned long) casting
> makes more sense, so let's make a change for conceptual uniformity
> and there is no functional change.
> 
> Signed-off-by: Pu Lehui <pulehui@huawei.com>

Only a few in bpf system, agree that we can do the change so in
the future we can recommend 'unsigned long' vs. 'long' casting
based on existing code base.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   kernel/bpf/core.c     | 2 +-
>   kernel/bpf/helpers.c  | 6 +++---
>   kernel/bpf/syscall.c  | 2 +-
>   kernel/bpf/verifier.c | 6 +++---
>   4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index cfb8a50a9f12..e14b399dd408 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1954,7 +1954,7 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
>   		CONT;							\
>   	LDX_PROBE_MEM_##SIZEOP:						\
>   		bpf_probe_read_kernel(&DST, sizeof(SIZE),		\
> -				      (const void *)(long) (SRC + insn->off));	\
> +				      (const void *)(unsigned long) (SRC + insn->off));	\
>   		DST = *((SIZE *)&DST);					\
>   		CONT;
>   
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index a1c84d256f83..92c01dd007a6 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -903,7 +903,7 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
>   					err = snprintf(tmp_buf,
>   						       (tmp_buf_end - tmp_buf),
>   						       "%pB",
> -						       (void *)(long)raw_args[num_spec]);
> +						       (void *)(unsigned long)raw_args[num_spec]);
>   					tmp_buf += (err + 1);
>   				}
>   
> @@ -929,7 +929,7 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
>   				goto out;
>   			}
>   
> -			unsafe_ptr = (char *)(long)raw_args[num_spec];
> +			unsafe_ptr = (char *)(unsigned long)raw_args[num_spec];
>   			err = copy_from_kernel_nofault(cur_ip, unsafe_ptr,
>   						       sizeof_cur_ip);
>   			if (err < 0)
> @@ -966,7 +966,7 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
>   				goto out;
>   			}
>   
> -			unsafe_ptr = (char *)(long)raw_args[num_spec];
> +			unsafe_ptr = (char *)(unsigned long)raw_args[num_spec];
>   			err = bpf_trace_copy_string(tmp_buf, unsafe_ptr,
>   						    fmt_ptype,
>   						    tmp_buf_end - tmp_buf);
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 83c7136c5788..d1380473e620 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5108,7 +5108,7 @@ BPF_CALL_3(bpf_sys_bpf, int, cmd, union bpf_attr *, attr, u32, attr_size)
>   			bpf_prog_put(prog);
>   			return -EBUSY;
>   		}
> -		attr->test.retval = bpf_prog_run(prog, (void *) (long) attr->test.ctx_in);
> +		attr->test.retval = bpf_prog_run(prog, (void *) (unsigned long) attr->test.ctx_in);
>   		__bpf_prog_exit_sleepable(prog, 0 /* bpf_prog_run does runtime stats */, &run_ctx);
>   		bpf_prog_put(prog);
>   		return 0;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..d91f17598833 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4445,7 +4445,7 @@ static int bpf_map_direct_read(struct bpf_map *map, int off, int size, u64 *val)
>   	err = map->ops->map_direct_value_addr(map, &addr, off);
>   	if (err)
>   		return err;
> -	ptr = (void *)(long)addr + off;
> +	ptr = (void *)(unsigned long)addr + off;
>   
>   	switch (size) {
>   	case sizeof(u8):
> @@ -6113,7 +6113,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
>   			return err;
>   		}
>   
> -		str_ptr = (char *)(long)(map_addr);
> +		str_ptr = (char *)(unsigned long)(map_addr);
>   		if (!strnchr(str_ptr + map_off, map->value_size - map_off, 0)) {
>   			verbose(env, "string is not zero-terminated\n");
>   			return -EINVAL;
> @@ -7099,7 +7099,7 @@ static int check_bpf_snprintf_call(struct bpf_verifier_env *env,
>   		verbose(env, "verifier bug\n");
>   		return -EFAULT;
>   	}
> -	fmt = (char *)(long)fmt_addr + fmt_map_off;
> +	fmt = (char *)(unsigned long)fmt_addr + fmt_map_off;
>   
>   	/* We are also guaranteed that fmt+fmt_map_off is NULL terminated, we
>   	 * can focus on validating the format specifiers.