Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <2a722851-e99f-176e-0f0b-4c32854f762d@fb.com>
Date:   Tue, 17 May 2022 00:08:28 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.9.0
Subject: Re: [PATCH bpf v2 2/4] bpf_trace: support 32-bit kernels in
 bpf_kprobe_multi_link_attach
Content-Language: en-US
To:     Eugene Syromiatnikov <esyr@redhat.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>
Cc:     Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, netdev@vger.kernel.org,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
        Shuah Khan <shuah@kernel.org>, linux-kselftest@vger.kernel.org
References: <20220516230506.GA25374@asgard.redhat.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <20220516230506.GA25374@asgard.redhat.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?ZmwvcmZCR28zSCtIcHEvckR1M1ZFQmx3eDZMS05kTk01dmh4NUhQeHBTQUVx?=
 =?utf-8?B?b1N4a0ZoS3NUazdaaHpDOHpqUTFZQjRqeGp1ODdqK3hoK2ZEQ0RTcHlmUU44?=
 =?utf-8?B?UW5qV1lGaWFHelk4MEJYSnRod1Awem5GRGI1SXlnZWlRdlFnaFVxTHF0cDR3?=
 =?utf-8?B?Z3hLZkdUbkFjS2tSSXFBS21BcVk3WTlrTmxiTDBCeTFKak4xY2xyaDBlUUY4?=
 =?utf-8?B?SllHMWkxRjFxazlxcCsrbS9CaGlKc251SnZNbGlXNG1vQ2JyL0x2UmQveG9t?=
 =?utf-8?B?VWM4SmYza0t4TThtZTM1YXVycXkvU3Y2VmFKKzZFb0xqeDBVYm10ZkFaN2RR?=
 =?utf-8?B?YWtzR25MbVd4ZUQ3UmNRMXMza0V1cVlRbHVTN1JhM3IrTW1zbk1STGo3NVJr?=
 =?utf-8?B?RUNCYWdZdFExeTFzamlucE1oQmVqQTBiVGJvUVVPZ3JWVUI4Um1ZbHZiU1k5?=
 =?utf-8?B?UWpUdlpSZTZXd0dPQ0hBRnZCeXhIZ0crTnQxMFlBWnRvYUpoejhQL2M4d3c2?=
 =?utf-8?B?SUorWEhtYkFDUFpQOHVIN254UmVkR1ZQcTd6ME5LbkhCUkErK1dUeThhb2cv?=
 =?utf-8?B?NE8rdkhWaDVSTElNS0orcEdKK3hiZXE1SzdiMnNTQVJ3elg4WXlxVVpESk1W?=
 =?utf-8?B?aUo4enYyMzI1cTYzMFRIbjJpR1hOdWFVRlZtOEU5MkZUVndKaWdFTVQwR2Jw?=
 =?utf-8?B?S3BsT2RlZTRrRHg1ME94VTRpRWpZclBvbTBORVgrL0IvYm9CVVlXUnpiL3FC?=
 =?utf-8?B?aWtnMUQrbDJMWFR6OTZXaXErdTFFOU03WGhkQ3JVTkYySEFUVW1PejBBUGlN?=
 =?utf-8?B?VUdTcmVQaUhpbCs5UnRzS29jR1JPWnd1eWsrUllxcnNqSGprYVVXK1g4dEJ6?=
 =?utf-8?B?UlVjQnRlMlkvTnlEeFErdGRRbzY3YnZNZ3A0bkRURHRpbWNpa1hhSXZvdHYr?=
 =?utf-8?B?NVRnQXpUNk5xRlNUSmlBWmpXQ0pwVzFvM0tMUTJnN0lSTEdUUkZIcmNYRC9X?=
 =?utf-8?B?Tk1TQXE1TDdSY2EveHFkOGg3WkkxU0duMUJZNEk1UFRrRk5mYm04ZHYwZE5B?=
 =?utf-8?B?N2VuYkFxNjJzTVN3bC85VEZJcTlIRWxHS0x0QXpDN041Y0pyTkZWRzJpM0lX?=
 =?utf-8?B?UmwvT1J6ekZJMUFadzF2WEpyVDh2NStpY2lhd3U5ZEtBZ0pSTGxSOThhTlhj?=
 =?utf-8?B?N1FUUWJVVE8ySFJpVm90MlRlTmNXYm85eFdBUDZiL0J0bHVrdmF2MXp6VDVP?=
 =?utf-8?B?V0lKemMyejV5NEhkck14VWVUSkQ3SXRMOGlTeEtWZlp2RTlBZDFHcUY4ejhG?=
 =?utf-8?B?Zm1jN21qR0xKeTVpWWNTVlNzSG9pNDgrSnI5MmRTMmFlL0tlQjl1bnBud0FB?=
 =?utf-8?B?ME01NElidndxOXBSQjdDN3dqQVJVTHd2TzI3aGJlaG9Cd2pCWklnc1h6cXNa?=
 =?utf-8?B?ZGR5bTFFUjdtU1BhZHVCVlZneDJoNGQ2a2lEeVB6NXA2TndwTUdPK1dEcnJI?=
 =?utf-8?B?d2NyUFlqcVNmdFFXUWlmWnlrTmJIZXRMYWJnQ3dOd2oyVUhKeis0ZWJQWWo4?=
 =?utf-8?B?NzVONUdRWVhSWkpBS0IyWDJOWjRtcWxaNG9HVlRlVU94OExxZzk3WHdYa0p1?=
 =?utf-8?B?Y2lmRWNQenM5L0xLM1V4SFJhUGJvellqUERWdjNEZExRTmVsSXR1MWxLWVQr?=
 =?utf-8?B?YXRwYnpscG1TckpBS2FsVENBcVVRQ0VpS3prRzJtUjZuSk5UaHdHaVg4Qnh6?=
 =?utf-8?B?Y1VocEJNZDJUVmJxV0duc0tQa01QQnhWeWlRTXRNWGI1bWk0V2FSTFVZb2RQ?=
 =?utf-8?B?UjdJTGVKT05tM2FMZ2RQdlNLamRBYWtLR0NoT043N2NYMVg1NUhocENIV0N1?=
 =?utf-8?B?bFBJZ3NYZmJuQjFJQzBqVktyRzhlL1p4TnJWL0gyYzhrWFhvbkUraExVRjFp?=
 =?utf-8?B?TzNMK1BhUUg4Vm0rM1VDOWtSUEZ0YmJPZG84c3dvRVRwQnZZUkY5SStCcity?=
 =?utf-8?B?YjBFQzRZQW1uNlRmOGl4SFhCYzJtRmkwcFdMdk0vWS9BTWtpKzdod2FKbjEx?=
 =?utf-8?B?UnJGc0xDMUVQd1hBSk9rL25hY1ZJTkFQOUlnZ0d4YURESmFhc1prTUh1UGtl?=
 =?utf-8?B?U1luWEVFUnc4L3lqTEtmMkk4cDc2cHhxcjRzeitHalBCS1ZsaE9HeGd6T1lQ?=
 =?utf-8?B?dmdTUjRkbmhvMjhpdGJRa0R3b09MeGRiMTBaMVRHMWxOMXlMZndSSUFmLzZY?=
 =?utf-8?B?SWxUN0Q1NWJyUEc2Y2E3cDZyZnpkMUJmOFJob2gxVENKVjliUTFSNFByUU1x?=
 =?utf-8?B?QU9JdFpiRnlOS2ZhZndoNTJuMTFwTUdVeXFlTExhU1ExZWRrekMrcFFBYisv?=
 =?utf-8?Q?7DBcX2ilpsGGnGt0=3D?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0e54c73f-ad2d-4fb9-39c5-08da37d40ca1
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2022 07:08:31.2083
 (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: 051x/ctxOjPs6DxX6KqFBZOnFIcJipE3rIJ6vBfGRXkfmUPthYDZLOk8uFd3Ah24
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR15MB5409
X-Proofpoint-GUID: KeZfh98U5TKsV-beGOpLlKQ-A7n71dkk
X-Proofpoint-ORIG-GUID: KeZfh98U5TKsV-beGOpLlKQ-A7n71dkk
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.858,Hydra:6.0.486,FMLib:17.11.64.514
 definitions=2022-05-17_01,2022-05-16_02,2022-02-23_01
X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 5/16/22 4:05 PM, Eugene Syromiatnikov wrote:
> It seems that there is no reason not to support 32-bit architectures;
> doing so requires a bit of rework with respect to cookies handling,
> however, as the current code implicitly assumes
> that sizeof(long) == sizeof(u64).
> 
> Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
> ---
>   kernel/trace/bpf_trace.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index f1d4e68..bf5bcfb 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2406,16 +2406,12 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>   	struct bpf_link_primer link_primer;
>   	void __user *ucookies;
>   	unsigned long *addrs;
> -	u32 flags, cnt, size;
> +	u32 flags, cnt, size, cookies_size;
>   	void __user *uaddrs;
>   	u64 *cookies = NULL;
>   	void __user *usyms;
>   	int err;
>   
> -	/* no support for 32bit archs yet */
> -	if (sizeof(u64) != sizeof(void *))
> -		return -EOPNOTSUPP;
> -
>   	if (prog->expected_attach_type != BPF_TRACE_KPROBE_MULTI)
>   		return -EINVAL;
>   
> @@ -2425,6 +2421,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>   
>   	uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs);
>   	usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms);
> +	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
>   	if (!!uaddrs == !!usyms)
>   		return -EINVAL;
>   
> @@ -2432,8 +2429,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>   	if (!cnt)
>   		return -EINVAL;
>   
> -	if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size))
> +	if (check_mul_overflow(cnt, (u32)sizeof(*addrs), &size) ||
> +	    (ucookies &&
> +	     check_mul_overflow(cnt, (u32)sizeof(*cookies), &cookies_size))) {
>   		return -EOVERFLOW;
> +	}
>   	size = cnt * sizeof(*addrs);

size has been calculated, no need to calculate again.

>   	addrs = kvmalloc(size, GFP_KERNEL);
>   	if (!addrs)
> @@ -2450,14 +2450,14 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>   			goto error;
>   	}
>   
> -	ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
>   	if (ucookies) {
> -		cookies = kvmalloc(size, GFP_KERNEL);
> +		cookies_size = cnt * sizeof(*cookies);

same for cookies_size.

> +		cookies = kvmalloc(cookies_size, GFP_KERNEL);
>   		if (!cookies) {
>   			err = -ENOMEM;
>   			goto error;
>   		}
> -		if (copy_from_user(cookies, ucookies, size)) {
> +		if (copy_from_user(cookies, ucookies, cookies_size)) {
>   			err = -EFAULT;
>   			goto error;
>   		}