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

[John Fastabend <john.fastabend@gmail.com>]

Yonghong Song wrote:
> 
> On 4/5/24 9:23 AM, John Fastabend wrote:
> > Yonghong Song wrote:
> >> On 4/5/24 8:19 AM, John Fastabend wrote:
> >>> Yonghong Song 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(-)
> >>>>
> >>> LGTM one question below.
> >>>
> >>>> +/* Handle the following two cases:
> >>>> + * case 1: link != NULL, prog != NULL, old != NULL
> >>>> + * case 2: link != NULL, prog != NULL, old == NULL
> >>>> + */
> >>>> +static int sock_map_link_update_prog(struct bpf_link *link,
> >>>> +				     struct bpf_prog *prog,
> >>>> +				     struct bpf_prog *old)
> >>>> +{
> >>>> +	const struct sockmap_link *sockmap_link = container_of(link, struct sockmap_link, link);
> >>>> +	struct bpf_prog **pprog;
> >>>> +	struct bpf_link **plink;
> >>>> +	int ret = 0;
> >>>> +
> >>>> +	mutex_lock(&sockmap_mutex);
> >>>> +
> >>>> +	/* If old prog is not NULL, ensure old prog is the same as link->prog. */
> >>>> +	if (old && link->prog != old) {
> >>>> +		ret = -EINVAL;
> >>>> +		goto out;
> >>>> +	}
> >>>> +	/* Ensure link->prog has the same type/attach_type as the new prog. */
> >>>> +	if (link->prog->type != prog->type ||
> >>>> +	    link->prog->expected_attach_type != prog->expected_attach_type) {
> >>>> +		ret = -EINVAL;
> >>>> +		goto out;
> >>>> +	}
> >>>> +
> >>>> +	ret = sock_map_prog_lookup(sockmap_link->map, &pprog,
> >>>> +				   sockmap_link->attach_type);
> >>>> +	if (ret)
> >>>> +		goto out;
> >>>> +
> >>>> +	/* Ensure the same link between the one in map and the passed-in. */
> >>>> +	ret = sock_map_link_lookup(sockmap_link->map, &plink, link, false,
> >>>> +				   sockmap_link->attach_type);
> >>>> +	if (ret)
> >>>> +		goto out;
> >>>> +
> >>>> +	if (old) {
> >>>> +		ret = psock_replace_prog(pprog, prog, old);
> >>>> +		goto out;
> >>>> +	}
> >>>> +
> >>>> +	psock_set_prog(pprog, prog);
> >>>> +
> >>>> +out:
> >>>> +	if (!ret) {
> >>>> +		bpf_prog_inc(prog);
> >>>> +		old = xchg(&link->prog, prog);
> >>>> +		bpf_prog_put(old);
> >>> Need to check old? I don't think we can clal bpf_prog_put on null?
> >>>
> >>>     if (old)
> >>>        bpf_prog_put(old)
> >> The 'old' here represents the *old* link->prog program and
> >> link->prog should not be NULL at this point.
> > Ah ok. Maybe instead of using the input old var make it
> > explicit?
> >
> >      if (!ret) {
> >         struct bpf_prog *old_link;
> >
> >         bpf_prog_inc(prog);
> >         old_link = xchg(&link->prog, prog);
> >         bpf_prog_put(old)
> >      }
> >
> > Is a bit more obious to me at least. Up to you I have a slight preference
> > for the explicit more verbose above.
> 
> Regarding naming convention, yes, it is hard. My above code similar to
> kernel/bpf/net_namespace.c:
> 
> static int bpf_netns_link_update_prog(struct bpf_link *link,
>                                        struct bpf_prog *new_prog,
>                                        struct bpf_prog *old_prog)
> {
>          struct bpf_netns_link *net_link =
>                  container_of(link, struct bpf_netns_link, link);
>          enum netns_bpf_attach_type type = net_link->netns_type;
>          struct bpf_prog_array *run_array;
>          struct net *net;
>          int idx, ret;
>          
>          if (old_prog && old_prog != link->prog)
>                  return -EPERM;
> 	...
> 	old_prog = xchg(&link->prog, new_prog);
>          bpf_prog_put(old_prog);
> 	...
> }
> 
> The 'old_prog' is reused in the above.
> 
> I am okay to change
> 	old = xchg(&link->prog, prog);
> to
> 	old_link_prog = xchg(&link->prog, prog);
> 
> in next revision (if requested or additional changes needed)
> or as a followup.

I'm good with this series as is LGTM. We can do a follow up if we want.
Although the xchg is exactly one line above so I'm not sure its even necessary.

> 
> >
> > Otherwise for the series.
> >
> > Reviewed-by: John Fastabend <john.fastabend@gmail.com>