On Wed, Apr 3, 2024 at 7:53 PM Yonghong Song <yonghong.song@xxxxxxxxx> 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(-)
>
[...]
> @@ -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?
> +{
> + 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)
> +
> +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
> +
> + 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
> + if (!ret) {
> + bpf_prog_inc(prog);
> + old = xchg(&link->prog, prog);
> + bpf_prog_put(old);
> + }
> + mutex_unlock(&sockmap_mutex);
> + return ret;
> +}
> +
[...]
