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

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

On 4/6/24 11:47 AM, Andrii Nakryiko wrote:
> On Sat, Apr 6, 2024 at 9:04 AM 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.
>>
>> Reviewed-by: John Fastabend <john.fastabend@gmail.com>
>> 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            | 270 ++++++++++++++++++++++++++++++---
>>   tools/include/uapi/linux/bpf.h |   5 +
>>   6 files changed, 277 insertions(+), 17 deletions(-)
>>
> Please check bpf_prog_attach_check_attach_type(), it probably should
> be updated as well. Other than that looks good.

We are fine here. In function attach_type_to_prog_type(), we already
have checking:
         case BPF_SK_MSG_VERDICT:
                 return BPF_PROG_TYPE_SK_MSG;
         case BPF_SK_SKB_STREAM_PARSER:
         case BPF_SK_SKB_STREAM_VERDICT:
         case BPF_SK_SKB_VERDICT:
                 return BPF_PROG_TYPE_SK_SKB;

>
> [...]
>
>>   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;
>>
>> -       ret = sock_map_prog_lookup(map, &pprog, which);
>> +       ret = sock_map_prog_link_lookup(map, &pprog, &plink, NULL, link && !prog, which);
>>          if (ret)
>> -               return ret;
>> +               goto out;
> probably could have kept `return ret;` here?
>
>> -       if (old)
>> -               return psock_replace_prog(pprog, prog, old);
>> +       if (old) {
>> +               ret = psock_replace_prog(pprog, prog, old);
>> +               if (!ret)
>> +                       *plink = NULL;
>> +       } else {
>> +               psock_set_prog(pprog, prog);
>> +               if (link)
>> +                       *plink = link;
>> +       }
>>
>> -       psock_set_prog(pprog, prog);
>> -       return 0;
>> +out:
> and wouldn't need out: then

Ack. I can make this change.

>
>> +       return ret;
>>   }
>>
> [...]