[RFC PATCH bpf-next 0/5] Add bpf_link support for sk_msg prog

[RFC PATCH bpf-next 0/5] Add bpf_link support for sk_msg prog

[Yonghong Song]

One of our internal services started to use sk_msg program and currently
it used existing prog attach/detach2 as demonstrated in selftests.
But attach/detach of all other bpf programs are based on bpf_link.
Consistent attach/detach APIs for all programs will make things easy to
undersand and less error prone. So this patch added bpf_link
support for BPF_PROG_TYPE_SK_MSG.

I marked the patch as RFC as not all functionality are covered
by tests yet, e.g. update_prog(). Maybe somebody can suggest
an existing test which I can look into.
Or maybe some other tests need to be added as well.

Yonghong Song (5):
  bpf: Add link support for sk_msg prog
  libbpf: Refactor bpf_program_attach_fd()
  libbpf: Add link support for BPF_PROG_TYPE_SK_MSG
  bpftool: Add link dump support for BPF_LINK_TYPE_SK_MSG
  selftests/bpf: Add some tests with new bpf_program__attach_sk_msg()
    API

 include/linux/bpf.h                           |  13 ++
 include/uapi/linux/bpf.h                      |   5 +
 kernel/bpf/syscall.c                          |   3 +
 net/core/skmsg.c                              | 153 ++++++++++++++++++
 net/core/sock_map.c                           |   6 +-
 tools/bpf/bpftool/link.c                      |   8 +
 tools/include/uapi/linux/bpf.h                |   5 +
 tools/lib/bpf/libbpf.c                        |  26 ++-
 tools/lib/bpf/libbpf.h                        |   3 +
 tools/lib/bpf/libbpf.map                      |   1 +
 .../selftests/bpf/prog_tests/sockmap_basic.c  |  27 ++++
 .../selftests/bpf/prog_tests/sockmap_listen.c |  38 +++++
 12 files changed, 279 insertions(+), 9 deletions(-)

-- 
2.43.0

[RFC PATCH bpf-next 1/5] bpf: Add link support for sk_msg prog

[Yonghong Song]

Add link support for sk_msg program. This will make user space
easy to manage as most common used programs have alrady have
link support.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 include/linux/bpf.h            |  13 +++
 include/uapi/linux/bpf.h       |   5 ++
 kernel/bpf/syscall.c           |   3 +
 net/core/skmsg.c               | 153 +++++++++++++++++++++++++++++++++
 net/core/sock_map.c            |   6 +-
 tools/include/uapi/linux/bpf.h |   5 ++
 6 files changed, 181 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 785660810e6a..a3112a295f5c 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2982,10 +2982,14 @@ int sock_map_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype);
 int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
 int sock_map_bpf_prog_query(const union bpf_attr *attr,
 			    union bpf_attr __user *uattr);
+int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+			 struct bpf_prog *old, u32 which);
 
 void sock_map_unhash(struct sock *sk);
 void sock_map_destroy(struct sock *sk);
 void sock_map_close(struct sock *sk, long timeout);
+
+int bpf_skmsg_link_create(const union bpf_attr *attr, struct bpf_prog *prog);
 #else
 static inline int bpf_dev_bound_kfunc_check(struct bpf_verifier_log *log,
 					    struct bpf_prog_aux *prog_aux)
@@ -3080,6 +3084,15 @@ static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
 {
 	return -EINVAL;
 }
+int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+			 struct bpf_prog *old, u32 which)
+{
+	return -EOPNOTSUPP;
+}
+int bpf_skmsg_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	return -EOPNOTSUPP;
+}
 #endif /* CONFIG_BPF_SYSCALL */
 #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
 
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index a241f407c234..c7d2a5fcf37a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1129,6 +1129,7 @@ enum bpf_link_type {
 	BPF_LINK_TYPE_TCX = 11,
 	BPF_LINK_TYPE_UPROBE_MULTI = 12,
 	BPF_LINK_TYPE_NETKIT = 13,
+	BPF_LINK_TYPE_SK_MSG = 14,
 	__MAX_BPF_LINK_TYPE,
 };
 
@@ -6699,6 +6700,10 @@ struct bpf_link_info {
 			__u32 ifindex;
 			__u32 attach_type;
 		} netkit;
+		struct {
+			__u32 map_id;
+			__u32 attach_type;
+		} skmsg;
 	};
 } __attribute__((aligned(8)));
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b2750b79ac80..7fd3e6c93612 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5155,6 +5155,9 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
 	case BPF_PROG_TYPE_SK_LOOKUP:
 		ret = netns_bpf_link_create(attr, prog);
 		break;
+	case BPF_PROG_TYPE_SK_MSG:
+		ret = bpf_skmsg_link_create(attr, prog);
+		break;
 #ifdef CONFIG_NET
 	case BPF_PROG_TYPE_XDP:
 		ret = bpf_xdp_link_attach(attr, prog);
diff --git a/net/core/skmsg.c b/net/core/skmsg.c
index 4d75ef9d24bf..2e3d15294966 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1256,3 +1256,156 @@ void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
 	sk->sk_data_ready = psock->saved_data_ready;
 	psock->saved_data_ready = NULL;
 }
