Message-ID: <ca31484f-4656-fb3e-8982-6a068bcb0738@iogearbox.net>
Date:   Fri, 28 Feb 2020 00:45:43 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        David Miller <davem@...emloft.net>, kernel-team@...com,
        netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH bpf-next v2 4/4] bpf: inet_diag: Dump bpf_sk_storages in
 inet_diag_dump()

On 2/27/20 2:34 AM, Martin KaFai Lau wrote:
> On Wed, Feb 26, 2020 at 06:21:33PM +0100, Daniel Borkmann wrote:
>> On 2/26/20 12:04 AM, Martin KaFai Lau wrote:
>>> This patch will dump out the bpf_sk_storages of a sk
>>> if the request has the INET_DIAG_REQ_SK_BPF_STORAGES nlattr.
>>>
>>> An array of SK_DIAG_BPF_STORAGE_REQ_MAP_FD can be specified in
>>> INET_DIAG_REQ_SK_BPF_STORAGES to select which bpf_sk_storage to dump.
>>> If no map_fd is specified, all bpf_sk_storages of a sk will be dumped.
>>>
>>> bpf_sk_storages can be added to the system at runtime.  It is difficult
>>> to find a proper static value for cb->min_dump_alloc.
>>>
>>> This patch learns the nlattr size required to dump the bpf_sk_storages
>>> of a sk.  If it happens to be the very first nlmsg of a dump and it
>>> cannot fit the needed bpf_sk_storages,  it will try to expand the
>>> skb by "pskb_expand_head()".
>>>
>>> Instead of expanding it in inet_sk_diag_fill(), it is expanded at a
>>> sleepable context in __inet_diag_dump() so __GFP_DIRECT_RECLAIM can
>>> be used.  In __inet_diag_dump(), it will retry as long as the
>>> skb is empty and the cb->min_dump_alloc becomes larger than before.
>>> cb->min_dump_alloc is bounded by KMALLOC_MAX_SIZE.  The min_dump_alloc
>>> is also changed from 'u16' to 'u32' to accommodate a sk that may have
>>> a few large bpf_sk_storages.
>>>
>>> The updated cb->min_dump_alloc will also be used to allocate the skb in
>>> the next dump.  This logic already exists in netlink_dump().
>>>
>>> Here is the sample output of a locally modified 'ss' and it could be made
>>> more readable by using BTF later:
>>> [root@...h-fb-vm1 ~]# ss --bpf-map-id 14 --bpf-map-id 13 -t6an 'dst [::1]:8989'
>>> State Recv-Q Send-Q Local Address:Port  Peer Address:PortProcess
>>> ESTAB 0      0              [::1]:51072        [::1]:8989
>>> 	 bpf_map_id:14 value:[ 3feb ]
>>> 	 bpf_map_id:13 value:[ 3f ]
>>> ESTAB 0      0              [::1]:51070        [::1]:8989
>>> 	 bpf_map_id:14 value:[ 3feb ]
>>> 	 bpf_map_id:13 value:[ 3f ]
>>>
>>> [root@...h-fb-vm1 ~]# ~/devshare/github/iproute2/misc/ss --bpf-maps -t6an 'dst [::1]:8989'
>>> State         Recv-Q         Send-Q                   Local Address:Port                    Peer Address:Port         Process
>>> ESTAB         0              0                                [::1]:51072                          [::1]:8989
>>> 	 bpf_map_id:14 value:[ 3feb ]
>>> 	 bpf_map_id:13 value:[ 3f ]
>>> 	 bpf_map_id:12 value:[ 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000... total:65407 ]
>>> ESTAB         0              0                                [::1]:51070                          [::1]:8989
>>> 	 bpf_map_id:14 value:[ 3feb ]
>>> 	 bpf_map_id:13 value:[ 3f ]
>>> 	 bpf_map_id:12 value:[ 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000... total:65407 ]
>>>
>>> Acked-by: Song Liu <songliubraving@...com>
>>> Signed-off-by: Martin KaFai Lau <kafai@...com>
>>
>> Hmm, the whole approach is not too pleasant to be honest. I can see why you need
>> it since the regular sk_storage lookup only takes sock fd as a key and you don't
>> have it otherwise available from outside, but then dumping up to KMALLOC_MAX_SIZE
>> via netlink skb is not a great experience either. :( Also, are we planning to add
>> the BTF dump there in addition to bpftool? Thus resulting in two different lookup
>> APIs and several tools needed for introspection instead of one? :/ Also, how do we
>> dump task local storage maps in future? Does it need a third lookup interface?
>>
>> In your commit logs I haven't read on other approaches and why they won't work;
>> I was wondering, given sockets are backed by inodes, couldn't we have a variant
>> of iget_locked() (minus the alloc_inode() part from there) where you pass in ino
>> number to eventually get to the socket and then dump the map value associated with
>> it the regular way from bpf() syscall?
> Thanks for the feedback!
> 
> I think (1) dumping all sk(s) in a system is different from
> (2) dumping all sk of a bpf_sk_storage_map or lookup a particular
> sk from a bpf_sk_storage_map.

Yeah, it is; I was mostly brain-storming if there is a cleaner way for (1)
by having (2)b resolved as an intermediate step (1) can then build on, but
seems it's tricky w/o much extra infra.

[...]
> 
> or the netlink can return map_id only when the max-sized skb cannot fit
> all the bpf_sk_storages.  The userspace then do another syscall to
> lookup the data from each individual bpf_sk_storage_map and
> that requires to lookup side support with another key (non-fd).
> IMO, it is weird and a bit opposite of what bpf_sk_storage should be (fast
> bpf_sk_storage lookup while holding a sk).  The iteration API already
> holds the sk but instead it is asking the usespace to go back to find
> out the sk again in order to get the bpf_sk_storages.  I think that
> should be avoided if possible.
> 
> Regarding i_ino, after looking at sock_alloc() and get_next_ino(),
> hmmm...is it unique?

It would wrap around after 2^32 allocations, so scratch that thought,
iget_locked()/find_inode_fast()-based lookup usage must ensure it's unique.

> If it is, what is the different usecase between i_ino and
> sk->sk_cookie?

Agree that advantage of reusing diag is that you already hold the sk and
are able to iterate through all of them, the ugly part is having to place
the value data into netlink as an API along with all other socket data as
a, for better or worse, bpf sk map lookup/introspection interface given
there is no other way to have a fast and global (non-fd based) id->socket
lookup interface that we could reuse atm.

I was wondering about sk->sk_cookie as well, but it wouldn't make sense
to do a ss dump, get (sk cookie, map_id) from the dump and use that for
a bpf() lookup if we need to reiterate the diag tables once again in the
background just to get the storage data. And I presume it won't make sense
either to reuse the diag's walk as a stand-alone for a bpf sk storage dump
interface API via bpf() ... at least from ss tool side it would require a
correlation based on sk cookie. Not nice either ... so current approach
might indeed be the tradeoff. :/

Thanks,
Daniel

Powered by blists - more mailing lists