Re: Extending bpf_get_ns_current_pid_tgid()

[Carlos Neira <cneirabustos@gmail.com>]

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.


>>
>>
>>
>>
>>
>> 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
>>      >
>>      >
>>     [...]
>>