+
+struct bpf_skmsg_link {
+	struct bpf_link link;
+	struct bpf_map *map;
+	enum bpf_attach_type attach_type;
+};
+
+static DEFINE_MUTEX(link_mutex);
+
+static struct bpf_skmsg_link *bpf_skmsg_link(const struct bpf_link *link)
+{
+	return container_of(link, struct bpf_skmsg_link, link);
+}
+
+static void bpf_skmsg_link_release(struct bpf_link *link)
+{
+	struct bpf_skmsg_link *skmsg_link = bpf_skmsg_link(link);
+
+	mutex_lock(&link_mutex);
+	if (skmsg_link->map) {
+		(void)sock_map_prog_update(skmsg_link->map, NULL, link->prog,
+					   skmsg_link->attach_type);
+		bpf_map_put_with_uref(skmsg_link->map);
+		skmsg_link->map = NULL;
+	}
+	mutex_unlock(&link_mutex);
+}
+
+static int bpf_skmsg_link_detach(struct bpf_link *link)
+{
+	bpf_skmsg_link_release(link);
+	return 0;
+}
+
+static void bpf_skmsg_link_dealloc(struct bpf_link *link)
+{
+	kfree(bpf_skmsg_link(link));
+}
+
+static int bpf_skmsg_link_update_prog(struct bpf_link *link,
+				      struct bpf_prog *new_prog,
+				      struct bpf_prog *old_prog)
+{
+	const struct bpf_skmsg_link *skmsg_link = bpf_skmsg_link(link);
+	int ret = 0;
+
+	mutex_lock(&link_mutex);
+	if (old_prog && link->prog != old_prog) {
+		ret = -EPERM;
+		goto out;
+	}
+
+	if (link->prog->type != new_prog->type) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = sock_map_prog_update(skmsg_link->map, new_prog, old_prog,
+				   skmsg_link->attach_type);
+	if (!ret)
+		bpf_prog_inc(new_prog);
+
+out:
+	mutex_unlock(&link_mutex);
+	return ret;
+}
+
+static int bpf_skmsg_link_fill_info(const struct bpf_link *link,
+				    struct bpf_link_info *info)
+{
+	const struct bpf_skmsg_link *skmsg_link = bpf_skmsg_link(link);
+	u32 map_id = 0;
+
+	mutex_lock(&link_mutex);
+	if (skmsg_link->map)
+		map_id = skmsg_link->map->id;
+	mutex_unlock(&link_mutex);
+
+	info->skmsg.map_id = map_id;
+	info->skmsg.attach_type = skmsg_link->attach_type;
+	return 0;
+}
+
+static void bpf_skmsg_link_show_fdinfo(const struct bpf_link *link,
+				       struct seq_file *seq)
+{
+	const struct bpf_skmsg_link *skmsg_link = bpf_skmsg_link(link);
+	u32 map_id = 0;
+
+	mutex_lock(&link_mutex);
+	if (skmsg_link->map)
+		map_id = skmsg_link->map->id;
+	mutex_unlock(&link_mutex);
+
+	seq_printf(seq, "map_id:\t%u\n", map_id);
+	seq_printf(seq, "attach_type:\t%u (...)\n", skmsg_link->attach_type);
+}
+
+static const struct bpf_link_ops bpf_skmsg_link_ops = {
+	.release = bpf_skmsg_link_release,
+	.dealloc = bpf_skmsg_link_dealloc,
+	.detach = bpf_skmsg_link_detach,
+	.update_prog = bpf_skmsg_link_update_prog,
+	.fill_link_info = bpf_skmsg_link_fill_info,
+	.show_fdinfo = bpf_skmsg_link_show_fdinfo,
+};
+
+int bpf_skmsg_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	struct bpf_link_primer link_primer;
+	struct bpf_skmsg_link *skmsg_link;
+	enum bpf_attach_type attach_type;
+	struct bpf_map *map;
+	int ret;
+
+	if (attr->link_create.flags)
+		return -EINVAL;
+
+	map = bpf_map_get_with_uref(attr->link_create.target_fd);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	skmsg_link = kzalloc(sizeof(*skmsg_link), GFP_USER);
+	if (!skmsg_link) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	attach_type = attr->link_create.attach_type;
+	bpf_link_init(&skmsg_link->link, BPF_LINK_TYPE_SK_MSG, &bpf_skmsg_link_ops, prog);
+	skmsg_link->map = map;
+	skmsg_link->attach_type = attach_type;
+
+	ret = bpf_link_prime(&skmsg_link->link, &link_primer);
+	if (ret) {
+		kfree(skmsg_link);
+		goto out;
+	}
+
+	ret = sock_map_prog_update(map, prog, NULL, attach_type);
+	if (ret) {
+		bpf_link_cleanup(&link_primer);
+		goto out;
+	}
+
+	bpf_prog_inc(prog);
+
+	return bpf_link_settle(&link_primer);
+
+out:
+	bpf_map_put_with_uref(map);
+	return ret;
+}
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 27d733c0f65e..63372bc368f1 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -24,8 +24,6 @@ struct bpf_stab {
 #define SOCK_CREATE_FLAG_MASK				\
 	(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
 
-static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
-				struct bpf_prog *old, u32 which);
 static struct sk_psock_progs *sock_map_progs(struct bpf_map *map);
 
 static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
@@ -1488,8 +1486,8 @@ static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
 	return 0;
 }
 
-static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
-				struct bpf_prog *old, u32 which)
+int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+			 struct bpf_prog *old, u32 which)
 {
 	struct bpf_prog **pprog;
 	int ret;
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a241f407c234..c7d2a5fcf37a 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1129,6 +1129,7 @@ enum bpf_link_type {
 	BPF_LINK_TYPE_TCX = 11,
 	BPF_LINK_TYPE_UPROBE_MULTI = 12,
 	BPF_LINK_TYPE_NETKIT = 13,
+	BPF_LINK_TYPE_SK_MSG = 14,
 	__MAX_BPF_LINK_TYPE,
 };
 
@@ -6699,6 +6700,10 @@ struct bpf_link_info {
 			__u32 ifindex;
 			__u32 attach_type;
 		} netkit;
+		struct {
+			__u32 map_id;
+			__u32 attach_type;
+		} skmsg;
 	};
 } __attribute__((aligned(8)));
 
-- 
2.43.0

Re: [RFC PATCH bpf-next 1/5] bpf: Add link support for sk_msg prog

[Andrii Nakryiko]

On Tue, Mar 5, 2024 at 12:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> Add link support for sk_msg program. This will make user space
> easy to manage as most common used programs have alrady have
> link support.

So we have:

SEC("sk_skb/stream_parser") mapping to SK_SKB/BPF_SK_SKB_STREAM_PARSER.
SEC("sk_skb/stream_verdict") mapping to SK_SKB/BPF_SK_SKB_STREAM_VERDICT.
SEC("sk_msg") mapping to SK_MSG/BPF_SK_MSG_VERDICT.

Are those all kind of in the same category and should we support link
for both SK_MSG and SK_SKB? I'm not too familiar, maybe John or
someone else can clarify.

>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  include/linux/bpf.h            |  13 +++
>  include/uapi/linux/bpf.h       |   5 ++
>  kernel/bpf/syscall.c           |   3 +
>  net/core/skmsg.c               | 153 +++++++++++++++++++++++++++++++++
>  net/core/sock_map.c            |   6 +-
>  tools/include/uapi/linux/bpf.h |   5 ++
>  6 files changed, 181 insertions(+), 4 deletions(-)
>

[...]

Re: [RFC PATCH bpf-next 1/5] bpf: Add link support for sk_msg prog

[Yonghong Song]

On 3/8/24 4:59 PM, Andrii Nakryiko wrote:
> On Tue, Mar 5, 2024 at 12:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Add link support for sk_msg program. This will make user space
>> easy to manage as most common used programs have alrady have
>> link support.
> So we have:
>
> SEC("sk_skb/stream_parser") mapping to SK_SKB/BPF_SK_SKB_STREAM_PARSER.
> SEC("sk_skb/stream_verdict") mapping to SK_SKB/BPF_SK_SKB_STREAM_VERDICT.
> SEC("sk_msg") mapping to SK_MSG/BPF_SK_MSG_VERDICT.
>
> Are those all kind of in the same category and should we support link
> for both SK_MSG and SK_SKB? I'm not too familiar, maybe John or
> someone else can clarify.

