On 09/24/2018 08:05 PM, Joe Stringer wrote:
> On Mon, 24 Sep 2018 at 05:38, Daniel Borkmann <dan...@iogearbox.net> wrote:
>> On 09/24/2018 02:12 PM, Daniel Borkmann wrote:
>>> Hi Joe,
>>>
>>> couple of comments inline:
>>>
>>> On 09/21/2018 07:10 PM, Joe Stringer wrote:
>>>> This patch adds new BPF helper functions, bpf_sk_lookup_tcp() and
>>>> bpf_sk_lookup_udp() which allows BPF programs to find out if there is a
>>>> socket listening on this host, and returns a socket pointer which the
>>>> BPF program can then access to determine, for instance, whether to
>>>> forward or drop traffic. bpf_sk_lookup_xxx() may take a reference on the
>>>> socket, so when a BPF program makes use of this function, it must
>>>> subsequently pass the returned pointer into the newly added sk_release()
>>>> to return the reference.
>>>>
>>>> By way of example, the following pseudocode would filter inbound
>>>> connections at XDP if there is no corresponding service listening for
>>>> the traffic:
>>>>
>>>> struct bpf_sock_tuple tuple;
>>>> struct bpf_sock_ops *sk;
>>>>
>>>> populate_tuple(ctx, &tuple); // Extract the 5tuple from the packet
>>>> sk = bpf_sk_lookup_tcp(ctx, &tuple, sizeof tuple, netns, 0);
>>>> if (!sk) {
>>>> // Couldn't find a socket listening for this traffic. Drop.
>>>> return TC_ACT_SHOT;
>>>> }
>>>> bpf_sk_release(sk, 0);
>>>> return TC_ACT_OK;
>>>>
>>>> Signed-off-by: Joe Stringer <j...@wand.net.nz>
>>>>
>>>> ---
>>>>
>>>> v2: Rework 'struct bpf_sock_tuple' to allow passing a packet pointer
>>>> Limit netns_id field to 32 bits
>>>> Fix compile error with CONFIG_IPV6 enabled
>>>> Allow direct packet access from helper
>>>> ---
>>>> include/uapi/linux/bpf.h | 57 ++++++++-
>>>> kernel/bpf/verifier.c | 8 +-
>>>> net/core/filter.c | 149 ++++++++++++++++++++++
>>>> tools/include/uapi/linux/bpf.h | 57 ++++++++-
>>>> tools/testing/selftests/bpf/bpf_helpers.h | 12 ++
>>>> 5 files changed, 280 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>> index aa5ccd2385ed..620adbb09a94 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -2143,6 +2143,41 @@ union bpf_attr {
>>>> * request in the skb.
>>>> * Return
>>>> * 0 on success, or a negative error in case of failure.
>>>> + *
>>>> + * struct bpf_sock_ops *bpf_sk_lookup_tcp(ctx, tuple, tuple_size, netns,
>>>> flags)
>>>
>>> Nit: could you add proper signature like in other cases that are documented?
>>>
>>>> + * Decription
>>>> + * Look for TCP socket matching 'tuple'. The return value must
>>>> + * be checked, and if non-NULL, released via bpf_sk_release().
>>>> + * @ctx: pointer to ctx
>>>> + * @tuple: pointer to struct bpf_sock_tuple
>>>> + * @tuple_size: size of the tuple
>>>> + * @netns: network namespace id
>>>> + * @flags: flags value
>>>
>>> Should probably say in all three cases that it's unused right now and
>>> reserved
>>> for future.
>>
>> I think the two lookup helpers here also need to have a better documentation
>> in
>> terms of semantics wrt netns, so it definitely needs to be documented when
>> it's
>> derived from the skb's dev and when netns is used.
>
> Good point, maybe some text like this:
>
> If 'netns' is 0, then the netns associated with the ctx will be used.
> For the tc hooks, this is the netns of the device in the skb. For
> socket hooks, this is the netns of the socket. If netns is nonzero,
> then this is the netns id relative to the netns associated with the
> ctx.
Looks good, I'd probably add to the first one "[...] then the socket lookup
table in the netns associated with the ctx will be used."
