From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id AD410C48297 for ; Fri, 9 Feb 2024 18:11:44 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 298B96B0071; Fri, 9 Feb 2024 13:11:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 248C76B0075; Fri, 9 Feb 2024 13:11:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 112E96B0078; Fri, 9 Feb 2024 13:11:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 0148A6B0071 for ; Fri, 9 Feb 2024 13:11:43 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id B14D11C1B76 for ; Fri, 9 Feb 2024 18:11:43 +0000 (UTC) X-FDA: 81773058486.05.0CC5924 Received: from mail-qk1-f174.google.com (mail-qk1-f174.google.com [209.85.222.174]) by imf14.hostedemail.com (Postfix) with ESMTP id A6A5A100023 for ; Fri, 9 Feb 2024 18:11:40 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf14.hostedemail.com: domain of dcvernet@gmail.com designates 209.85.222.174 as permitted sender) smtp.mailfrom=dcvernet@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707502300; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Ujf1now2MFzpY/1fz97ZcaW/TJT9oGqy/4/GaeaHiHk=; b=pX0Bnlvjyg2/ZCmqffFgqeFHb20T1vvAkMf2Hcti54ldCodsiKhfSJcDwS6CCCNYtViSEE 8FSNBNNBkQBcRxyiKkG34N+sXVgFwPhqCXXi7y1h9yRuNQWLYQLEoyRNSOonSXLkZ5ZOVa QiMBpSVTd95QVyJUBiFrqxBq25WSgiI= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf14.hostedemail.com: domain of dcvernet@gmail.com designates 209.85.222.174 as permitted sender) smtp.mailfrom=dcvernet@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707502300; a=rsa-sha256; cv=none; b=0B0+u8eiJkB8Hdy0L/bhlt72Lx3zfx+SqC94JeELTPQdQf0R0aPWWinfLtouCn1XGTSJgI Mc7FRs9B40Uft4Z5+ihLpDitJ3T91MOvYxmOt1fvv/hFf5q6LK6+rcXlS7ihrXUG7dXUc4 IvhkleiF3bMdfJ1S/S1ZmSUmk63EVjQ= Received: by mail-qk1-f174.google.com with SMTP id af79cd13be357-783d4b3ad96so72492985a.3 for ; Fri, 09 Feb 2024 10:11:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707502300; x=1708107100; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Ujf1now2MFzpY/1fz97ZcaW/TJT9oGqy/4/GaeaHiHk=; b=shYiXptNKYPkehAskLaSbBnIW9wkTNpsEu7fW6j2xKOq0Ba9kZD6itOX8elVGmW4Aw 9CVIapeEM265e+i73eTYTBIS8ChWfL5cc9NyTOrg0oQisezZChbvpbmeIQ3mFvLTiPN6 DdTZS2LmLCtMW1slya2W9VM1pS8kAgYe3X9c1JKgvTrQlZlsZjKUeT756KQV5a5d9p5j 09WV/KrvLjrCBZ9voYFu8iiYCfIymiCteAdf6yavHyqGY0OFasahbzIFO+iB1mYK79v6 jO1Yb0WCRhJCdv+A4ygfEvOTutT4zxr0EwqIS/d/hVahwA8NeGY6eb0n4cUnzm8XMDAQ HQyg== X-Gm-Message-State: AOJu0YyCeMSSHLL3bQ0DVQxgycVRfJvuDuDhY4Fu+2AQvi7D3FtleKrL hJAeV92Jcvj0KeXMvzqL4Gt/fR9Nr8FwafTySIRi5R4c53404q0V X-Google-Smtp-Source: AGHT+IHOHD9AHcpdzJoBLNm/zSEbLSHecV2HlJYgAVRKKL8UC4rOxQjIWVMLD/Zljm9zTT789A8EsQ== X-Received: by 2002:a0c:f391:0:b0:68c:a9bd:d048 with SMTP id i17-20020a0cf391000000b0068ca9bdd048mr2442534qvk.28.1707502299691; Fri, 09 Feb 2024 10:11:39 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCUyrn7uiR1XBlfUDJWyM0bspvY50QCnkrsUDJTcv46ZvW0omUANVAPn0UhnjQFO/xeyX5zMML7l0dQJxyCV5BfR1Kk+jfnZLVqy6/dmmqHPQJTiLQuLao0yjWE6AZyma4Ymp7+fziQWwI8Sqc87eprCmeR45W8FF4i3sc8Z9LeG/Ji38KG+x+1f9oj59TmojXKqpRU9tUfFscPybrAl8gh4RomiqbOy5rPq2zPl5ldvV/NOZ+V4Gwig1EYa9biLChMeW6qK1n4hWb1Qc+MhHCDuHOg6C5+4ZcPrkMaVGj04NRQiEZHGLQG8JHzw6HSnOQ== Received: from maniforge.lan (c-24-1-27-177.hsd1.il.comcast.net. [24.1.27.177]) by smtp.gmail.com with ESMTPSA id nz10-20020a0562143a8a00b0068cbacf7327sm1029932qvb.107.2024.02.09.10.11.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 09 Feb 2024 10:11:39 -0800 (PST) Date: Fri, 9 Feb 2024 12:11:36 -0600 From: David Vernet To: Alexei Starovoitov Cc: bpf@vger.kernel.org, daniel@iogearbox.net, andrii@kernel.org, martin.lau@kernel.org, memxor@gmail.com, eddyz87@gmail.com, tj@kernel.org, brho@google.com, hannes@cmpxchg.org, linux-mm@kvack.org, kernel-team@fb.com Subject: Re: [PATCH bpf-next 02/16] bpf: Recognize '__map' suffix in kfunc arguments Message-ID: <20240209181136.GD975217@maniforge.lan> References: <20240206220441.38311-1-alexei.starovoitov@gmail.com> <20240206220441.38311-3-alexei.starovoitov@gmail.com> <20240209165745.GB975217@maniforge.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="N8uZMuqoGFctCgn7" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/2.2.12 (2023-09-09) X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: A6A5A100023 X-Stat-Signature: rm38r467azsbdq6z4mksk43h6h43mwkc X-HE-Tag: 1707502300-855500 X-HE-Meta: U2FsdGVkX198l6dwix6s6MK4jKv+FDQcvu2/aehOhAE4ciTtsYtt+AimFIS3TgXVND+ug3sLph9aQaanr1lXMLqUSqZWDo4yeV8uCvYOadj3MmlmenLKNmsRnOPS9ku+0CqRP6F6Ibcpbm56JwqhmcvO1R0oDLL9XBT6SBn9HjuHzmOaNSDU4cLNRgt4DDwJt3XWRJ6O+B2vVWDygITMo0OAcj2BbNKSrEeFSTNN58rBn8FdIc/KL92Klp6EsIZr6TlmUfmfOJ/JBRaWcGLrohaoXyTsNMC/A8J2aG6Fx5uAztTMdOCu12umabzgPLud3MU/7jylxCB0VMe2Asl9hkSonA6h0dqvaEMmMkJQGy/LsOYpEaLFNSdgDpfxo/AMMKldlQejhQBIAAbDhel6edslY8NyqWvF4ZYnn/DWPkgxmXAr18KUP1LuwE1LfRBP2V92OVbwycUCLH9BfdVCWHkSKdV3woJD9OYIkVfnXDQIyNGBCHaOxkGxvfTbrVg6Acux00cmxcs3tU+JYaF4LvC8RDI4daE0dgW7KYDrN93aGzvT4Y3Fy8N6+adS+BCE+uf+hcpl4/t84HiCyLt4KgdtX3Yx5lYDjlYcSqg0EbcW8CDPC3BAuj3aHiyUc90Az7t8WUuP5vapFq2UMfmye1bHKI9zKwRG+eDYX+s73d6pn/Y8fQuDsHfmWAhWE3hk0bQUCratW69V2Hi7K2rdUwvlPFglIC93Qjc/9BjbWguoSzfv7FJH5/GL0I3aBCkm7R/pFfGwUc2/vPLIAYC496oJVG2zwwkkupvLUhExKYvRfU39U+1a3c8VDXjFGxBsWIgTdmTxShxUbB9kE2Cbb25dmMn3tr1L4Rw0uPmTzXsr/0FivvRRwEqbRr06gyv/EaLF8Cnch318ZEX7gJc3Lm8ESlFjYT5Pkhmhxg34+Xz3drzbd6GwWby9YJclTwJLWrvW2qZ73i/nWIMjLZ3 AfhY5uA3 /NCcjdbJkgt6VKi20VYrMFPkt20xsrKwbxyKz2x0NqR9gNFZnEEjunuYVc0UUNLC/e6SEiFPe2NbZD4gDT7EwwmdYNA9TIxRDmdqfayjb+fGaq2DQyJq4hXvMg7f1wb2Ek9CETdQxO4I8Xt45W1qzKFuIfdAipnrnOqQJq9cKoGmQnUJxisKGh2/VntozSkQ69UmQ85wL2rGnt742J0+WTHdncZi8sLMcOg+sEyeqQ9U5u1/4p1EnH9vFXpA4cvd8QbN/LlREJybxqj5ZCI8IxKGtzVoJHGtnLJbKghb909Jh/PNry07Kn8damaR7QHsb6TJJ5TxW7ZjrSxXBhbt4dM9mVgiNR7HOC4r1REEMVtg2uIicrzcjAzqJSzC4zOxCbA2LLqFiYsyhnfttQJrBD8q45BvyQLX7ZzRobmekSVdcN5tJwtvXInQXDWvkZfj1UD1qtwi9ggzTes552cZ0q2tp8464hL5YOAKSVjjfR8ARdGaul3gXVxY1iYCwN3ilFSKnYcwoZ1pXa4GsiMQe2JlJgNa6VrVFYAToo8hueBN8aD9AKVzK8bwI1cEU40HCdpMb4DcbhiqQxwh3IeS5hKMIiTZlXLveYkcO+dDwqG79oqeCEn8/leGCwq79T9HD0ldP X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: --N8uZMuqoGFctCgn7 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 09, 2024 at 09:46:57AM -0800, Alexei Starovoitov wrote: > On Fri, Feb 09, 2024 at 10:57:45AM -0600, David Vernet wrote: > > On Tue, Feb 06, 2024 at 02:04:27PM -0800, Alexei Starovoitov wrote: > > > From: Alexei Starovoitov > > >=20 > > > Recognize 'void *p__map' kfunc argument as 'struct bpf_map *p__map'. > > > It allows kfunc to have 'void *' argument for maps, since bpf progs > > > will call them as: > > > struct { > > > __uint(type, BPF_MAP_TYPE_ARENA); > > > ... > > > } arena SEC(".maps"); > > >=20 > > > bpf_kfunc_with_map(... &arena ...); > > >=20 > > > Underneath libbpf will load CONST_PTR_TO_MAP into the register via ld= _imm64 insn. > > > If kfunc was defined with 'struct bpf_map *' it would pass > > > the verifier, but bpf prog would need to use '(void *)&arena'. > > > Which is not clean. > > >=20 > > > Signed-off-by: Alexei Starovoitov > > > --- > > > kernel/bpf/verifier.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > >=20 > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index d9c2dbb3939f..db569ce89fb1 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct= btf *btf, const struct btf_param *a > > > return __kfunc_param_match_suffix(btf, arg, "__ign"); > > > } > > > =20 > > > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf= _param *arg) > > > +{ > > > + return __kfunc_param_match_suffix(btf, arg, "__map"); > > > +} > > > + > > > static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const stru= ct btf_param *arg) > > > { > > > return __kfunc_param_match_suffix(btf, arg, "__alloc"); > > > @@ -11064,7 +11069,7 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_en= v *env, > > > return KF_ARG_PTR_TO_CONST_STR; > > > =20 > > > if ((base_type(reg->type) =3D=3D PTR_TO_BTF_ID || reg2btf_ids[base_= type(reg->type)])) { > > > - if (!btf_type_is_struct(ref_t)) { > > > + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t)) { > > > verbose(env, "kernel function %s args#%d pointer type %s %s is no= t supported\n", > > > meta->func_name, argno, btf_type_str(ref_t), ref_tname); > > > return -EINVAL; > > > @@ -11660,6 +11665,13 @@ static int check_kfunc_args(struct bpf_verif= ier_env *env, struct bpf_kfunc_call_ > > > if (kf_arg_type < 0) > > > return kf_arg_type; > > > =20 > > > + if (is_kfunc_arg_map(btf, &args[i])) { > > > + /* If argument has '__map' suffix expect 'struct bpf_map *' */ > > > + ref_id =3D *reg2btf_ids[CONST_PTR_TO_MAP]; > > > + ref_t =3D btf_type_by_id(btf_vmlinux, ref_id); > > > + ref_tname =3D btf_name_by_offset(btf, ref_t->name_off); > > > + } > >=20 > > This is fine, but given that this should only apply to KF_ARG_PTR_TO_BT= F_ID, > > this seems a bit cleaner, wdyt? > >=20 > > index ddaf09db1175..998da8b302ac 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -10741,6 +10741,11 @@ static bool is_kfunc_arg_ignore(const struct b= tf *btf, const struct btf_param *a > > return __kfunc_param_match_suffix(btf, arg, "__ign"); > > } > >=20 > > +static bool is_kfunc_arg_map(const struct btf *btf, const struct btf_p= aram *arg) > > +{ > > + return __kfunc_param_match_suffix(btf, arg, "__map"); > > +} > > + > > static bool is_kfunc_arg_alloc_obj(const struct btf *btf, const struct= btf_param *arg) > > { > > return __kfunc_param_match_suffix(btf, arg, "__alloc"); > > @@ -10910,6 +10915,7 @@ enum kfunc_ptr_arg_type { > > KF_ARG_PTR_TO_RB_NODE, > > KF_ARG_PTR_TO_NULL, > > KF_ARG_PTR_TO_CONST_STR, > > + KF_ARG_PTR_TO_MAP, /* pointer to a struct bpf_map */ > > }; > >=20 > > enum special_kfunc_type { > > @@ -11064,12 +11070,12 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_en= v *env, > > return KF_ARG_PTR_TO_CONST_STR; > >=20 > > if ((base_type(reg->type) =3D=3D PTR_TO_BTF_ID || reg2btf_ids[b= ase_type(reg->type)])) { > > - if (!btf_type_is_struct(ref_t)) { > > + if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref= _t)) { > > verbose(env, "kernel function %s args#%d pointe= r type %s %s is not supported\n", > > meta->func_name, argno, btf_type_str(re= f_t), ref_tname); > > return -EINVAL; > > } > > - return KF_ARG_PTR_TO_BTF_ID; > > + return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_A= RG_PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID; >=20 > Makes sense, but then should I add the following on top: >=20 > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index e970d9fd7f32..b524dc168023 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -11088,13 +11088,16 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env = *env, > if (is_kfunc_arg_const_str(meta->btf, &args[argno])) > return KF_ARG_PTR_TO_CONST_STR; >=20 > + if (is_kfunc_arg_map(meta->btf, &args[argno])) > + return KF_ARG_PTR_TO_MAP; > + Yeah, it's probably cleaner to pull it out of that block, which is already a bit of a mess. Only thing is that it doesn't make sense to invoke is_kfunc_arg_map() on something that doesn't have base_type(reg->type) =3D=3D CONST_PTR_TO_MAP right? We sort of had that covered in the below block beacuse of the reg2btf_ids[base_type(reg->type)] check, but even then it was kind of sketchy because we could have base_type(reg->type) =3D=3D PTR_TO_BTF_ID or some other base_type with a nonzero btf ID and still treat it as a KF_ARG_PTR_TO_MAP depending on how the kfunc was named. So maybe something like this would be yet another improvement on top of both proposals that would avoid any weird edge cases or confusion on the part of the kfunc author? + if (is_kfunc_arg_map(meta->btf, &args[argno])) { + if (base_type(reg->type) !=3D CONST_PTR_TO_MAP) { + verbose(env, "kernel function %s map arg#%d %s reg was no= t type %s\n", + meta->func_name, argno, ref_name, reg_type_str(en= v, CONST_PTR_TO_MAP)); + return -EINVAL; + } + return KF_ARG_PTR_TO_MAP; + } + > if ((base_type(reg->type) =3D=3D PTR_TO_BTF_ID || reg2btf_ids[bas= e_type(reg->type)])) { > - if (!btf_type_is_struct(ref_t) && !btf_type_is_void(ref_t= )) { > + if (!btf_type_is_struct(ref_t)) { > verbose(env, "kernel function %s args#%d pointer = type %s %s is not supported\n", > meta->func_name, argno, btf_type_str(ref_= t), ref_tname); > return -EINVAL; > } > - return is_kfunc_arg_map(meta->btf, &args[argno]) ? KF_ARG= _PTR_TO_MAP : KF_ARG_PTR_TO_BTF_ID; > + return KF_ARG_PTR_TO_BTF_ID; > } >=20 > ? >=20 --N8uZMuqoGFctCgn7 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEARYKAB0WIQRBxU1So5MTLwphjdFZ5LhpZcTzZAUCZcZq2AAKCRBZ5LhpZcTz ZKUOAQDRRc7irh9+fJhc4DTqsgWwRwTuvsgt26G3PUb13rdO7wEAshz5Sa+s20HU XAG9ejn6wbxerx5gVejyVaq3IY874ww= =FtWy -----END PGP SIGNATURE----- --N8uZMuqoGFctCgn7--