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: <03e16727-7a0a-0e1e-e9b9-c947a64becb9@meta.com>
Date:   Fri, 13 Jan 2023 00:00:55 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH] libbpf: resolve kernel function name optimization for
 kprobe
Content-Language: en-US
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>,
        Alan Maguire <alan.maguire@oracle.com>
Cc:     Menglong Dong <menglong8.dong@gmail.com>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Alexei Starovoitov <ast@kernel.org>,
        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>,
        bpf <bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Menglong Dong <imagedong@tencent.com>
References: <20230109094247.1464856-1-imagedong@tencent.com>
 <504cc35a-74a8-751a-5899-186d7a0aff87@meta.com>
 <CADxym3bRciuyM1nYCrbaAwSMRJQvgV=hJFSLeiu9jysejPaTQQ@mail.gmail.com>
 <6c14e7ad-3b6d-4f88-64b8-8e3968d2b2e6@meta.com>
 <6455133c-87a2-1a0f-7da4-f8b99f02fc95@oracle.com>
 <CAADnVQJRntdqa4uCHtTrQNAsgGS13DtNV-ue2wTdHQxiuLo_Yg@mail.gmail.com>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <CAADnVQJRntdqa4uCHtTrQNAsgGS13DtNV-ue2wTdHQxiuLo_Yg@mail.gmail.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?dmJBdklBTnJncTBsajJEMytyV2d1QTFRS1lsZTlsSnJwOWtoODBqTWc1djdy?=
 =?utf-8?B?bVBCaEhGS0g4My9vV0I5aXJRMTViMDRaRUg0TUJrMllEM1ZEbXhjQThSUHZu?=
 =?utf-8?B?bDBXYnFVNWl6eEp0RE4wRTU0K1VKWG1JTmVEQ0EzaVE5dXBTZFNnVG5vcTli?=
 =?utf-8?B?YVdyYUdhcTFWcFFob3NwOEV4UHZybnV1UjVTTFpuaW1FdDAydGZWbVg4Z293?=
 =?utf-8?B?cTFnQXY3M0R3eVVDWjZST3JOSUlNak1ibGxIR05nYjV2N1pGU0xTdm53ZTNi?=
 =?utf-8?B?aW9vRUwySVlxQUREMUQ4QUgwaG9Fa2h0WnJvNkV6V3NEbmlKRVh3Q3MwNUlk?=
 =?utf-8?B?ZUhTSFZDMzkvekExUG9sTEdUNWpKK3hNL3E1Nlp6T1BKcWZEWjJPMjhOVTBn?=
 =?utf-8?B?cEhsSjVCQ0c4dFRpcENXQVJJRlh0RHc1ZEh1Qno3N0ZOR3cxdkVBQU0yU1No?=
 =?utf-8?B?MjR3MGhJUktOalhnS2QzaU5RUnY4RTVvelJ2Wm55bHJRWXVWRTgzRVl1TUxo?=
 =?utf-8?B?eDlPT0FuY1lVbXZPU21GWFladnIvSFlOZ1lxbDI2ZFFHT2oxemxUUGQ4Nmpw?=
 =?utf-8?B?YkhtSXV0NUlmWkN0UlFJbFE2bDh2czZjY3BjTUVrV3k4NUhjbHl3WXcvQkdV?=
 =?utf-8?B?a0JuWVZsdmNzc2lqbmgzd2tISVFDckEyMGdLRVdFbTEzTngzT2tkV0lSc2NY?=
 =?utf-8?B?ZzQ0eEE0a0RUT2NCWGRCRkhndHFEb2Y4aGRBaTEwRG5rTlA3VmhUNHZaTkpQ?=
 =?utf-8?B?a3JRVWs5YmhPWDcydkxiMC83T05LZDhJYTFMdjNoYkhWblJZem9ZZjlpc3ZM?=
 =?utf-8?B?Q1lvV1kvQ20zNGNHaEpEdm5lT1dtNDI0WVdmdUFMZmJ4Z2ZCQmxXb1E1R2tG?=
 =?utf-8?B?MWcxZE54Rk94dTZvRlNqYWZQYmRJVktVMHdHTGNGcUNDRHFSM0FadHE4OE9k?=
 =?utf-8?B?aGR6WHg2VjZhUm9xSXhMdlA5eWxKUDZVS1RDbUxVZ3loY1ppbXhhRHJxY1VE?=
 =?utf-8?B?S1ROcUdycEJlRU52K3ljSzNUMmZOVFpURFUwMXNNZVBIVXh5amtiZVQ1T3hK?=
 =?utf-8?B?QllLcnVGQzREWmpsUWZlQzRkODJrRENLU3BFenBxajR2eTFMcEhKRmcxUytr?=
 =?utf-8?B?ak5mL1I4OUIybTA3cGlQWVdUWjVpZ3o1M1hocFdBNlU4bDBMZ1laUm05emlm?=
 =?utf-8?B?dkhDVEdmV3I2cVY1N0FFZDVGV0MvdkVORElmc2h5WS84TTFadDVIYW1DR3RZ?=
 =?utf-8?B?bURWMWt5Uzl6UGtEVERUR2ZyMVZ4QUhhdStZazl5VC9hWmE5Rzgydm1nOFNS?=
 =?utf-8?B?RDNFSDRwdS85bFZEU1lsSzZpOHB6SG51ZDBBV1VUK0o3MlAxaXFHa0o2aWhR?=
 =?utf-8?B?VHJqRlJVVFU4QmNkQUNkd3NJTFNsVm1sUk1vcGdUbHZ2TUtkVXJjVWNvMDNV?=
 =?utf-8?B?NkVvaHJHOVBNbkNoa0dtOW9HbVN5L05lQUhUbUE5SkdnZ3k1WmU5b0JodEpJ?=
 =?utf-8?B?S2tJcGhwZHBCOFNzTkxGQU5wQmtpb05id01QSDRaOXFJQi8zS3pjNFhUNk9o?=
 =?utf-8?B?MCtiZDhmVmxRMllaQnc3RTltellURUltblIwcTZDNmw5S2tpNVg0eHR3YnUv?=
 =?utf-8?B?cCtLcm5BOVFjdTNLY3RsNnZuajhEUG9DUDl3UUh6VGNPNk10NWlPcktwVGZn?=
 =?utf-8?B?cU03YkgwOHVJMnJCbEhqRlFub2JSUCs2eGErcG95SEl1QU5wSi82N1IxY2RD?=
 =?utf-8?B?Wko4L3lnbTRIQjhZMWVLd2RKNmNMMXJMd210eGFWZHNYQ1pZRVp6Q0drTWlr?=
 =?utf-8?B?bVZ6UVFzaWpKSUJYU1N1NHNuSDk4dnBpOTBUSUpydEJ5NEZxb1VmOUNKUE84?=
 =?utf-8?B?dDBUL2JCbVhKb1BMUThEWkRTakNKS3dhMXNmUDFYLzBEYlh5OVRuUzhPM0NK?=
 =?utf-8?B?N0RkS0VhbWdNaEpSYndlRU5xZ3NCbC9leWkxM0ErRzFReGxWYnpUeGlvdHRr?=
 =?utf-8?B?UXhPQjJpcURnaGJSRll1WWRwYVZCcnU3NUNjazhmV1V3Ym1lWlc1NVcxNjEx?=
 =?utf-8?B?YjZzYVNCeUJGZ3MrSGNVVk43dzJqSWRwTVNuNllqcHJRQXgrMlErckNQeWNl?=
 =?utf-8?B?ck02dnUvUTAwQmlPYXc3OWNHVFpOQ21HOE0rZTdzSy84QlBta09QL3pjcTUw?=
 =?utf-8?B?K1E9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 96e3a361-f8dc-4605-a73f-08daf53c4e5b
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jan 2023 08:00:58.9381
 (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: iyz2ziDzkaS+EaIeM89mqjOb+lvpBb35NG9Z8asXTs+TCyBNVOXleYLhxILQ2XsB
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR15MB4892
X-Proofpoint-GUID: r5de66iUmKUokRG7hWIubOFr3PT_nSNS
X-Proofpoint-ORIG-GUID: r5de66iUmKUokRG7hWIubOFr3PT_nSNS
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2023-01-13_03,2023-01-12_01,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 1/12/23 1:07 PM, Alexei Starovoitov wrote:
> On Thu, Jan 12, 2023 at 2:20 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>>
>> On 12/01/2023 07:23, Yonghong Song wrote:
>>>
>>>
>>> On 1/9/23 7:11 PM, Menglong Dong wrote:
>>>> On Tue, Jan 10, 2023 at 4:29 AM Yonghong Song <yhs@meta.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 1/9/23 1:42 AM, menglong8.dong@gmail.com wrote:
>>>>>> From: Menglong Dong <imagedong@tencent.com>
>>>>>>
>>>>>> The function name in kernel may be changed by the compiler. For example,
>>>>>> the function 'ip_rcv_core' can be compiled to 'ip_rcv_core.isra.0'.
>>>>>>
>>>>>> This kind optimization can happen in any kernel function. Therefor, we
>>>>>> should conside this case.
>>>>>>
>>>>>> If we failed to attach kprobe with a '-ENOENT', then we can lookup the
>>>>>> kallsyms and check if there is a similar function end with '.xxx', and
>>>>>> retry.
>>>>>
>>>>> This might produce incorrect result, so this approach won't work
>>>>> for all .isra.0 cases. When a function name is changed from
>>>>> <func> to <func>.isra.<num>, it is possible that compiler may have
>>>>> make some changes to the arguments, e.g., removing one argument,
>>>>> chaning a semantics of argument, etc. if bpf program still
>>>>> uses the original function signature, the bpf program may
>>>>> produce unexpected result.
>>>>
>>>> Oops, I wasn't aware of this part. Can we make this function disabled
>>>> by default and offer an option to users to enable it? Such as:
>>>>
>>>>       bpf_object_adapt_sym(struct bpf_object *obj)
>>>>
>>>> In my case, kernel function rename is common, and I have to
>>>> check all functions and do such adaptation before attaching
>>>> my kprobe programs, which makes me can't use auto-attach.
>>>>
>>>> What's more, I haven't seen the arguments change so far, and
>>>> maybe it's not a common case?
>>>
>>> I don't have statistics, but it happens. In general, if you
>>> want to attach to a function like <foo>, but it has a variant
>>> <foo>.isra.<num>, you probably should check assembly code
>>> to ensure the parameter semantics not changed, and then
>>> you can attach to kprobe function <foo>.isra.<num>, which
>>> I assume current libbpf infrastructure should support it.
>>> After you investigate all these <foo>.isra.<num> functions
>>> and confirm their argument semantics won't change, you
>>> could use kprobe multi to do attachment.
>>>
>>
>> I crunched some numbers on this, and discovered out of ~1600
>> .isra/.constprop functions, 76 had a missing argument. The patch series
>> at [1] is a rough attempt to get pahole to spot these, and add
>> BTF entries for each, where the BTF representation reflects
>> reality by skipping optimized-out arguments. So for a function
>> like
>>
>> static int ip6_nh_lookup_table(struct net *net, struct fib6_config *cfg,
>>                                 const struct in6_addr *gw_addr, u32 tbid,
>>                                 int flags, struct fib6_result *res);
>>
>> Examining the BTF representation using pahole from [1], we see
>>
>> int ip6_nh_lookup_table.isra.0(struct net *net, struct fib6_config *cfg, struct in6_addr *gw_addr, u32 tbid, int flags);
>>
>> Comparing to the definition, we see the last parameter is missing,
>> i.e. the "struct fib6_result *" argument is missing. The calling pattern -
>> where the callers have a struct fib6_result on the stack and pass a pointer -
>> is reflected in late DWARF info which shows the argument is not actually
>> passed as a register, but can be expressed as an offset relative to the current
>> function stack (DW_OP_fbreg).
>>
>> This approach howvever introduces the problem that currently the kernel
>> doesn't  allow a "." in a function name. We can fix that, but any BTF encoding
>> that introduced optimized functions containing a  "." would have to be opt-in
>> via a pahole option, so we do not generate invalid vmlinux BTF for kernels
>> without that change.
>>
>> An alternative approach would be to simply encode .isra functions
>> in BTF without the .isra suffix (i.e. using "function_name" not
>> "function_name.isra"), only doing the BTF encoding if no arguments were
>> optimized out - i.e. if the function signature matches expectations.
>> The 76 functions with optimized-out parameters could simply be skipped.
>> To me that feels like the simpler approach - it avoids issues
>> with function name BTF encoding, and with that sort of model a
>> loose-matching kallsyms approach - like that described here - could be used
>> for kprobes and fentry/fexit. It also fits with the DWARF representation -
>> the .isra suffixes are not present in DWARF representations of the function,
>> only in the symbol table and kallsyms, so perhaps BTF should follow suit
>> and not add the suffixes. What do you think?
> 
> Sounds like a great idea to me.
> Addresses this issue in a clean way.

Yes, the second approach seems a reasonable approach. If the number
of parameters for the *actual* functions equals to the number
of parameters for the defined function (abstract_origin),
we can roughly assume the actual function signature matches
the prototype. Although it is theoretically possible that
compiler might change parameter types, e.g., from a
struct pointer (struct foo *p) to a int value (p->field1).
But this should be extremely rare and we need compiler emitting
additional dwarf data (might through btf_decl_tag) to discover
such cases.