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: <33c52b4c-2a16-7578-f782-51267deff750@meta.com>
Date:   Thu, 8 Jun 2023 10:38:12 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.11.2
Subject: Re: [PATCH bpf v2 2/2] bpf: ensure main program has an extable
Content-Language: en-US
To:     Krister Johansen <kjlx@templeofstupid.com>, bpf@vger.kernel.org
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        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>,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        stable@vger.kernel.org
References: <cover.1686166633.git.kjlx@templeofstupid.com>
 <de425e99876dc6c344e1a4254894a3c81e71a2ec.1686166633.git.kjlx@templeofstupid.com>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <de425e99876dc6c344e1a4254894a3c81e71a2ec.1686166633.git.kjlx@templeofstupid.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?ZGthQXdlMkJpWDVtckNoa0xsUmsxcjNUbmpCRVJYM0FvQWFoN09FZGxVcEdj?=
 =?utf-8?B?OHBBWTh1N3RtWEgwOGFpUTBPOHN1SUlTMzV3RUlTT3RjQm5Ud2FQUC9lYndM?=
 =?utf-8?B?ODJ6bEhqcDgyYitIU0tiME1jd3JOZ3ZFM0NsaVRkOEw1dGNnVDdkMFIrSmhV?=
 =?utf-8?B?RnZqdE1Dd2R0SVJLU3dORDBYbThZbXhia2owZDR5VGpITkVmR2pWUzVFS2NE?=
 =?utf-8?B?VXFMZHpNdWxFNWxIMVhld0dRSzVXU0VjZkdrbTFFMXFqSDhLTU9icVhBQWpX?=
 =?utf-8?B?V1d2Slg4bFc3OXZvblYyVnMvaXlXek9Oc0F4SDk3b2dSQ1ZJcGtndWRMNUl5?=
 =?utf-8?B?SzRJVVZpWjJncWlIakxVOE1tSWxrNkdWU1VhK1J6SVFiQWxHN0l3VDJwUWt1?=
 =?utf-8?B?a3ZlOUNjNHhFSC9Bb2I5UjN2Vm16d0NnZnBJd1VmaGtvQVd1Mk9BNGowZmZu?=
 =?utf-8?B?akxuS3Q3ZDVhUTNxSkpJNG9UUTE1ZUxoaDE2L2hsWHNYMitYTTdMZVFIRHZk?=
 =?utf-8?B?NHdaZ1ViVFRoZGFrMU1STkw5dUUzMGYzOHVUYTdYQzFlSncyODVJWVJpSTBw?=
 =?utf-8?B?My9BUU5ZRUI0WjdBMlZtSS9WdzVvZWUvNVFTb3RaVENwUVZVSHJIWTMxU0tX?=
 =?utf-8?B?aE40SXY0aHZlbFZLZmJUZlhBN25CdWpPSDBxOHZ6YTBrbjJnWnF3ZS9hODNG?=
 =?utf-8?B?WGhEK2EvcFlrUG0wTC9tODZzR3BYUitZRUdsQjNDMkJ6QURkcFhZM3JZQ3FV?=
 =?utf-8?B?aFk2SWpOdmRNazBNdDNKdFFQNTNNU3d0VEk3NjYzOUFLNDZvRmg5dTJjOVd6?=
 =?utf-8?B?VElwU05RTlRTL3BlWEZKT0kyUDRNWXFjbEhOcjYxdC9IUmc5c3QvYndkUjNt?=
 =?utf-8?B?UThlOUJ3UVVXajJZR29kRTAxeUdoaC8wejBESXJlaTJ0SWV1UW9PNHlVbjg4?=
 =?utf-8?B?QzBrRDh3NmVBWjVyTXljQWdpckw2UDFSNzNiYUNsaUp4MDJQQnA2SjBKZ2VM?=
 =?utf-8?B?YitZU0xtQ3A3anp2Y0xTdlJreE82UklBOEk2L1hVZUJuaFg2bjd5aWIwQnhr?=
 =?utf-8?B?Q0d3RW9lRjR4emlNM2EvU2F3cy90SktGN0Z5QmIzSS83WHBnb0xpamVqMDVu?=
 =?utf-8?B?UGtFZDR2OVVJY2pRVCs4YVVPOEtoWWZEbmtmQXhiQmY4Sjd1M2FxbThabDdX?=
 =?utf-8?B?UXFrMzk2QmxIc2k1aDQ3K3kxTnFhM1RvcCs2UCtrMnk3TmY1bi94eGJIaGVM?=
 =?utf-8?B?RG1NVUNpQm1vdmNWRE1McE1YeWpTb2VJaWNZaDh6R1ZybHJpY0hJcURsVDhj?=
 =?utf-8?B?QVBrcHg2VUVYaHh3emE2U3hHTEtkMjRmOTlENUhLd3NrbHNxWHNtaklCN3Yw?=
 =?utf-8?B?bSs4Rm1YNStxRXpUdjkxVWMrMUFIQVBYeEUvZnVueEpCMFB0OFlPdy9GU1Nv?=
 =?utf-8?B?dkhnenBGWldNWlpCQXRVOEZvYUZhTEpKR2g2aUNBUWlMZmF3endoNWdFbDJR?=
 =?utf-8?B?S0pzNEhUclVld3NNTzNvSkR3U20wQzJwbzlVS2dOejVaanh3VSt3V1VMQjVT?=
 =?utf-8?B?aTc5bnpWMmJHYjY0bVJVOS9tazg4bGRWV1JCMGhWOVpMdG0rTjlZbWtZZXdu?=
 =?utf-8?B?S3VpVXpuS29ndTd1a1FqWmVvTGZOdmpsK1RPL1ZKdm5oSnNBVUxpcHhFbmZy?=
 =?utf-8?B?VmROTnZ6bFA4WnBDTGdPTk5BMmkrTUw0SUhxa1dHeVlnTHAwbFFPeUtNVVd1?=
 =?utf-8?B?dlE3T1Q0SG5jaEMyWDd3dUQ5RzQ3cmtiMEhsK2t4VXBDV2Q2ZzdYT2FaTUla?=
 =?utf-8?B?bTRaNFNuM0dzYVNMTWs3ZUdNWTNkeHpSby9ORjZBZ2pIUlM5dXd0VGROU0Jo?=
 =?utf-8?B?Yytzd2Nzb1dkamgrYUZsQlpUYTdGNWdzQzAwSzhxVVJMZU5xQ3FEMWJIZzdN?=
 =?utf-8?B?M1RsUUIvNUZjZlNYOW5wVWNzYStocU9uUW92dVplMFdvMVZRZ1pnUW1oQ2lx?=
 =?utf-8?B?dDJXK1NLK0ZmVm9QVE5JRWtRMjl4TThTcHNIaXJmbElsVGFiVkVZRFpUMGgz?=
 =?utf-8?B?UysyQUsvbEx0QmdxbmtoTExrTjBJUVBheU9DQUhYTkFyZjJMYnN0b3RBc0hk?=
 =?utf-8?B?Q0s2NG5IZzlZRUVJejFlYVErM3JJcEs2UVBHOXpsdmNWVzFzVmNJQUIzNHc0?=
 =?utf-8?B?WHc9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6e56f441-3450-4aa6-92cf-08db6847239f
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jun 2023 17:38:15.4034
 (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: S8aPY9fHSWXPy/gXtf9f6m+D9m4O+lr5hGsNOkKrtpEAsZB0uqq95BuGegLilI2w
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR15MB6076
X-Proofpoint-GUID: cPY49ZeJkutA-I4M2OvnlP_sUd4N2d1K
X-Proofpoint-ORIG-GUID: cPY49ZeJkutA-I4M2OvnlP_sUd4N2d1K
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26
 definitions=2023-06-08_12,2023-06-08_01,2023-05-22_02
X-Spam-Status: No, score=-2.9 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_PASS,
        T_SCC_BODY_TEXT_LINE 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 6/7/23 2:04 PM, Krister Johansen wrote:
> When bpf subprograms are in use, the main program is not jit'd after the
> subprograms because jit_subprogs sets a value for prog->bpf_func upon
> success.  Subsequent calls to the JIT are bypassed when this value is
> non-NULL.  This leads to a situation where the main program and its
> func[0] counterpart are both in the bpf kallsyms tree, but only func[0]
> has an extable.  Extables are only created during JIT.  Now there are
> two nearly identical program ksym entries in the tree, but only one has
> an extable.  Depending upon how the entries are placed, there's a chance
> that a fault will call search_extable on the aux with the NULL entry.
> 
> Since jit_subprogs already copies state from func[0] to the main
> program, include the extable pointer in this state duplication.  The
> alternative is to skip adding the main program to the bpf_kallsyms
> table, but that would mean adding a check for subprograms into the
> middle of bpf_prog_load.

I think having two early identical program ksym entries is bad.
When people 'cat /proc/kallsyms | grep <their program name>',
they will find two programs with identical kernel address but different
hash value. This is just very confusing. I think removing the
duplicate in kallsyms is better from user's perspective.

> 
> Cc: stable@vger.kernel.org
> Fixes: 1c2a088a6626 ("bpf: x64: add JIT support for multi-function programs")
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> ---
>   kernel/bpf/verifier.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5871aa78d01a..d6939db9fbf9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17242,6 +17242,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>   	prog->jited = 1;
>   	prog->bpf_func = func[0]->bpf_func;
>   	prog->jited_len = func[0]->jited_len;
> +	prog->aux->extable = func[0]->aux->extable;
>   	prog->aux->func = func;
>   	prog->aux->func_cnt = env->subprog_cnt;
>   	bpf_prog_jit_attempt_done(prog);