netdev - Re: [PATCH bpf-next v2 05/17] inet: Run SK_LOOKUP BPF program on socket lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87v9kzubwp.fsf@cloudflare.com>
Date:   Wed, 13 May 2020 16:50:30 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Lorenz Bauer <lmb@...udflare.com>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        dccp@...r.kernel.org, kernel-team <kernel-team@...udflare.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Gerrit Renker <gerrit@....abdn.ac.uk>,
        Jakub Kicinski <kuba@...nel.org>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Martin KaFai Lau <kafai@...com>,
        Marek Majkowski <marek@...udflare.com>
Subject: Re: [PATCH bpf-next v2 05/17] inet: Run SK_LOOKUP BPF program on socket lookup

On Wed, May 13, 2020 at 04:21 PM CEST, Lorenz Bauer wrote:
> On Tue, 12 May 2020 at 14:52, Jakub Sitnicki <jakub@...udflare.com> wrote:

[...]

>> So if IIUC the rough idea here would be like below?
>>
>> - 1st program calls
>>
>>   bpf_sk_assign(ctx, sk1, 0 /*flags*/) -> 0 (OK)
>>
>> - 2nd program calls
>>
>>   bpf_sk_assign(ctx, sk2, 0) -> -EBUSY (already selected)
>>   bpf_sk_assign(ctx, sk2, BPF_EXIST) -> 0 (OK, replace existing)
>>
>> In this case the last program to run has the final say, as opposed to
>> the semantics where DROP/REDIRECT terminates.
>
> Does sk_assign from TC also gain BPF_EXIST semantics? As you know,
> I'm a bit concerned that TC and sk_lookup sk_assign are actually to completely
> separate helpers. This is a good way to figure out if its a good idea to
> overload the name, imo.

I don't have a strong opinion here. We could have a dedicated helper.

Personally I'm not finding it confusing. As a BPF user you know what
program type you're working with (TC vs SK_LOOKUP), and both helper
variants will be documented separately in the bpf-helpers man-page, like
so:

       int bpf_sk_assign(struct sk_buff *skb, struct bpf_sock *sk, u64
       flags)

              Description
                     Helper is overloaded  depending  on  BPF  program
                     type.     This     description     applies     to
                     BPF_PROG_TYPE_SCHED_CLS                       and
                     BPF_PROG_TYPE_SCHED_ACT programs.

                     Assign  the  sk  to  the  skb. When combined with
                     appropriate routing configuration to receive  the
                     packet  towards  the socket, will cause skb to be
                     delivered to the  specified  socket.   Subsequent
                     redirection    of    skb   via    bpf_redirect(),
                     bpf_clone_redirect() or other methods outside  of
                     BPF may interfere with successful delivery to the
                     socket.

                     This operation is  only  valid  from  TC  ingress
                     path.

                     The flags argument must be zero.

              Return 0  on  success,  or  a  negative errno in case of
                     failure.

                     · -EINVAL           Unsupported flags specified.

                     · -ENOENT            Socket  is  unavailable  for
                       assignment.

                     · -ENETUNREACH       Socket is unreachable (wrong
                       netns).

                     ·

                       -EOPNOTSUPP Unsupported operation, for  example
                       a
                              call from outside of TC ingress.

                     · -ESOCKTNOSUPPORT   Socket  type  not  supported
                       (reuseport).

       int bpf_sk_assign(struct bpf_sk_lookup  *ctx,  struct  bpf_sock
       *sk, u64 flags)

              Description
                     Helper  is  overloaded  depending  on BPF program
                     type.     This     description     applies     to
                     BPF_PROG_TYPE_SK_LOOKUP programs.

                     Select the sk as a result of a socket lookup.

                     For  the  operation to succeed passed socket must
                     be compatible with the  packet  description  pro‐
                     vided by the ctx object.

                     L4  protocol (IPPROTO_TCP or IPPROTO_UDP) must be
                     an exact  match.  While  IP  family  (AF_INET  or
                     AF_INET6)  must be compatible, that is IPv6 sock‐
                     ets that are not v6-only can be selected for IPv4
                     packets.

                     Only TCP listeners and UDP sockets, that is sock‐
                     ets which have SOCK_RCU_FREE  flag  set,  can  be
                     selected.

                     The flags argument must be zero.

              Return 0  on  success,  or  a  negative errno in case of
                     failure.

                     -EAFNOSUPPORT is socket  family  (sk->family)  is
                     not compatible with packet family (ctx->family).

                     -EINVAL if unsupported flags were specified.

                     -EPROTOTYPE  if socket L4 protocol (sk->protocol)
                     doesn't match packet protocol (ctx->protocol).

                     -ESOCKTNOSUPPORT if socket does not use RCU free‐
                     ing.

But it would be helpful to hear what others think about it.