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: <01ade45b-8ca6-d584-199b-a06778038356@meta.com>
Date:   Mon, 12 Dec 2022 10:03:31 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.5.1
Subject: Re: [PATCH 2/5] Replace invocation of weak PRNG in kernel/bpf/core.c
Content-Language: en-US
To:     david.keisarschm@mail.huji.ac.il,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        John Fastabend <john.fastabend@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Cc:     aksecurity@gmail.com, ilay.bahat1@gmail.com, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
References: <cover.1670778651.git.david.keisarschm@mail.huji.ac.il>
 <7c16cafe96c47ff5234fbb980df9d3e3d48a0296.1670778652.git.david.keisarschm@mail.huji.ac.il>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <7c16cafe96c47ff5234fbb980df9d3e3d48a0296.1670778652.git.david.keisarschm@mail.huji.ac.il>
Content-Type: text/plain; charset=UTF-8; format=flowed
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZGVvVHhpKzJSa01nT1NZc01WdlhxSm5vSkd2M3ZtcGJTa3g2b1U2S1g0SUJ6?=
 =?utf-8?B?Q21YVXpoWWhKYVM1RzA0T3ZNSi9qd3ZCVy9id21jZms2MzJwdVo2TE1OUFN5?=
 =?utf-8?B?NVBneEJnT1d0Wk1FN1hWZjRPbWllUDZ4c1g2ZkRGRGt4SnRwQStwdm9tRG4v?=
 =?utf-8?B?aFNyeTBHeXZuUGpGQ3luanV2RktQNm5TSWVEZUFuSmtvcGZFMzk4ZFhLaXdS?=
 =?utf-8?B?cHF0SkMrcE05b1Q3T1F2TmJ1K3I1Qm9yZWR6VVVtQjhLdmdwaTFDQ1ZKMWhr?=
 =?utf-8?B?bUhvYWtTbkc0K2JLZmk4NFpRaTRUbnBuQk5BUXU0WDF0cTJKOExsb2NzL3ZF?=
 =?utf-8?B?YlNBcWNrZGhDcU05L3d2MXl0R3FUUElpNURpcnp5VmtOdm83WUJuK1V1bUxi?=
 =?utf-8?B?NTVKNHJjMGJQQkpqSFduZ3ZxeVJRbGtObjN2T0NmTWowaHcxSmt5NzBRL3A1?=
 =?utf-8?B?REFUVDE1akxONnMveFZNWmtFeUxIbnUyc3h1UkkxQWx1SWtBc3d4cXkrY2tN?=
 =?utf-8?B?dDVmRGR3OUJ0MFVsR1k4dDBKRFNOYlhJUXpieExzdWlBc2c3SUVFTFNjdWZk?=
 =?utf-8?B?bVVDTnhoSHVRS000NVB4ckt4YmxnRXFXYXJCN2VvN2Y3Wk5DVGJQa0prWkox?=
 =?utf-8?B?ejdJdTBTT3ozWm5NaHFBTGxLdktPSlhRMjVZMEc4OVRoUE43d1FQRUZlNjdo?=
 =?utf-8?B?VU45VFlRT0JDRFdLTldveXhtZGpndXRydVV0WVZzdUs1bmphVEZOV3ZaZitl?=
 =?utf-8?B?ZFE4K2V3Z3V2UHZXd3pvL2tDUGsxV2tOOHVRWlBTaGtRWHR2cVB0Vno0L25m?=
 =?utf-8?B?SjFKNEs5bVJVaGJSWjZVYnlEWVRMMUp2SmZsbDFKU2xkeGRVOE1FOXdGY0xl?=
 =?utf-8?B?SFQ3d2V1S3l2VjVEV1VCUDZ1ZEdSUElQV29CVS9hSjlGd0tWR1RKVTdtVHVN?=
 =?utf-8?B?cmk2dVhDbzMxMVRxaUtKeGFGRUd3ZVp3S1lkUStSQ2FIYWxtQmtCcjhxZVFF?=
 =?utf-8?B?U05MT01WalhFK3V3MUlvbXJJdndXSWkvanhCZVVsRmRaemUzU3FxS2ZISy9x?=
 =?utf-8?B?MFVwdDFoYWFXUkJDVjVUQUIxalp4aisyQS9HS1pDUWZWSWI1clRLQ3BtUFRX?=
 =?utf-8?B?a1VRNWFwcU91T2drU2xyRld3eW84Rnp1Yi9tWFJOT01MNERlS1U0VkFrd1px?=
 =?utf-8?B?YVJzSlhiTHUrQ0Y4OGRKRG9TQytzQVJnSVI1TWszdWJGUnVNSm9nSVYrQ2xa?=
 =?utf-8?B?U3NsRXJQK3UvR2xRb2FYTTk1bWloUGEvL0dUNnlUOUN5c21hWjlVZUFYOGpV?=
 =?utf-8?B?TnJnTnQwNXNYcktOY0VDWXYxUGZqc1k5WjV0Z2pVNUtYYUx5cTRHenRyY0dM?=
 =?utf-8?B?VnQ1Q3VPam1OSy9xOHV5YkRlakU5ZXFnaTB3RmFVbkt6Z1d0N2pYdVBwcVJF?=
 =?utf-8?B?ZnFYL0tqcDJvcDE1eVovUHIzR3VhdU5PWVdKWlY2QlpqbTd2VVZtKzlNS3Bh?=
 =?utf-8?B?Z3Jzd1NGbGhodDNoSkpweXI1SWJjVGRtYUplYUxaUU5aeGtPYVpkc1I1SWEy?=
 =?utf-8?B?bGpGczY5YWtNRTJyQW1DK2pYYXVkQ3VCNTVjQUVqZWhBZmgxcTJPaVJmRS9C?=
 =?utf-8?B?cHlhL0VpQmV2bGRmZlY2d1I5OEtFM3RFYkN4ZWRONTF5T0dUYWh1WlA1RDJF?=
 =?utf-8?B?TGp0Z2ZwS2NWYThVZkpCcEJjMDFGaDZ3MnJ5OGVud05IbHRjN3pxSitrREF1?=
 =?utf-8?B?aHZqUWlLYVJRL0RKbTI4LzhkYVBUc095SzZ3MHdkbjYxUlQ2OFIrTUNxR1R6?=
 =?utf-8?B?aVZmTVY5bDJEK2xQN0F0UE9FNUd6ZFNQYWR1enVISEhvZHJTSXd3UnRIS3NW?=
 =?utf-8?B?SWk1cWhMKytnemZvaGhUcytSZFFrRnZtUVFCSjE2L2pGa1U4S0xZSEt3N1pj?=
 =?utf-8?B?bTdoUU9FOU84M3NHVC9VU1NxMHBBeG1hZUpieG5UbnhVZnU0YVhrekRtd2Mw?=
 =?utf-8?B?MnRoVjRpdmlQUUFNUlA3ajRmRTZwazRBeGlSK2FEcENzS203blUvYWZ6akI3?=
 =?utf-8?B?akRBMU5BMFZzcWZ2QjUzRjVyeTByRGhkZjNIcERyQy8zazFVSWNmWWJMR2o0?=
 =?utf-8?B?cTNZcE5sQmppWEExZjRxNTdEbDg2blNidDZvUlJsRU03MGQ3UnVHRVpYSCtC?=
 =?utf-8?B?NGc9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e9b0e789-6987-4a6d-fd42-08dadc6b302e
