Re: Extending bpf_get_ns_current_pid_tgid()

[Blaise Sanouillet <blez@fb.com>]

>On 6/16/21 2:44 PM, Carlos Neira wrote:
>> On 6/16/21 1:02 PM, Yonghong Song wrote:
>>>
>>>
>>> On 6/15/21 6:08 PM, carlos antonio neira bustos wrote:
>>>> I'm resuming work on this and would like to know your opinion and 
>>>> concerns about the following proposal:
>>>>
>>>> - Add  s_dev from  nsfs to ns_common, so now ns_common will have 
>>>> inode and device to identify the namespace, as in the future 
>>>> namespaces will need to match against ino and device.
>>>>
>>>> - That will allow us to remove the call to ns_match on because the 
>>>> values checked are now present in ns_common (ino and dev_t).
>>>
>>> I understand its benefit but I am not 100% sure whether adding s_dev 
>>> to ns_common will be accepted or not by upstream just because of this.
>>>
>>> Note that if adding s_dev to ns_common, you then need to ensure s_dev
>>> contains valid value for all usages of ns_common, practically all
>>> namespaces, not just nsfs, otherwise people may argument against this
>>> as existing mechanism works and the change brings little value.
>>> If you go this route, please ensure other namespaces can also
>>> take advantage of this field.
>> 
>> This route seems like a long one, but is the easier solution that I can 
>> think at this moment.I'll read more of the code to have a better 
>> understanding of the consequences.
>> 
>> 
>>>>
>>>> - Add a new helper named  bpf_get_current_pid_tgid_from_ns that will 
>>>> return pid/tgid from the current task if pid ns matches ino and dev 
>>>> supplied by the user as now both values are in ns_common.
>>>
>>> I think current helper get_ns_current_pid_tgid() can already do this.
>>> Did I miss anything?
>>>
>> 
>> The problem with get_ns_current_pid_tgid is that device and ino provided 
>> by the user are compared against the current task pid namespace ino but 
>> dev_t as is not part of ns_common is compared with against the current 
>> nsfs dev_t. So the helper will only return pid/tgid from the current 
>> namespace but not will be able to do it for a target ns due to this 
>> limitation.
>
>Okay, I see you want to get tgid/pid for an arbitrary pidns identified
>with (dev, inode). That makes sense as you need both to compare given
>pidns (dev, inode) info. What is your use case? I guess you try to have
>a daemon monitoring selected containers, is that right?

A user-space tracer like bpftrace needs the pid of the traced process in the
tracer's namespace to be able to symbolize the stack traces, for example.
Note that that the tracer and the traced process are not necessarily in the
same pid namespace or container.
For reference, here's our last discussion on this:
	https://lore.kernel.org/bpf/ba5f3c14-8261-af6f-8850-90848963d63a@fb.com/

Thanks,
Blaise

>
>> 
>> 
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, Nov 13, 2020 at 1:59 PM Yonghong Song <yhs@fb.com 
>>>> <mailto:yhs@fb.com>> wrote:
>>>>
>>>>
>>>>
>>>>     On 11/13/20 6:34 AM, carlos antonio neira bustos wrote:
>>>>      > Hi Blaise and Daniel,
>>>>      >
>>>>      >
>>>>      > I was following a couple of months ago how bpftrace was going to
>>>>     handle
>>>>      > this situation. I thought this PR
>>>>      > https://github.com/iovisor/bpftrace/pull/1602
>>>>     <https://github.com/iovisor/bpftrace/pull/1602>
>>>>      > <https://github.com/iovisor/bpftrace/pull/1602
>>>>     <https://github.com/iovisor/bpftrace/pull/1602>> was going to be 
>>>> merged
>>>>      > but just found today that is not working.
>>>>      >
>>>>      > I agree with Yonghong Song on the approach of using the two 
>>>> helpers
>>>>      > (bpf_get_pid_tgid() and bpf_get_ns_current_pid_tgid()) to move
>>>>     forward
>>>>      > on the short term, bpf_get_ns_current_pid_tgid works as a
>>>>     replacement
>>>>      > to bpf_get_pid_tgid when you are instrumenting inside a 
>>>> container.
>>>>      >
>>>>      > But the use case described by Blaise is one I would like to use
>>>>     bpftrace,
>>>>      >
>>>>      > If nobody is against it, I could start working on a new helper to
>>>>      > address that situation as I need to have bpftrace working in that
>>>>     scenario.
>>>>
>>>>     Yes, please. Thanks!
>>>>
>>>>      >
>>>>      > For my understanding of the problem the new helper should be 
>>>> able to
>>>>      > return pid/tgid from a target namespace, is that correct?.
>>>>
>>>>     Yes. This way, the stack trace can correlate to target namespace for
>>>>     symbolization purpose.
>>>>
>>>>      >
>>>>      >
>>>>      > Bests
>>>>      >
>>>>      >
>>>>     [...]
>>>>
>>