On 2022/7/1 03:20, Andrii Nakryiko wrote:
> On Thu, Jun 30, 2022 at 6:53 AM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote:
>>
>> Currently, libbpf requires specifying binary path when attach USDT BPF program
>> manually. This is not necessary because we can infer that from /proc/$PID/exe.
>> This also avoids coredump when user do not provide binary path.
>>
>> Signed-off-by: Hengqi Chen <hengqi.chen@xxxxxxxxx>
>> ---
>
> Hm... just because I specify PID, doesn't mean I mean main binary of
> that process, it could be some other shared library within that
> process.
>
> I don't really like this change because it doesn't feel 100% obvious
> and natural. User can easily specify "/proc/self/exe" or
> "/proc/<pid>/exe" if that's what they are targeting.
>
Actually, this is an implication and practice set by BCC.
If path is not set, BCC assumes that it is targeting the process.
> Documentation for bpf_program__attach_usdt() states
>
> @param binary_path Path to binary that contains provided USDT probe
>
> it doesn't say it is optional and can be NULL, so there is no valid
> reason to pass NULL and expect things to work.
>
>
OK, then we should at least check against NULL and emit a warning.
>> tools/lib/bpf/libbpf.c | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 8a45a84eb9b2..4ee9b6a0944e 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -10686,7 +10686,19 @@ struct bpf_link *bpf_program__attach_usdt(const struct bpf_program *prog,
>> return libbpf_err_ptr(-EINVAL);
>> }
>>
>> - if (!strchr(binary_path, '/')) {
>> + if (!binary_path) {
>> + if (pid < 0) {
>> + pr_warn("prog '%s': missing attach target, pid or binary path required\n",
>> + prog->name);
>> + return libbpf_err_ptr(-EINVAL);
>> + }
>> + if (!pid)
>> + binary_path = "/proc/self/exe";
>> + else {
>> + snprintf(resolved_path, sizeof(resolved_path), "/proc/%d/exe", pid);
>> + binary_path = resolved_path;
>> + }
>> + } else if (!strchr(binary_path, '/')) {
>> err = resolve_full_path(binary_path, resolved_path, sizeof(resolved_path));
>> if (err) {
>> pr_warn("prog '%s': failed to resolve full path for '%s': %d\n",
>> --
>> 2.30.2
>>