X-MS-Exchange-CrossTenant-AuthSource: DM5PR1501MB2055.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Dec 2022 18:03:35.5749
 (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: HmHlDyRFDII2cnpxmYw1OXrnPyNKGhUJ/ZRHuLB3bPwEPd2nwZXR9guYap1Qk2ag
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB5819
X-Proofpoint-GUID: WiALj-EdSbm8VbA8F3N8YJyZHmBtUZXg
X-Proofpoint-ORIG-GUID: WiALj-EdSbm8VbA8F3N8YJyZHmBtUZXg
Content-Transfer-Encoding: 7bit
X-Proofpoint-UnRewURL: 0 URL was un-rewritten
MIME-Version: 1.0
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-12-12_02,2022-12-12_02,2022-06-22_01
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        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 12/11/22 2:16 PM, david.keisarschm@mail.huji.ac.il wrote:
> From: David <david.keisarschm@mail.huji.ac.il>
> 
> We changed the invocation of
>   prandom_u32_state to get_random_u32.
>   We deleted the maintained state,
>   which was a CPU-variable,
>   since get_random_u32 maintains its own CPU-variable.
>   We also deleted the state initializer,
>   since it is not needed anymore.
> 
> Signed-off-by: David <david.keisarschm@mail.huji.ac.il>
> ---
>   include/linux/bpf.h   |  1 -
>   kernel/bpf/core.c     | 13 +------------
>   kernel/bpf/verifier.c |  2 --
>   net/core/filter.c     |  1 -
>   4 files changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index c1bd1bd10..0689520b9 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2572,7 +2572,6 @@ const struct bpf_func_proto *tracing_prog_func_proto(
>     enum bpf_func_id func_id, const struct bpf_prog *prog);
>   
>   /* Shared helpers among cBPF and eBPF. */
> -void bpf_user_rnd_init_once(void);
>   u64 bpf_user_rnd_u32(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
>   u64 bpf_get_raw_cpu_id(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
>   
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 4cb5421d9..a6f06894e 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2579,13 +2579,6 @@ void bpf_prog_free(struct bpf_prog *fp)
>   }
>   EXPORT_SYMBOL_GPL(bpf_prog_free);
>   
> -/* RNG for unpriviledged user space with separated state from prandom_u32(). */
> -static DEFINE_PER_CPU(struct rnd_state, bpf_user_rnd_state);
> -
> -void bpf_user_rnd_init_once(void)
> -{
> -	prandom_init_once(&bpf_user_rnd_state);
> -}
>   
>   BPF_CALL_0(bpf_user_rnd_u32)
>   {
> @@ -2595,12 +2588,8 @@ BPF_CALL_0(bpf_user_rnd_u32)
>   	 * transformations. Register assignments from both sides are
>   	 * different, f.e. classic always sets fn(ctx, A, X) here.
>   	 */
> -	struct rnd_state *state;
>   	u32 res;
> -
> -	state = &get_cpu_var(bpf_user_rnd_state);
> -	res = predictable_rng_prandom_u32_state(state);
> -	put_cpu_var(bpf_user_rnd_state);
> +	res = get_random_u32();
>   
>   	return res;
>   }

Please see the discussion here.
https://lore.kernel.org/bpf/87edtctz8t.fsf@toke.dk/
There is a performance concern with the above change.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 264b3dc71..9f22fb3fa 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -14049,8 +14049,6 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
>   
>   		if (insn->imm == BPF_FUNC_get_route_realm)
>   			prog->dst_needed = 1;
> -		if (insn->imm == BPF_FUNC_get_prandom_u32)
> -			bpf_user_rnd_init_once();
>   		if (insn->imm == BPF_FUNC_override_return)
>   			prog->kprobe_override = 1;
>   		if (insn->imm == BPF_FUNC_tail_call) {
> diff --git a/net/core/filter.c b/net/core/filter.c
> index bb0136e7a..7a595ac00 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -443,7 +443,6 @@ static bool convert_bpf_extensions(struct sock_filter *fp,
>   			break;
>   		case SKF_AD_OFF + SKF_AD_RANDOM:
>   			*insn = BPF_EMIT_CALL(bpf_user_rnd_u32);
> -			bpf_user_rnd_init_once();
>   			break;
>   		}
>   		break;