On 4/5/24 1:12 PM, Andrii Nakryiko wrote:
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?
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;
+}
+
[...]
