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.
>> + * Return
>> + * pointer to socket ops on success, or
>> + * NULL in case of failure
>> + *
>> + * struct bpf_sock_ops *bpf_sk_lookup_udp(ctx, tuple, tuple_size, netns,
>> flags)
>> + * Decription
>> + * Look for UDP 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
>> + * Return
>> + * pointer to socket ops on success, or
>> + * NULL in case of failure
>> + *
>> + * int bpf_sk_release(sock, flags)
>> + * Description
>> + * Release the reference held by 'sock'.
>> + * @sock: Pointer reference to release. Must be found via
>> + * bpf_sk_lookup_xxx().
>> + * @flags: flags value
>> + * Return
>> + * 0 on success, or a negative error in case of failure.
>> */