I can add sk_skb bpf_link support as well. Yes, sk_msg and sk_skb are
similar to each other.

>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   include/linux/bpf.h            |  13 +++
>>   include/uapi/linux/bpf.h       |   5 ++
>>   kernel/bpf/syscall.c           |   3 +
>>   net/core/skmsg.c               | 153 +++++++++++++++++++++++++++++++++
>>   net/core/sock_map.c            |   6 +-
>>   tools/include/uapi/linux/bpf.h |   5 ++
>>   6 files changed, 181 insertions(+), 4 deletions(-)
>>
> [...]

Re: [RFC PATCH bpf-next 1/5] bpf: Add link support for sk_msg prog

[Jakub Sitnicki]

On Fri, Mar 08, 2024 at 04:59 PM -08, Andrii Nakryiko wrote:
> On Tue, Mar 5, 2024 at 12:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> Add link support for sk_msg program. This will make user space
>> easy to manage as most common used programs have alrady have
>> link support.
>
> So we have:
>
> SEC("sk_skb/stream_parser") mapping to SK_SKB/BPF_SK_SKB_STREAM_PARSER.
> SEC("sk_skb/stream_verdict") mapping to SK_SKB/BPF_SK_SKB_STREAM_VERDICT.
> SEC("sk_msg") mapping to SK_MSG/BPF_SK_MSG_VERDICT.
>
> Are those all kind of in the same category and should we support link
> for both SK_MSG and SK_SKB? I'm not too familiar, maybe John or
> someone else can clarify.

We also have the most recent SK_SKB/BPF_SK_SKB_VERDICT [1], which is
what one would use instead of SK_SKB/BPF_SK_SKB_STREAM_* prog pair when
chopping the stream of bytes into frames is not needed. For instance,
because we're redirecting from a UDP socket.

I have a cheatsheet that clarifies in which redirect-with-sockmap
configuration what program type + attach type is used [2].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a7ba4558e69a3c2ae4ca521f015832ef44799538
[2] https://github.com/jsitnicki/srecon-2023-sockmap/blob/main/sockmap-cheatsheet.png

Re: [RFC PATCH bpf-next 1/5] bpf: Add link support for sk_msg prog

[Yonghong Song]

On 3/10/24 12:23 PM, Jakub Sitnicki wrote:
> On Fri, Mar 08, 2024 at 04:59 PM -08, Andrii Nakryiko wrote:
>> On Tue, Mar 5, 2024 at 12:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>> Add link support for sk_msg program. This will make user space
>>> easy to manage as most common used programs have alrady have
>>> link support.
>> So we have:
>>
>> SEC("sk_skb/stream_parser") mapping to SK_SKB/BPF_SK_SKB_STREAM_PARSER.
>> SEC("sk_skb/stream_verdict") mapping to SK_SKB/BPF_SK_SKB_STREAM_VERDICT.
>> SEC("sk_msg") mapping to SK_MSG/BPF_SK_MSG_VERDICT.
>>
>> Are those all kind of in the same category and should we support link
>> for both SK_MSG and SK_SKB? I'm not too familiar, maybe John or
>> someone else can clarify.
> We also have the most recent SK_SKB/BPF_SK_SKB_VERDICT [1], which is
> what one would use instead of SK_SKB/BPF_SK_SKB_STREAM_* prog pair when
> chopping the stream of bytes into frames is not needed. For instance,
> because we're redirecting from a UDP socket.
>
> I have a cheatsheet that clarifies in which redirect-with-sockmap
> configuration what program type + attach type is used [2].
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a7ba4558e69a3c2ae4ca521f015832ef44799538
> [2] https://github.com/jsitnicki/srecon-2023-sockmap/blob/main/sockmap-cheatsheet.png

Thanks. I will take a look. Yes, agree, we only need to implement
the recommended one, which is SK_SKB/BPF_SK_SKB_VERDICT if I understand correctly.

Re: [RFC PATCH bpf-next 1/5] bpf: Add link support for sk_msg prog

[Jiri Olsa]

On Tue, Mar 05, 2024 at 12:22:00PM -0800, Yonghong Song wrote:

SNIP

> +
> +int bpf_skmsg_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
> +{
> +	struct bpf_link_primer link_primer;
> +	struct bpf_skmsg_link *skmsg_link;
> +	enum bpf_attach_type attach_type;
> +	struct bpf_map *map;
> +	int ret;
> +
> +	if (attr->link_create.flags)
> +		return -EINVAL;
> +
> +	map = bpf_map_get_with_uref(attr->link_create.target_fd);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	skmsg_link = kzalloc(sizeof(*skmsg_link), GFP_USER);
> +	if (!skmsg_link) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	attach_type = attr->link_create.attach_type;
> +	bpf_link_init(&skmsg_link->link, BPF_LINK_TYPE_SK_MSG, &bpf_skmsg_link_ops, prog);
> +	skmsg_link->map = map;
> +	skmsg_link->attach_type = attach_type;
> +
> +	ret = bpf_link_prime(&skmsg_link->link, &link_primer);
> +	if (ret) {
> +		kfree(skmsg_link);
> +		goto out;
> +	}
> +
> +	ret = sock_map_prog_update(map, prog, NULL, attach_type);
> +	if (ret) {
> +		bpf_link_cleanup(&link_primer);
> +		goto out;
> +	}
> +
> +	bpf_prog_inc(prog);

there's already prog ref taken in link_create, is this needed?
also I might be missing some skmsg logic, but I can't see thi
being released

jirka

> +
> +	return bpf_link_settle(&link_primer);
> +
> +out:
> +	bpf_map_put_with_uref(map);
> +	return ret;
> +}
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 27d733c0f65e..63372bc368f1 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -24,8 +24,6 @@ struct bpf_stab {
>  #define SOCK_CREATE_FLAG_MASK				\
>  	(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
>  
> -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> -				struct bpf_prog *old, u32 which);
>  static struct sk_psock_progs *sock_map_progs(struct bpf_map *map);
>  
>  static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
> @@ -1488,8 +1486,8 @@ static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
>  	return 0;
>  }
>  
> -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> -				struct bpf_prog *old, u32 which)
> +int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> +			 struct bpf_prog *old, u32 which)
>  {
>  	struct bpf_prog **pprog;
>  	int ret;
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index a241f407c234..c7d2a5fcf37a 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -1129,6 +1129,7 @@ enum bpf_link_type {
>  	BPF_LINK_TYPE_TCX = 11,
>  	BPF_LINK_TYPE_UPROBE_MULTI = 12,
>  	BPF_LINK_TYPE_NETKIT = 13,
> +	BPF_LINK_TYPE_SK_MSG = 14,
>  	__MAX_BPF_LINK_TYPE,
>  };
>  
> @@ -6699,6 +6700,10 @@ struct bpf_link_info {
>  			__u32 ifindex;
>  			__u32 attach_type;
>  		} netkit;
> +		struct {
> +			__u32 map_id;
> +			__u32 attach_type;
> +		} skmsg;
>  	};
>  } __attribute__((aligned(8)));
>  
> -- 
> 2.43.0
> 
> 

