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@xxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxx>
