Re: [PATCH bpf-next v4 1/5] bpf: Add bpf_link support for sk_msg and sk_skb progs

[Yonghong Song <yonghong.song@linux.dev>]

On 4/5/24 1:12 PM, Andrii Nakryiko wrote:
> On Wed, Apr 3, 2024 at 7:53 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Add bpf_link support for sk_msg and sk_skb programs. We have an
>> internal request to support bpf_link for sk_msg programs so user
>> space can have a uniform handling with bpf_link based libbpf
>> APIs. Using bpf_link based libbpf API also has a benefit which
>> makes system robust by decoupling prog life cycle and
>> attachment life cycle.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   include/linux/bpf.h            |   6 +
>>   include/linux/skmsg.h          |   4 +
>>   include/uapi/linux/bpf.h       |   5 +
>>   kernel/bpf/syscall.c           |   4 +
>>   net/core/sock_map.c            | 268 ++++++++++++++++++++++++++++++++-
>>   tools/include/uapi/linux/bpf.h |   5 +
>>   6 files changed, 284 insertions(+), 8 deletions(-)
>>
> [...]
>
>> @@ -103,7 +111,7 @@ int sock_map_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype)
>>                  goto put_prog;
>>          }
>>
>> -       ret = sock_map_prog_update(map, NULL, prog, attr->attach_type);
>> +       ret = sock_map_prog_update(map, NULL, prog, NULL, attr->attach_type);
>>   put_prog:
>>          bpf_prog_put(prog);
>>   put_map:
>> @@ -1488,21 +1496,79 @@ static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
>>          return 0;
>>   }
>>
>> +static int sock_map_link_lookup(struct bpf_map *map, struct bpf_link ***plink,
>> +                               struct bpf_link *link, bool skip_check, u32 which)
> why not combine prog + link into a single lookup? also it seems like
> sock_map_prog_lookup has some additional EBUSY conditions, do we need
> to replicate them here?

I can combine them together.

>
>> +{
>> +       struct sk_psock_progs *progs = sock_map_progs(map);
>> +       struct bpf_link **cur_plink;
>> +
>> +       switch (which) {
>> +       case BPF_SK_MSG_VERDICT:
>> +               cur_plink = &progs->msg_parser_link;
>> +               break;
>> +#if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
>> +       case BPF_SK_SKB_STREAM_PARSER:
>> +               cur_plink = &progs->stream_parser_link;
>> +               break;
>> +#endif
>> +       case BPF_SK_SKB_STREAM_VERDICT:
>> +               cur_plink = &progs->stream_verdict_link;
>> +               break;
>> +       case BPF_SK_SKB_VERDICT:
>> +               cur_plink = &progs->skb_verdict_link;
>> +               break;
>> +       default:
>> +               return -EOPNOTSUPP;
>> +       }
>> +
>> +       if (!skip_check && ((!link && *cur_plink) || (link && link != *cur_plink)))
>> +               return -EBUSY;
>> +
>> +       *plink = cur_plink;
>> +       return 0;
>> +}
>> +
>> +/* Handle the following four cases:
>> + * prog_attach: prog != NULL, old == NULL, link == NULL
>> + * prog_detach: prog == NULL, old != NULL, link == NULL
>> + * link_attach: prog != NULL, old == NULL, link != NULL
>> + * link_detach: prog == NULL, old != NULL, link != NULL
>> + */
>>   static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
>> -                               struct bpf_prog *old, u32 which)
>> +                               struct bpf_prog *old, struct bpf_link *link,
>> +                               u32 which)
>>   {
>>          struct bpf_prog **pprog;
>> +       struct bpf_link **plink;
>>          int ret;
>>
>> +       mutex_lock(&sockmap_mutex);
>> +
>>          ret = sock_map_prog_lookup(map, &pprog, which);
>>          if (ret)
>> -               return ret;
>> +               goto out;
>>
>> -       if (old)
>> -               return psock_replace_prog(pprog, prog, old);
>> +       if (!link || prog)
>> +               ret = sock_map_link_lookup(map, &plink, NULL, false, which);
>> +       else
>> +               ret = sock_map_link_lookup(map, &plink, NULL, true, which);
>> +       if (ret)
>> +               goto out;
>> +
>> +       if (old) {
>> +               ret = psock_replace_prog(pprog, prog, old);
>> +               if (!ret)
>> +                       *plink = NULL;
>> +               goto out;
>> +       }
>>
>>          psock_set_prog(pprog, prog);
>> -       return 0;
>> +       if (link)
>> +               *plink = link;
> nit: feels more natural to do
>
> if (old) {
>      psock_replace_prog(...)
> } else {
>      psock_set_prog(...)
> }
>
> it's two alternatives, not one unlikely vs one main use case (but it's minor)

Ack indeed better.

>
>> +
>> +out:
>> +       mutex_unlock(&sockmap_mutex);
>> +       return ret;
>>   }
>>
>>   int sock_map_bpf_prog_query(const union bpf_attr *attr,
>> @@ -1657,6 +1723,192 @@ void sock_map_close(struct sock *sk, long timeout)
>>   }
>>   EXPORT_SYMBOL_GPL(sock_map_close);
>>
>> +struct sockmap_link {
>> +       struct bpf_link link;
>> +       struct bpf_map *map;
>> +       enum bpf_attach_type attach_type;
>> +};
>> +
>> +static void sock_map_link_release(struct bpf_link *link)
>> +{
>> +       struct sockmap_link *sockmap_link = container_of(link, struct sockmap_link, link);
>> +
>> +       if (sockmap_link->map) {
> nit: if (!sockmap_link->map) return;
>
> and reduce nesting of everything else
>
>> +               WARN_ON_ONCE(sock_map_prog_update(sockmap_link->map, NULL, link->prog, link,
>> +                                                 sockmap_link->attach_type));
> I think sockmap_link->map access in general has to be always protected
> my sockmap_mutex (I'd do that even for the if above), because it can
> race with force-detach logic at least

Ack. will fix this.

>
>> +
>> +               mutex_lock(&sockmap_mutex);
>> +               bpf_map_put_with_uref(sockmap_link->map);
>> +               sockmap_link->map = NULL;
>> +               mutex_unlock(&sockmap_mutex);
>> +       }
>> +}
>> +
> [...]
>
>> +       if (old) {
>> +               ret = psock_replace_prog(pprog, prog, old);
>> +               goto out;
>> +       }
>> +
>> +       psock_set_prog(pprog, prog);
>> +
>> +out:
> same nit, feels like
>
> if (old) /* replace */ else /* set */ is more natural, and then you
> can move xchg logic before out: knowing that it's the only success
> case

Ack. will do.

>
>> +       if (!ret) {
>> +               bpf_prog_inc(prog);
>> +               old = xchg(&link->prog, prog);
>> +               bpf_prog_put(old);
>> +       }
>> +       mutex_unlock(&sockmap_mutex);
>> +       return ret;
>> +}
>> +
> [...]