Re: [RFC PATCH bpf-next 1/5] bpf: Add link support for sk_msg prog

[Yonghong Song]

On 3/11/24 1:30 AM, Jiri Olsa wrote:
> On Tue, Mar 05, 2024 at 12:22:00PM -0800, Yonghong Song wrote:
>
> SNIP
>
>> +
>> +int bpf_skmsg_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
>> +{
>> +	struct bpf_link_primer link_primer;
>> +	struct bpf_skmsg_link *skmsg_link;
>> +	enum bpf_attach_type attach_type;
>> +	struct bpf_map *map;
>> +	int ret;
>> +
>> +	if (attr->link_create.flags)
>> +		return -EINVAL;
>> +
>> +	map = bpf_map_get_with_uref(attr->link_create.target_fd);
>> +	if (IS_ERR(map))
>> +		return PTR_ERR(map);
>> +
>> +	skmsg_link = kzalloc(sizeof(*skmsg_link), GFP_USER);
>> +	if (!skmsg_link) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	attach_type = attr->link_create.attach_type;
>> +	bpf_link_init(&skmsg_link->link, BPF_LINK_TYPE_SK_MSG, &bpf_skmsg_link_ops, prog);
>> +	skmsg_link->map = map;
>> +	skmsg_link->attach_type = attach_type;
>> +
>> +	ret = bpf_link_prime(&skmsg_link->link, &link_primer);
>> +	if (ret) {
>> +		kfree(skmsg_link);
>> +		goto out;
>> +	}
>> +
>> +	ret = sock_map_prog_update(map, prog, NULL, attach_type);
>> +	if (ret) {
>> +		bpf_link_cleanup(&link_primer);
>> +		goto out;
>> +	}
>> +
>> +	bpf_prog_inc(prog);
> there's already prog ref taken in link_create, is this needed?
> also I might be missing some skmsg logic, but I can't see thi
> being released

Here, we are trying to do create/attach.
The prog reference count will be decremented during detach
(sock_map_prog_detach), so we need to increase prog ref count here.

Another thing, it is possible a attached prog is swapped out.
In current implementation, ref count will be decremented for
the prog swapped out. So increasing ref count for prog here
is needed to balance that ref count decrement.

>
> jirka
>
>> +
>> +	return bpf_link_settle(&link_primer);
>> +
>> +out:
>> +	bpf_map_put_with_uref(map);
>> +	return ret;
>> +}
[...]

[RFC PATCH bpf-next 2/5] libbpf: Refactor bpf_program_attach_fd()

[Yonghong Song]

Refactor function bpf_program_attach_fd() to provide a helper function
which has attach_type as one of input parameters. This will make later
libbpf change easier to understand.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 tools/lib/bpf/libbpf.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6c2979f1b471..97b573516675 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -12151,11 +12151,10 @@ static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_li
 }
 
 static struct bpf_link *
-bpf_program_attach_fd(const struct bpf_program *prog,
-		      int target_fd, const char *target_name,
-		      const struct bpf_link_create_opts *opts)
+__bpf_program_attach_fd(const struct bpf_program *prog, int target_fd,
+			enum bpf_attach_type attach_type, const char *target_name,
+			const struct bpf_link_create_opts *opts)
 {
-	enum bpf_attach_type attach_type;
 	char errmsg[STRERR_BUFSIZE];
 	struct bpf_link *link;
 	int prog_fd, link_fd;
@@ -12171,7 +12170,6 @@ bpf_program_attach_fd(const struct bpf_program *prog,
 		return libbpf_err_ptr(-ENOMEM);
 	link->detach = &bpf_link__detach_fd;
 
-	attach_type = bpf_program__expected_attach_type(prog);
 	link_fd = bpf_link_create(prog_fd, target_fd, attach_type, opts);
 	if (link_fd < 0) {
 		link_fd = -errno;
@@ -12185,6 +12183,16 @@ bpf_program_attach_fd(const struct bpf_program *prog,
 	return link;
 }
 
+static struct bpf_link *
+bpf_program_attach_fd(const struct bpf_program *prog,
+		      int target_fd, const char *target_name,
+		      const struct bpf_link_create_opts *opts)
+{
+	return __bpf_program_attach_fd(prog, target_fd,
+				       bpf_program__expected_attach_type(prog),
+				       target_name, opts);
+}
+
 struct bpf_link *
 bpf_program__attach_cgroup(const struct bpf_program *prog, int cgroup_fd)
 {
-- 
2.43.0

Re: [RFC PATCH bpf-next 2/5] libbpf: Refactor bpf_program_attach_fd()

[Andrii Nakryiko]

On Tue, Mar 5, 2024 at 12:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> Refactor function bpf_program_attach_fd() to provide a helper function
> which has attach_type as one of input parameters. This will make later
> libbpf change easier to understand.
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6c2979f1b471..97b573516675 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -12151,11 +12151,10 @@ static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_li
>  }
>
>  static struct bpf_link *
> -bpf_program_attach_fd(const struct bpf_program *prog,
> -                     int target_fd, const char *target_name,
> -                     const struct bpf_link_create_opts *opts)
> +__bpf_program_attach_fd(const struct bpf_program *prog, int target_fd,

I hope we won't need this patch at all (see my comment on later
patch), but if we do need to do this refactoring then a) let's not
used underscore-prefixed names, this is not a common practice in
libbpf code base and b) I think we can just update all callers of
bpf_program_attach_fd() to pass attach_type directly, there are just 6
of them.

> +                       enum bpf_attach_type attach_type, const char *target_name,
> +                       const struct bpf_link_create_opts *opts)
>  {
> -       enum bpf_attach_type attach_type;
>         char errmsg[STRERR_BUFSIZE];
>         struct bpf_link *link;
>         int prog_fd, link_fd;
> @@ -12171,7 +12170,6 @@ bpf_program_attach_fd(const struct bpf_program *prog,
>                 return libbpf_err_ptr(-ENOMEM);
>         link->detach = &bpf_link__detach_fd;
>
> -       attach_type = bpf_program__expected_attach_type(prog);
>         link_fd = bpf_link_create(prog_fd, target_fd, attach_type, opts);
>         if (link_fd < 0) {
>                 link_fd = -errno;
> @@ -12185,6 +12183,16 @@ bpf_program_attach_fd(const struct bpf_program *prog,
>         return link;
>  }
>
> +static struct bpf_link *
> +bpf_program_attach_fd(const struct bpf_program *prog,
> +                     int target_fd, const char *target_name,
> +                     const struct bpf_link_create_opts *opts)
> +{
> +       return __bpf_program_attach_fd(prog, target_fd,
> +                                      bpf_program__expected_attach_type(prog),
> +                                      target_name, opts);
> +}
> +
>  struct bpf_link *
>  bpf_program__attach_cgroup(const struct bpf_program *prog, int cgroup_fd)
>  {
> --
> 2.43.0
>

Re: [RFC PATCH bpf-next 2/5] libbpf: Refactor bpf_program_attach_fd()

[Yonghong Song]

On 3/8/24 5:02 PM, Andrii Nakryiko wrote:
> On Tue, Mar 5, 2024 at 12:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Refactor function bpf_program_attach_fd() to provide a helper function
>> which has attach_type as one of input parameters. This will make later
>> libbpf change easier to understand.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/lib/bpf/libbpf.c | 18 +++++++++++++-----
>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 6c2979f1b471..97b573516675 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -12151,11 +12151,10 @@ static int attach_lsm(const struct bpf_program *prog, long cookie, struct bpf_li
>>   }
>>
>>   static struct bpf_link *
>> -bpf_program_attach_fd(const struct bpf_program *prog,
>> -                     int target_fd, const char *target_name,
>> -                     const struct bpf_link_create_opts *opts)
>> +__bpf_program_attach_fd(const struct bpf_program *prog, int target_fd,
> I hope we won't need this patch at all (see my comment on later
> patch), but if we do need to do this refactoring then a) let's not
> used underscore-prefixed names, this is not a common practice in
> libbpf code base and b) I think we can just update all callers of
> bpf_program_attach_fd() to pass attach_type directly, there are just 6
> of them.

I thought about this 'update all callers of bpf_program_attach_fd
' but did not do that since it needs to update all callers. But since
you suggest this I certainly can do this.

>
>> +                       enum bpf_attach_type attach_type, const char *target_name,
>> +                       const struct bpf_link_create_opts *opts)
>>   {
>> -       enum bpf_attach_type attach_type;
>>          char errmsg[STRERR_BUFSIZE];
>>          struct bpf_link *link;
>>          int prog_fd, link_fd;
>> @@ -12171,7 +12170,6 @@ bpf_program_attach_fd(const struct bpf_program *prog,
>>                  return libbpf_err_ptr(-ENOMEM);
>>          link->detach = &bpf_link__detach_fd;
>>
>> -       attach_type = bpf_program__expected_attach_type(prog);
>>          link_fd = bpf_link_create(prog_fd, target_fd, attach_type, opts);
>>          if (link_fd < 0) {
>>                  link_fd = -errno;
>> @@ -12185,6 +12183,16 @@ bpf_program_attach_fd(const struct bpf_program *prog,
>>          return link;
>>   }
>>
>> +static struct bpf_link *
>> +bpf_program_attach_fd(const struct bpf_program *prog,
>> +                     int target_fd, const char *target_name,
>> +                     const struct bpf_link_create_opts *opts)
>> +{
>> +       return __bpf_program_attach_fd(prog, target_fd,
>> +                                      bpf_program__expected_attach_type(prog),
>> +                                      target_name, opts);
>> +}
>> +
>>   struct bpf_link *
>>   bpf_program__attach_cgroup(const struct bpf_program *prog, int cgroup_fd)
>>   {
>> --
>> 2.43.0
>>

[RFC PATCH bpf-next 3/5] libbpf: Add link support for BPF_PROG_TYPE_SK_MSG

[Yonghong Song]

Introduce a libbpf API bpf_program__attach_sk_msg() which allows
user to get a bpf_link. The API makes auto-deletion easier and
also allows user space application easier as link based APIs
are used for all programs.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 tools/lib/bpf/libbpf.c   | 8 ++++++++
 tools/lib/bpf/libbpf.h   | 3 +++
 tools/lib/bpf/libbpf.map | 1 +
 3 files changed, 12 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 97b573516675..b3982bb3f979 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -149,6 +149,7 @@ static const char * const link_type_name[] = {
 	[BPF_LINK_TYPE_TCX]			= "tcx",
 	[BPF_LINK_TYPE_UPROBE_MULTI]		= "uprobe_multi",
 	[BPF_LINK_TYPE_NETKIT]			= "netkit",
+	[BPF_LINK_TYPE_SK_MSG]			= "sk_msg",
 };
 
 static const char * const map_type_name[] = {
@@ -12280,6 +12281,13 @@ bpf_program__attach_netkit(const struct bpf_program *prog, int ifindex,
 	return bpf_program_attach_fd(prog, ifindex, "netkit", &link_create_opts);
 }
 
+struct bpf_link *
+bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd,
+			   enum bpf_attach_type attach_type)
+{
+	return __bpf_program_attach_fd(prog, map_fd, attach_type, "sk_msg", NULL);
+}
+
 struct bpf_link *bpf_program__attach_freplace(const struct bpf_program *prog,
 					      int target_fd,
 					      const char *attach_func_name)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 5723cbbfcc41..c8448f05e8d6 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -786,6 +786,9 @@ bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd);
 LIBBPF_API struct bpf_link *
 bpf_program__attach_xdp(const struct bpf_program *prog, int ifindex);
 LIBBPF_API struct bpf_link *
+bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd,
+			   enum bpf_attach_type attach_type);
+LIBBPF_API struct bpf_link *
 bpf_program__attach_freplace(const struct bpf_program *prog,
 			     int target_fd, const char *attach_func_name);
 
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 86804fd90dd1..c59986c6dbc5 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -410,6 +410,7 @@ LIBBPF_1.3.0 {
 
 LIBBPF_1.4.0 {
 	global:
+		bpf_program__attach_sk_msg;
 		bpf_token_create;
 		btf__new_split;
 		btf_ext__raw_data;
-- 
2.43.0

Re: [RFC PATCH bpf-next 3/5] libbpf: Add link support for BPF_PROG_TYPE_SK_MSG

[Andrii Nakryiko]

On Tue, Mar 5, 2024 at 12:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> Introduce a libbpf API bpf_program__attach_sk_msg() which allows
> user to get a bpf_link. The API makes auto-deletion easier and
> also allows user space application easier as link based APIs
> are used for all programs.
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c   | 8 ++++++++
>  tools/lib/bpf/libbpf.h   | 3 +++
>  tools/lib/bpf/libbpf.map | 1 +
>  3 files changed, 12 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 97b573516675..b3982bb3f979 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -149,6 +149,7 @@ static const char * const link_type_name[] = {
>         [BPF_LINK_TYPE_TCX]                     = "tcx",
>         [BPF_LINK_TYPE_UPROBE_MULTI]            = "uprobe_multi",
>         [BPF_LINK_TYPE_NETKIT]                  = "netkit",
> +       [BPF_LINK_TYPE_SK_MSG]                  = "sk_msg",
>  };
>
>  static const char * const map_type_name[] = {
> @@ -12280,6 +12281,13 @@ bpf_program__attach_netkit(const struct bpf_program *prog, int ifindex,
>         return bpf_program_attach_fd(prog, ifindex, "netkit", &link_create_opts);
>  }
>
> +struct bpf_link *
> +bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd,
> +                          enum bpf_attach_type attach_type)

Why do we need to allow users to override attach_type? Why can't it
come from bpf_program's expected attach type?

> +{
> +       return __bpf_program_attach_fd(prog, map_fd, attach_type, "sk_msg", NULL);
> +}
> +
>  struct bpf_link *bpf_program__attach_freplace(const struct bpf_program *prog,
>                                               int target_fd,
>                                               const char *attach_func_name)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 5723cbbfcc41..c8448f05e8d6 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -786,6 +786,9 @@ bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd);
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_xdp(const struct bpf_program *prog, int ifindex);
>  LIBBPF_API struct bpf_link *
> +bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd,
> +                          enum bpf_attach_type attach_type);
> +LIBBPF_API struct bpf_link *
>  bpf_program__attach_freplace(const struct bpf_program *prog,
>                              int target_fd, const char *attach_func_name);
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 86804fd90dd1..c59986c6dbc5 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -410,6 +410,7 @@ LIBBPF_1.3.0 {
>
>  LIBBPF_1.4.0 {
>         global:
> +               bpf_program__attach_sk_msg;
>                 bpf_token_create;
>                 btf__new_split;
>                 btf_ext__raw_data;
> --
> 2.43.0
>

Re: [RFC PATCH bpf-next 3/5] libbpf: Add link support for BPF_PROG_TYPE_SK_MSG

[Yonghong Song]

On 3/8/24 5:01 PM, Andrii Nakryiko wrote:
> On Tue, Mar 5, 2024 at 12:22 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Introduce a libbpf API bpf_program__attach_sk_msg() which allows
>> user to get a bpf_link. The API makes auto-deletion easier and
>> also allows user space application easier as link based APIs
>> are used for all programs.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/lib/bpf/libbpf.c   | 8 ++++++++
>>   tools/lib/bpf/libbpf.h   | 3 +++
>>   tools/lib/bpf/libbpf.map | 1 +
>>   3 files changed, 12 insertions(+)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 97b573516675..b3982bb3f979 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -149,6 +149,7 @@ static const char * const link_type_name[] = {
>>          [BPF_LINK_TYPE_TCX]                     = "tcx",
>>          [BPF_LINK_TYPE_UPROBE_MULTI]            = "uprobe_multi",
>>          [BPF_LINK_TYPE_NETKIT]                  = "netkit",
>> +       [BPF_LINK_TYPE_SK_MSG]                  = "sk_msg",
>>   };
>>
>>   static const char * const map_type_name[] = {
>> @@ -12280,6 +12281,13 @@ bpf_program__attach_netkit(const struct bpf_program *prog, int ifindex,
>>          return bpf_program_attach_fd(prog, ifindex, "netkit", &link_create_opts);
>>   }
>>
>> +struct bpf_link *
>> +bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd,
>> +                          enum bpf_attach_type attach_type)
> Why do we need to allow users to override attach_type? Why can't it
> come from bpf_program's expected attach type?

The interface is similar to previously used bpf_prog_attach(), but you are
right that this may not be needed. Let me double check.

>
>> +{
>> +       return __bpf_program_attach_fd(prog, map_fd, attach_type, "sk_msg", NULL);
>> +}
>> +
>>   struct bpf_link *bpf_program__attach_freplace(const struct bpf_program *prog,
>>                                                int target_fd,
>>                                                const char *attach_func_name)
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 5723cbbfcc41..c8448f05e8d6 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -786,6 +786,9 @@ bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd);
>>   LIBBPF_API struct bpf_link *
>>   bpf_program__attach_xdp(const struct bpf_program *prog, int ifindex);
>>   LIBBPF_API struct bpf_link *
>> +bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd,
>> +                          enum bpf_attach_type attach_type);
>> +LIBBPF_API struct bpf_link *
>>   bpf_program__attach_freplace(const struct bpf_program *prog,
>>                               int target_fd, const char *attach_func_name);
>>
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index 86804fd90dd1..c59986c6dbc5 100644
>> --- a/tools/lib/bpf/libbpf.map
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -410,6 +410,7 @@ LIBBPF_1.3.0 {
>>
>>   LIBBPF_1.4.0 {
>>          global:
>> +               bpf_program__attach_sk_msg;
>>                  bpf_token_create;
>>                  btf__new_split;
>>                  btf_ext__raw_data;
>> --
>> 2.43.0
>>

[RFC PATCH bpf-next 4/5] bpftool: Add link dump support for BPF_LINK_TYPE_SK_MSG

[Yonghong Song]

An example output looks like:
  9: sk_msg  prog 108
          map_id 84  attach_type sk_msg_verdict

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 tools/bpf/bpftool/link.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index afde9d0c2ea1..5eb140197d3f 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -526,6 +526,9 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
 		show_link_ifindex_json(info->netkit.ifindex, json_wtr);
 		show_link_attach_type_json(info->netkit.attach_type, json_wtr);
 		break;
+	case BPF_LINK_TYPE_SK_MSG:
+		jsonw_uint_field(json_wtr, "map_id", info->skmsg.map_id);
+		show_link_attach_type_json(info->skmsg.attach_type, json_wtr);
 	case BPF_LINK_TYPE_XDP:
 		show_link_ifindex_json(info->xdp.ifindex, json_wtr);
 		break;
@@ -915,6 +918,11 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
 		show_link_ifindex_plain(info->netkit.ifindex);
 		show_link_attach_type_plain(info->netkit.attach_type);
 		break;
+	case BPF_LINK_TYPE_SK_MSG:
+		printf("\n\t");
+		printf("map_id %u  ", info->skmsg.map_id);
+		show_link_attach_type_plain(info->skmsg.attach_type);
+		break;
 	case BPF_LINK_TYPE_XDP:
 		printf("\n\t");
 		show_link_ifindex_plain(info->xdp.ifindex);
-- 
2.43.0

Re: [RFC PATCH bpf-next 4/5] bpftool: Add link dump support for BPF_LINK_TYPE_SK_MSG

[Jakub Sitnicki]

On Tue, Mar 05, 2024 at 12:22 PM -08, Yonghong Song wrote:
> An example output looks like:
>   9: sk_msg  prog 108
>           map_id 84  attach_type sk_msg_verdict
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/bpf/bpftool/link.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
> index afde9d0c2ea1..5eb140197d3f 100644
> --- a/tools/bpf/bpftool/link.c
> +++ b/tools/bpf/bpftool/link.c
> @@ -526,6 +526,9 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
>  		show_link_ifindex_json(info->netkit.ifindex, json_wtr);
>  		show_link_attach_type_json(info->netkit.attach_type, json_wtr);
>  		break;
> +	case BPF_LINK_TYPE_SK_MSG:
> +		jsonw_uint_field(json_wtr, "map_id", info->skmsg.map_id);
> +		show_link_attach_type_json(info->skmsg.attach_type, json_wtr);

Compiler says - missing break.

>  	case BPF_LINK_TYPE_XDP:
>  		show_link_ifindex_json(info->xdp.ifindex, json_wtr);
>  		break;
> @@ -915,6 +918,11 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
>  		show_link_ifindex_plain(info->netkit.ifindex);
>  		show_link_attach_type_plain(info->netkit.attach_type);
>  		break;
> +	case BPF_LINK_TYPE_SK_MSG:
> +		printf("\n\t");
> +		printf("map_id %u  ", info->skmsg.map_id);
> +		show_link_attach_type_plain(info->skmsg.attach_type);
> +		break;
>  	case BPF_LINK_TYPE_XDP:
>  		printf("\n\t");
>  		show_link_ifindex_plain(info->xdp.ifindex);

Re: [RFC PATCH bpf-next 4/5] bpftool: Add link dump support for BPF_LINK_TYPE_SK_MSG

[Yonghong Song]

On 3/8/24 8:07 AM, Jakub Sitnicki wrote:
> On Tue, Mar 05, 2024 at 12:22 PM -08, Yonghong Song wrote:
>> An example output looks like:
>>    9: sk_msg  prog 108
>>            map_id 84  attach_type sk_msg_verdict
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/bpf/bpftool/link.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
>> index afde9d0c2ea1..5eb140197d3f 100644
>> --- a/tools/bpf/bpftool/link.c
>> +++ b/tools/bpf/bpftool/link.c
>> @@ -526,6 +526,9 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
>>   		show_link_ifindex_json(info->netkit.ifindex, json_wtr);
>>   		show_link_attach_type_json(info->netkit.attach_type, json_wtr);
>>   		break;
>> +	case BPF_LINK_TYPE_SK_MSG:
>> +		jsonw_uint_field(json_wtr, "map_id", info->skmsg.map_id);
>> +		show_link_attach_type_json(info->skmsg.attach_type, json_wtr);
> Compiler says - missing break.

Ack, will fix.

>
>>   	case BPF_LINK_TYPE_XDP:
>>   		show_link_ifindex_json(info->xdp.ifindex, json_wtr);
>>   		break;
>> @@ -915,6 +918,11 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
>>   		show_link_ifindex_plain(info->netkit.ifindex);
>>   		show_link_attach_type_plain(info->netkit.attach_type);
>>   		break;
>> +	case BPF_LINK_TYPE_SK_MSG:
>> +		printf("\n\t");
>> +		printf("map_id %u  ", info->skmsg.map_id);
>> +		show_link_attach_type_plain(info->skmsg.attach_type);
>> +		break;
>>   	case BPF_LINK_TYPE_XDP:
>>   		printf("\n\t");
>>   		show_link_ifindex_plain(info->xdp.ifindex);

[RFC PATCH bpf-next 5/5] selftests/bpf: Add some tests with new bpf_program__attach_sk_msg() API

[Yonghong Song]

In sockmap_basic.c and sockmap_listen.c, there are a few tests
dealing with BPF_PROG_TYPE_SK_MSG and those tests are using
prog attach/detach. This patch added new tests by copying the
original corresponding tests but using bpf_program__attach_sk_msg()
and bpf_link__detach() APIs.

All tests are passed.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 27 +++++++++++++
 .../selftests/bpf/prog_tests/sockmap_listen.c | 38 +++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 77e26ecffa9d..37eb1ce414a3 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -131,6 +131,29 @@ static void test_skmsg_helpers(enum bpf_map_type map_type)
 	test_skmsg_load_helpers__destroy(skel);
 }
 
+static void test_skmsg_helpers_with_link(enum bpf_map_type map_type)
+{
+	struct test_skmsg_load_helpers *skel;
+	struct bpf_program *prog;
+	struct bpf_link *link;
+	int map;
+
+	skel = test_skmsg_load_helpers__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_skmsg_load_helpers__open_and_load"))
+		return;
+
+	prog = skel->progs.prog_msg_verdict;
+	map = bpf_map__fd(skel->maps.sock_map);
+
+	link = bpf_program__attach_sk_msg(prog, map, BPF_SK_MSG_VERDICT);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sk_msg"))
+		goto out;
+
+	close(bpf_link__fd(link));
+out:
+	test_skmsg_load_helpers__destroy(skel);
+}
+
 static void test_sockmap_update(enum bpf_map_type map_type)
 {
 	int err, prog, src;
@@ -812,4 +835,8 @@ void test_sockmap_basic(void)
 		test_sockmap_many_maps();
 	if (test__start_subtest("sockmap same socket replace"))
 		test_sockmap_same_sock();
+	if (test__start_subtest("sockmap sk_msg attach helpers with link"))
+		test_skmsg_helpers_with_link(BPF_MAP_TYPE_SOCKMAP);
+	if (test__start_subtest("sockhash sk_msg attach helpers with link"))
+		test_skmsg_helpers_with_link(BPF_MAP_TYPE_SOCKHASH);
 }
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index a92807bfcd13..4bff0a857b04 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -767,6 +767,24 @@ static void test_msg_redir_to_connected(struct test_sockmap_listen *skel,
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_MSG_VERDICT);
 }
 
+static void test_msg_redir_to_connected_with_link(struct test_sockmap_listen *skel,
+						  struct bpf_map *inner_map, int family,
+						  int sotype)
+{
+	struct bpf_program *verdict = skel->progs.prog_msg_verdict;
+	int verdict_map = bpf_map__fd(skel->maps.verdict_map);
+	int sock_map = bpf_map__fd(inner_map);
+	struct bpf_link *link;
+
+	link = bpf_program__attach_sk_msg(verdict, sock_map, BPF_SK_MSG_VERDICT);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sk_msg"))
+		return;
+
+	redir_to_connected(family, sotype, sock_map, verdict_map, REDIR_EGRESS);
+
+	close(bpf_link__fd(link));
+}
+
 static void redir_to_listening(int family, int sotype, int sock_mapfd,
 			       int verd_mapfd, enum redir_mode mode)
 {
@@ -869,6 +887,24 @@ static void test_msg_redir_to_listening(struct test_sockmap_listen *skel,
 	xbpf_prog_detach2(verdict, sock_map, BPF_SK_MSG_VERDICT);
 }
 
+static void test_msg_redir_to_listening_with_link(struct test_sockmap_listen *skel,
+						  struct bpf_map *inner_map, int family,
+						  int sotype)
+{
+	struct bpf_program *verdict = skel->progs.prog_msg_verdict;
+	int verdict_map = bpf_map__fd(skel->maps.verdict_map);
+	int sock_map = bpf_map__fd(inner_map);
+	struct bpf_link *link;
+
+	link = bpf_program__attach_sk_msg(verdict, sock_map, BPF_SK_MSG_VERDICT);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sk_msg"))
+		return;
+
+	redir_to_listening(family, sotype, sock_map, verdict_map, REDIR_EGRESS);
+
+	close(bpf_link__fd(link));
+}
+
 static void redir_partial(int family, int sotype, int sock_map, int parser_map)
 {
 	int s, c0 = -1, c1 = -1, p0 = -1, p1 = -1;
@@ -1316,7 +1352,9 @@ static void test_redir(struct test_sockmap_listen *skel, struct bpf_map *map,
 		TEST(test_skb_redir_to_listening),
 		TEST(test_skb_redir_partial),
 		TEST(test_msg_redir_to_connected),
+		TEST(test_msg_redir_to_connected_with_link),
 		TEST(test_msg_redir_to_listening),
+		TEST(test_msg_redir_to_listening_with_link),
 	};
 	const char *family_name, *map_name;
 	const struct redir_test *t;
-- 
2.43.0

RE: [RFC PATCH bpf-next 0/5] Add bpf_link support for sk_msg prog

[John Fastabend]

Yonghong Song wrote:
> One of our internal services started to use sk_msg program and currently
> it used existing prog attach/detach2 as demonstrated in selftests.
> But attach/detach of all other bpf programs are based on bpf_link.
> Consistent attach/detach APIs for all programs will make things easy to
> undersand and less error prone. So this patch added bpf_link
> support for BPF_PROG_TYPE_SK_MSG.
> 
> I marked the patch as RFC as not all functionality are covered
> by tests yet, e.g. update_prog(). Maybe somebody can suggest
> an existing test which I can look into.
> Or maybe some other tests need to be added as well.

Agree this was missing it will be nice to get this merged. The links
are much nicer to deal with.

Re: [RFC PATCH bpf-next 0/5] Add bpf_link support for sk_msg prog

[Yonghong Song]

On 3/6/24 11:19 AM, John Fastabend wrote:
> Yonghong Song wrote:
>> One of our internal services started to use sk_msg program and currently
>> it used existing prog attach/detach2 as demonstrated in selftests.
>> But attach/detach of all other bpf programs are based on bpf_link.
>> Consistent attach/detach APIs for all programs will make things easy to
>> undersand and less error prone. So this patch added bpf_link
>> support for BPF_PROG_TYPE_SK_MSG.
>>
>> I marked the patch as RFC as not all functionality are covered
>> by tests yet, e.g. update_prog(). Maybe somebody can suggest
>> an existing test which I can look into.
>> Or maybe some other tests need to be added as well.
> Agree this was missing it will be nice to get this merged. The links
> are much nicer to deal with.

Agree that bpf_link is better as proved by other cases. It would be great
if you can take a look at the patch set and let me any issues, missing cases,
missing tests, etc. Thanks!

Re: [RFC PATCH bpf-next 0/5] Add bpf_link support for sk_msg prog

[Jakub Sitnicki]

On Tue, Mar 05, 2024 at 12:21 PM -08, Yonghong Song wrote:
> One of our internal services started to use sk_msg program and currently
> it used existing prog attach/detach2 as demonstrated in selftests.
> But attach/detach of all other bpf programs are based on bpf_link.
> Consistent attach/detach APIs for all programs will make things easy to
> undersand and less error prone. So this patch added bpf_link
> support for BPF_PROG_TYPE_SK_MSG.
>
> I marked the patch as RFC as not all functionality are covered
> by tests yet, e.g. update_prog(). Maybe somebody can suggest
> an existing test which I can look into.
> Or maybe some other tests need to be added as well.

I have a general remark, not specific to this work.

We can't attach with links from CLI, apart from when auto-attach is
supported.  `bpftool prog attach` doesn't use bpf links. For instance:

bpftool prog attach \
        pinned /sys/fs/bpf/test/sk_msg_prog \
        sk_msg_verdict \
        pinned /sys/fs/bpf/test/sock_map

Is there a plan for the CLI tooling to support it?

Re: [RFC PATCH bpf-next 0/5] Add bpf_link support for sk_msg prog

[Yonghong Song]

On 3/7/24 5:01 AM, Jakub Sitnicki wrote:
> On Tue, Mar 05, 2024 at 12:21 PM -08, Yonghong Song wrote:
>> One of our internal services started to use sk_msg program and currently
>> it used existing prog attach/detach2 as demonstrated in selftests.
>> But attach/detach of all other bpf programs are based on bpf_link.
>> Consistent attach/detach APIs for all programs will make things easy to
>> undersand and less error prone. So this patch added bpf_link
>> support for BPF_PROG_TYPE_SK_MSG.
>>
>> I marked the patch as RFC as not all functionality are covered
>> by tests yet, e.g. update_prog(). Maybe somebody can suggest
>> an existing test which I can look into.
>> Or maybe some other tests need to be added as well.
> I have a general remark, not specific to this work.
>
> We can't attach with links from CLI, apart from when auto-attach is
> supported.  `bpftool prog attach` doesn't use bpf links. For instance:
>
> bpftool prog attach \
>          pinned /sys/fs/bpf/test/sk_msg_prog \
>          sk_msg_verdict \
>          pinned /sys/fs/bpf/test/sock_map
>
> Is there a plan for the CLI tooling to support it?

As far as I know, there is no plan to support this. Of course, somebody
could try to implement this ...

Re: [RFC PATCH bpf-next 0/5] Add bpf_link support for sk_msg prog

[Jakub Sitnicki]

On Tue, Mar 05, 2024 at 12:21 PM -08, Yonghong Song wrote:

[...]

> I marked the patch as RFC as not all functionality are covered
> by tests yet, e.g. update_prog(). Maybe somebody can suggest
> an existing test which I can look into.
> Or maybe some other tests need to be added as well.

Looks like we don't have prog update covered today.
That would be something to add to sockmap_basic suite.