[PATCH bpf-next v2 0/6] bpf: Add bpf_link support for sk_msg and sk_skb progs

[PATCH bpf-next v2 0/6] bpf: Add bpf_link support for sk_msg and sk_skb progs

[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. Based on comments from
previous RFC patch, I added BPF_PROG_TYPE_SK_SKB support as well
as both program types have similar treatment w.r.t. bpf_link
handling.

For the patch series, patch 1 added kernel support. Patches 2/3
added libbpf support. Patch 4 added bpftool support and
patches 5/6 added some new tests.

Yonghong Song (6):
  bpf: Add bpf_link support for sk_msg and sk_skb progs
  libbpf: Add new sec_def "sk_skb/verdict"
  libbpf: Add bpf_link support for BPF_PROG_TYPE_SK_{MSG,SKB}
  bpftool: Add link dump support for BPF_LINK_TYPE_SK_{MSG,SKB}
  selftests/bpf: Refactor out helper functions for a few tests
  selftests/bpf: Add some tests with new
    bpf_program__attach_sk_{msg,skb}() APIs

 include/linux/bpf.h                           |  13 ++
 include/uapi/linux/bpf.h                      |  10 ++
 kernel/bpf/syscall.c                          |   4 +
 net/core/skmsg.c                              | 164 ++++++++++++++++++
 net/core/sock_map.c                           |   6 +-
 tools/bpf/bpftool/link.c                      |  18 ++
 tools/include/uapi/linux/bpf.h                |  10 ++
 tools/lib/bpf/libbpf.c                        |  15 ++
 tools/lib/bpf/libbpf.h                        |   4 +
 tools/lib/bpf/libbpf.map                      |   2 +
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 121 +++++++++++--
 .../selftests/bpf/prog_tests/sockmap_listen.c |  38 ++++
 .../bpf/progs/test_skmsg_load_helpers.c       |  15 +-
 .../bpf/progs/test_sockmap_pass_prog.c        |  11 +-
 .../progs/test_sockmap_skb_verdict_attach.c   |   2 +-
 15 files changed, 410 insertions(+), 23 deletions(-)

-- 
2.43.0

[PATCH bpf-next v2 1/6] bpf: Add bpf_link support for sk_msg and sk_skb progs

[Yonghong Song]

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            |  13 +++
 include/uapi/linux/bpf.h       |  10 ++
 kernel/bpf/syscall.c           |   4 +
 net/core/skmsg.c               | 164 +++++++++++++++++++++++++++++++++
 net/core/sock_map.c            |   6 +-
 tools/include/uapi/linux/bpf.h |  10 ++
 6 files changed, 203 insertions(+), 4 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 17843e66a1d3..8dabd47d3668 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2990,10 +2990,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_sk_msg_skb_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)
@@ -3088,6 +3092,15 @@ static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
 {
 	return -EINVAL;
 }
+static inline int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+			 struct bpf_prog *old, u32 which)
+{
+	return -EOPNOTSUPP;
+}
+static inline int bpf_sk_msg_skb_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 3c42b9f1bada..c5506cfca4f8 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1135,6 +1135,8 @@ 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,
+	BPF_LINK_TYPE_SK_SKB = 15,
 	__MAX_BPF_LINK_TYPE,
 };
 
@@ -6718,6 +6720,14 @@ struct bpf_link_info {
 			__u32 ifindex;
 			__u32 attach_type;
 		} netkit;
+		struct {
+			__u32 map_id;
+			__u32 attach_type;
+		} skmsg;
+		struct {
+			__u32 map_id;
+			__u32 attach_type;
+		} skskb;
 	};
 } __attribute__((aligned(8)));
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ae2ff73bde7e..3d13eec5a30d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -5213,6 +5213,10 @@ 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:
+	case BPF_PROG_TYPE_SK_SKB:
+		ret = bpf_sk_msg_skb_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..1aa900ad54d7 100644
--- a/net/core/skmsg.c
+++ b/net/core/skmsg.c
@@ -1256,3 +1256,167 @@ 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_sk_msg_skb_link {
+	struct bpf_link link;
+	struct bpf_map *map;
+	enum bpf_attach_type attach_type;
+};
+
+static DEFINE_MUTEX(link_mutex);
+
+static struct bpf_sk_msg_skb_link *bpf_sk_msg_skb_link(const struct bpf_link *link)
+{
+	return container_of(link, struct bpf_sk_msg_skb_link, link);
+}
+
+static void bpf_sk_msg_skb_link_release(struct bpf_link *link)
+{
+	struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
+
+	mutex_lock(&link_mutex);
+	if (sk_link->map) {
+		(void)sock_map_prog_update(sk_link->map, NULL, link->prog,
+					   sk_link->attach_type);
+		bpf_map_put_with_uref(sk_link->map);
+		sk_link->map = NULL;
+	}
+	mutex_unlock(&link_mutex);
+}
+
+static int bpf_sk_msg_skb_link_detach(struct bpf_link *link)
+{
+	bpf_sk_msg_skb_link_release(link);
+	return 0;
+}
+
+static void bpf_sk_msg_skb_link_dealloc(struct bpf_link *link)
+{
+	kfree(bpf_sk_msg_skb_link(link));
+}
+
+static int bpf_sk_msg_skb_link_update_prog(struct bpf_link *link,
+					   struct bpf_prog *new_prog,
+					   struct bpf_prog *old_prog)
+{
+	const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_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(sk_link->map, new_prog, old_prog,
+				   sk_link->attach_type);
+	if (!ret)
+		bpf_prog_inc(new_prog);
+
+out:
+	mutex_unlock(&link_mutex);
+	return ret;
+}
+
+static int bpf_sk_msg_skb_link_fill_info(const struct bpf_link *link,
+					 struct bpf_link_info *info)
+{
+	const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
+	u32 map_id = 0;
+
+	mutex_lock(&link_mutex);
+	if (sk_link->map)
+		map_id = sk_link->map->id;
+	mutex_unlock(&link_mutex);
+
+	if (link->type == BPF_LINK_TYPE_SK_MSG) {
+		info->skmsg.map_id = map_id;
+		info->skmsg.attach_type = sk_link->attach_type;
+	} else {
+		info->skskb.map_id = map_id;
+		info->skskb.attach_type = sk_link->attach_type;
+	}
+	return 0;
+}
+
+static void bpf_sk_msg_skb_link_show_fdinfo(const struct bpf_link *link,
+					    struct seq_file *seq)
+{
+	const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
+	u32 map_id = 0;
+
+	mutex_lock(&link_mutex);
+	if (sk_link->map)
+		map_id = sk_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", sk_link->attach_type);
+}
+
+static const struct bpf_link_ops bpf_sk_msg_skb_link_ops = {
+	.release = bpf_sk_msg_skb_link_release,
+	.dealloc = bpf_sk_msg_skb_link_dealloc,
+	.detach = bpf_sk_msg_skb_link_detach,
+	.update_prog = bpf_sk_msg_skb_link_update_prog,
+	.fill_link_info = bpf_sk_msg_skb_link_fill_info,
+	.show_fdinfo = bpf_sk_msg_skb_link_show_fdinfo,
+};
+
+int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+	struct bpf_link_primer link_primer;
+	struct bpf_sk_msg_skb_link *sk_link;
+	enum bpf_attach_type attach_type;
+	enum bpf_link_type link_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);
+
+	sk_link = kzalloc(sizeof(*sk_link), GFP_USER);
+	if (!sk_link) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	if (prog->type == BPF_PROG_TYPE_SK_MSG)
+		link_type = BPF_LINK_TYPE_SK_MSG;
+	else
+		link_type = BPF_LINK_TYPE_SK_SKB;
+
+	attach_type = attr->link_create.attach_type;
+	bpf_link_init(&sk_link->link, link_type, &bpf_sk_msg_skb_link_ops, prog);
+	sk_link->map = map;
+	sk_link->attach_type = attach_type;
+
+	ret = bpf_link_prime(&sk_link->link, &link_primer);
+	if (ret) {
+		kfree(sk_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 3c42b9f1bada..c5506cfca4f8 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1135,6 +1135,8 @@ 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,
+	BPF_LINK_TYPE_SK_SKB = 15,
 	__MAX_BPF_LINK_TYPE,
 };
 
@@ -6718,6 +6720,14 @@ struct bpf_link_info {
 			__u32 ifindex;
 			__u32 attach_type;
 		} netkit;
+		struct {
+			__u32 map_id;
+			__u32 attach_type;
+		} skmsg;
+		struct {
+			__u32 map_id;
+			__u32 attach_type;
+		} skskb;
 	};
 } __attribute__((aligned(8)));
 
-- 
2.43.0

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

[kernel test robot]

Hi Yonghong,

kernel test robot noticed the following build warnings:

[auto build test WARNING on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Yonghong-Song/bpf-Add-bpf_link-support-for-sk_msg-and-sk_skb-progs/20240320-015917
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20240319175406.2940628-1-yonghong.song%40linux.dev
patch subject: [PATCH bpf-next v2 1/6] bpf: Add bpf_link support for sk_msg and sk_skb progs
config: x86_64-randconfig-011-20240321 (https://download.01.org/0day-ci/archive/20240321/202403211543.wg8VXMiO-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240321/202403211543.wg8VXMiO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403211543.wg8VXMiO-lkp@intel.com/

All warnings (new ones prefixed by >>):

   net/core/skmsg.c:1279:9: error: call to undeclared function 'sock_map_prog_update'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1279 |                 (void)sock_map_prog_update(sk_link->map, NULL, link->prog,
         |                       ^
   net/core/skmsg.c:1281:3: error: call to undeclared function 'bpf_map_put_with_uref'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1281 |                 bpf_map_put_with_uref(sk_link->map);
         |                 ^
   net/core/skmsg.c:1316:8: error: call to undeclared function 'sock_map_prog_update'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1316 |         ret = sock_map_prog_update(sk_link->map, new_prog, old_prog,
         |               ^
   net/core/skmsg.c:1383:8: error: call to undeclared function 'bpf_map_get_with_uref'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1383 |         map = bpf_map_get_with_uref(attr->link_create.target_fd);
         |               ^
   net/core/skmsg.c:1383:6: error: incompatible integer to pointer conversion assigning to 'struct bpf_map *' from 'int' [-Wint-conversion]
    1383 |         map = bpf_map_get_with_uref(attr->link_create.target_fd);
         |             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   net/core/skmsg.c:1409:8: error: call to undeclared function 'sock_map_prog_update'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1409 |         ret = sock_map_prog_update(map, prog, NULL, attach_type);
         |               ^
   net/core/skmsg.c:1420:2: error: call to undeclared function 'bpf_map_put_with_uref'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1420 |         bpf_map_put_with_uref(map);
         |         ^
>> net/core/skmsg.c:1371:5: warning: no previous prototype for function 'bpf_sk_msg_skb_link_create' [-Wmissing-prototypes]
    1371 | int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
         |     ^
   net/core/skmsg.c:1371:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
    1371 | int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
         | ^
         | static 
   1 warning and 7 errors generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for DRM_I915_DEBUG_GEM
   Depends on [n]: HAS_IOMEM [=y] && DRM_I915 [=y] && EXPERT [=y] && DRM_I915_WERROR [=n]
   Selected by [y]:
   - DRM_I915_DEBUG [=y] && HAS_IOMEM [=y] && DRM_I915 [=y] && EXPERT [=y] && !COMPILE_TEST [=n]


vim +/bpf_sk_msg_skb_link_create +1371 net/core/skmsg.c

  1370	
> 1371	int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
  1372	{
  1373		struct bpf_link_primer link_primer;
  1374		struct bpf_sk_msg_skb_link *sk_link;
  1375		enum bpf_attach_type attach_type;
  1376		enum bpf_link_type link_type;
  1377		struct bpf_map *map;
  1378		int ret;
  1379	
  1380		if (attr->link_create.flags)
  1381			return -EINVAL;
  1382	
> 1383		map = bpf_map_get_with_uref(attr->link_create.target_fd);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Yonghong,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Yonghong-Song/bpf-Add-bpf_link-support-for-sk_msg-and-sk_skb-progs/20240320-015917
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20240319175406.2940628-1-yonghong.song%40linux.dev
patch subject: [PATCH bpf-next v2 1/6] bpf: Add bpf_link support for sk_msg and sk_skb progs
config: x86_64-randconfig-011-20240321 (https://download.01.org/0day-ci/archive/20240322/202403220430.affB6tuK-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240322/202403220430.affB6tuK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403220430.affB6tuK-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/core/skmsg.c:1279:9: error: call to undeclared function 'sock_map_prog_update'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1279 |                 (void)sock_map_prog_update(sk_link->map, NULL, link->prog,
         |                       ^
>> net/core/skmsg.c:1281:3: error: call to undeclared function 'bpf_map_put_with_uref'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1281 |                 bpf_map_put_with_uref(sk_link->map);
         |                 ^
   net/core/skmsg.c:1316:8: error: call to undeclared function 'sock_map_prog_update'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1316 |         ret = sock_map_prog_update(sk_link->map, new_prog, old_prog,
         |               ^
>> net/core/skmsg.c:1383:8: error: call to undeclared function 'bpf_map_get_with_uref'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1383 |         map = bpf_map_get_with_uref(attr->link_create.target_fd);
         |               ^
>> net/core/skmsg.c:1383:6: error: incompatible integer to pointer conversion assigning to 'struct bpf_map *' from 'int' [-Wint-conversion]
    1383 |         map = bpf_map_get_with_uref(attr->link_create.target_fd);
         |             ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   net/core/skmsg.c:1409:8: error: call to undeclared function 'sock_map_prog_update'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1409 |         ret = sock_map_prog_update(map, prog, NULL, attach_type);
         |               ^
   net/core/skmsg.c:1420:2: error: call to undeclared function 'bpf_map_put_with_uref'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1420 |         bpf_map_put_with_uref(map);
         |         ^
   net/core/skmsg.c:1371:5: warning: no previous prototype for function 'bpf_sk_msg_skb_link_create' [-Wmissing-prototypes]
    1371 | int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
         |     ^
   net/core/skmsg.c:1371:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
    1371 | int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
         | ^
         | static 
   1 warning and 7 errors generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for DRM_I915_DEBUG_GEM
   Depends on [n]: HAS_IOMEM [=y] && DRM_I915 [=y] && EXPERT [=y] && DRM_I915_WERROR [=n]
   Selected by [y]:
   - DRM_I915_DEBUG [=y] && HAS_IOMEM [=y] && DRM_I915 [=y] && EXPERT [=y] && !COMPILE_TEST [=n]


vim +/sock_map_prog_update +1279 net/core/skmsg.c

  1272	
  1273	static void bpf_sk_msg_skb_link_release(struct bpf_link *link)
  1274	{
  1275		struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
  1276	
  1277		mutex_lock(&link_mutex);
  1278		if (sk_link->map) {
> 1279			(void)sock_map_prog_update(sk_link->map, NULL, link->prog,
  1280						   sk_link->attach_type);
> 1281			bpf_map_put_with_uref(sk_link->map);
  1282			sk_link->map = NULL;
  1283		}
  1284		mutex_unlock(&link_mutex);
  1285	}
  1286	
  1287	static int bpf_sk_msg_skb_link_detach(struct bpf_link *link)
  1288	{
  1289		bpf_sk_msg_skb_link_release(link);
  1290		return 0;
  1291	}
  1292	
  1293	static void bpf_sk_msg_skb_link_dealloc(struct bpf_link *link)
  1294	{
  1295		kfree(bpf_sk_msg_skb_link(link));
  1296	}
  1297	
  1298	static int bpf_sk_msg_skb_link_update_prog(struct bpf_link *link,
  1299						   struct bpf_prog *new_prog,
  1300						   struct bpf_prog *old_prog)
  1301	{
  1302		const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
  1303		int ret = 0;
  1304	
  1305		mutex_lock(&link_mutex);
  1306		if (old_prog && link->prog != old_prog) {
  1307			ret = -EPERM;
  1308			goto out;
  1309		}
  1310	
  1311		if (link->prog->type != new_prog->type) {
  1312			ret = -EINVAL;
  1313			goto out;
  1314		}
  1315	
  1316		ret = sock_map_prog_update(sk_link->map, new_prog, old_prog,
  1317					   sk_link->attach_type);
  1318		if (!ret)
  1319			bpf_prog_inc(new_prog);
  1320	
  1321	out:
  1322		mutex_unlock(&link_mutex);
  1323		return ret;
  1324	}
  1325	
  1326	static int bpf_sk_msg_skb_link_fill_info(const struct bpf_link *link,
  1327						 struct bpf_link_info *info)
  1328	{
  1329		const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
  1330		u32 map_id = 0;
  1331	
  1332		mutex_lock(&link_mutex);
  1333		if (sk_link->map)
  1334			map_id = sk_link->map->id;
  1335		mutex_unlock(&link_mutex);
  1336	
  1337		if (link->type == BPF_LINK_TYPE_SK_MSG) {
  1338			info->skmsg.map_id = map_id;
  1339			info->skmsg.attach_type = sk_link->attach_type;
  1340		} else {
  1341			info->skskb.map_id = map_id;
  1342			info->skskb.attach_type = sk_link->attach_type;
  1343		}
  1344		return 0;
  1345	}
  1346	
  1347	static void bpf_sk_msg_skb_link_show_fdinfo(const struct bpf_link *link,
  1348						    struct seq_file *seq)
  1349	{
  1350		const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
  1351		u32 map_id = 0;
  1352	
  1353		mutex_lock(&link_mutex);
  1354		if (sk_link->map)
  1355			map_id = sk_link->map->id;
  1356		mutex_unlock(&link_mutex);
  1357	
  1358		seq_printf(seq, "map_id:\t%u\n", map_id);
  1359		seq_printf(seq, "attach_type:\t%u (...)\n", sk_link->attach_type);
  1360	}
  1361	
  1362	static const struct bpf_link_ops bpf_sk_msg_skb_link_ops = {
  1363		.release = bpf_sk_msg_skb_link_release,
  1364		.dealloc = bpf_sk_msg_skb_link_dealloc,
  1365		.detach = bpf_sk_msg_skb_link_detach,
  1366		.update_prog = bpf_sk_msg_skb_link_update_prog,
  1367		.fill_link_info = bpf_sk_msg_skb_link_fill_info,
  1368		.show_fdinfo = bpf_sk_msg_skb_link_show_fdinfo,
  1369	};
  1370	
  1371	int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
  1372	{
  1373		struct bpf_link_primer link_primer;
  1374		struct bpf_sk_msg_skb_link *sk_link;
  1375		enum bpf_attach_type attach_type;
  1376		enum bpf_link_type link_type;
  1377		struct bpf_map *map;
  1378		int ret;
  1379	
  1380		if (attr->link_create.flags)
  1381			return -EINVAL;
  1382	
> 1383		map = bpf_map_get_with_uref(attr->link_create.target_fd);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Yonghong,

kernel test robot noticed the following build errors:

[auto build test ERROR on bpf-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Yonghong-Song/bpf-Add-bpf_link-support-for-sk_msg-and-sk_skb-progs/20240320-015917
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link:    https://lore.kernel.org/r/20240319175406.2940628-1-yonghong.song%40linux.dev
patch subject: [PATCH bpf-next v2 1/6] bpf: Add bpf_link support for sk_msg and sk_skb progs
config: nios2-randconfig-001-20240321 (https://download.01.org/0day-ci/archive/20240322/202403220545.KEzjr9hI-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240322/202403220545.KEzjr9hI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403220545.KEzjr9hI-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   net/core/skmsg.c: In function 'bpf_sk_msg_skb_link_release':
>> net/core/skmsg.c:1279:23: error: implicit declaration of function 'sock_map_prog_update' [-Werror=implicit-function-declaration]
    1279 |                 (void)sock_map_prog_update(sk_link->map, NULL, link->prog,
         |                       ^~~~~~~~~~~~~~~~~~~~
>> net/core/skmsg.c:1281:17: error: implicit declaration of function 'bpf_map_put_with_uref' [-Werror=implicit-function-declaration]
    1281 |                 bpf_map_put_with_uref(sk_link->map);
         |                 ^~~~~~~~~~~~~~~~~~~~~
   net/core/skmsg.c: At top level:
>> net/core/skmsg.c:1371:5: warning: no previous prototype for 'bpf_sk_msg_skb_link_create' [-Wmissing-prototypes]
    1371 | int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
   net/core/skmsg.c: In function 'bpf_sk_msg_skb_link_create':
>> net/core/skmsg.c:1383:15: error: implicit declaration of function 'bpf_map_get_with_uref' [-Werror=implicit-function-declaration]
    1383 |         map = bpf_map_get_with_uref(attr->link_create.target_fd);
         |               ^~~~~~~~~~~~~~~~~~~~~
>> net/core/skmsg.c:1383:13: warning: assignment to 'struct bpf_map *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    1383 |         map = bpf_map_get_with_uref(attr->link_create.target_fd);
         |             ^
   cc1: some warnings being treated as errors


vim +/sock_map_prog_update +1279 net/core/skmsg.c

  1272	
  1273	static void bpf_sk_msg_skb_link_release(struct bpf_link *link)
  1274	{
  1275		struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
  1276	
  1277		mutex_lock(&link_mutex);
  1278		if (sk_link->map) {
> 1279			(void)sock_map_prog_update(sk_link->map, NULL, link->prog,
  1280						   sk_link->attach_type);
> 1281			bpf_map_put_with_uref(sk_link->map);
  1282			sk_link->map = NULL;
  1283		}
  1284		mutex_unlock(&link_mutex);
  1285	}
  1286	
  1287	static int bpf_sk_msg_skb_link_detach(struct bpf_link *link)
  1288	{
  1289		bpf_sk_msg_skb_link_release(link);
  1290		return 0;
  1291	}
  1292	
  1293	static void bpf_sk_msg_skb_link_dealloc(struct bpf_link *link)
  1294	{
  1295		kfree(bpf_sk_msg_skb_link(link));
  1296	}
  1297	
  1298	static int bpf_sk_msg_skb_link_update_prog(struct bpf_link *link,
  1299						   struct bpf_prog *new_prog,
  1300						   struct bpf_prog *old_prog)
  1301	{
  1302		const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
  1303		int ret = 0;
  1304	
  1305		mutex_lock(&link_mutex);
  1306		if (old_prog && link->prog != old_prog) {
  1307			ret = -EPERM;
  1308			goto out;
  1309		}
  1310	
  1311		if (link->prog->type != new_prog->type) {
  1312			ret = -EINVAL;
  1313			goto out;
  1314		}
  1315	
  1316		ret = sock_map_prog_update(sk_link->map, new_prog, old_prog,
  1317					   sk_link->attach_type);
  1318		if (!ret)
  1319			bpf_prog_inc(new_prog);
  1320	
  1321	out:
  1322		mutex_unlock(&link_mutex);
  1323		return ret;
  1324	}
  1325	
  1326	static int bpf_sk_msg_skb_link_fill_info(const struct bpf_link *link,
  1327						 struct bpf_link_info *info)
  1328	{
  1329		const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
  1330		u32 map_id = 0;
  1331	
  1332		mutex_lock(&link_mutex);
  1333		if (sk_link->map)
  1334			map_id = sk_link->map->id;
  1335		mutex_unlock(&link_mutex);
  1336	
  1337		if (link->type == BPF_LINK_TYPE_SK_MSG) {
  1338			info->skmsg.map_id = map_id;
  1339			info->skmsg.attach_type = sk_link->attach_type;
  1340		} else {
  1341			info->skskb.map_id = map_id;
  1342			info->skskb.attach_type = sk_link->attach_type;
  1343		}
  1344		return 0;
  1345	}
  1346	
  1347	static void bpf_sk_msg_skb_link_show_fdinfo(const struct bpf_link *link,
  1348						    struct seq_file *seq)
  1349	{
  1350		const struct bpf_sk_msg_skb_link *sk_link = bpf_sk_msg_skb_link(link);
  1351		u32 map_id = 0;
  1352	
  1353		mutex_lock(&link_mutex);
  1354		if (sk_link->map)
  1355			map_id = sk_link->map->id;
  1356		mutex_unlock(&link_mutex);
  1357	
  1358		seq_printf(seq, "map_id:\t%u\n", map_id);
  1359		seq_printf(seq, "attach_type:\t%u (...)\n", sk_link->attach_type);
  1360	}
  1361	
  1362	static const struct bpf_link_ops bpf_sk_msg_skb_link_ops = {
  1363		.release = bpf_sk_msg_skb_link_release,
  1364		.dealloc = bpf_sk_msg_skb_link_dealloc,
  1365		.detach = bpf_sk_msg_skb_link_detach,
  1366		.update_prog = bpf_sk_msg_skb_link_update_prog,
  1367		.fill_link_info = bpf_sk_msg_skb_link_fill_info,
  1368		.show_fdinfo = bpf_sk_msg_skb_link_show_fdinfo,
  1369	};
  1370	
> 1371	int bpf_sk_msg_skb_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
  1372	{
  1373		struct bpf_link_primer link_primer;
  1374		struct bpf_sk_msg_skb_link *sk_link;
  1375		enum bpf_attach_type attach_type;
  1376		enum bpf_link_type link_type;
  1377		struct bpf_map *map;
  1378		int ret;
  1379	
  1380		if (attr->link_create.flags)
  1381			return -EINVAL;
  1382	
> 1383		map = bpf_map_get_with_uref(attr->link_create.target_fd);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Andrii Nakryiko]

On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> 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            |  13 +++
>  include/uapi/linux/bpf.h       |  10 ++
>  kernel/bpf/syscall.c           |   4 +
>  net/core/skmsg.c               | 164 +++++++++++++++++++++++++++++++++
>  net/core/sock_map.c            |   6 +-
>  tools/include/uapi/linux/bpf.h |  10 ++
>  6 files changed, 203 insertions(+), 4 deletions(-)
>

[...]

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 3c42b9f1bada..c5506cfca4f8 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1135,6 +1135,8 @@ 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,
> +       BPF_LINK_TYPE_SK_SKB = 15,

they are both "sockmap attachments", so maybe we should just have
something like BPF_LINK_TYPE_SOCKMAP ?

>         __MAX_BPF_LINK_TYPE,
>  };
>
> @@ -6718,6 +6720,14 @@ struct bpf_link_info {
>                         __u32 ifindex;
>                         __u32 attach_type;
>                 } netkit;
> +               struct {
> +                       __u32 map_id;
> +                       __u32 attach_type;
> +               } skmsg;
> +               struct {
> +                       __u32 map_id;
> +                       __u32 attach_type;
> +               } skskb;

and then this would be also just one struct, instead of two identical
ones duplicated

>         };
>  } __attribute__((aligned(8)));
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index ae2ff73bde7e..3d13eec5a30d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5213,6 +5213,10 @@ 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:
> +       case BPF_PROG_TYPE_SK_SKB:
> +               ret = bpf_sk_msg_skb_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..1aa900ad54d7 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1256,3 +1256,167 @@ 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_sk_msg_skb_link {
> +       struct bpf_link link;
> +       struct bpf_map *map;
> +       enum bpf_attach_type attach_type;
> +};
> +
> +static DEFINE_MUTEX(link_mutex);

maybe more specific name, sockmap_link_mutex? link_mutex sounds very generic

> +
> +static struct bpf_sk_msg_skb_link *bpf_sk_msg_skb_link(const struct bpf_link *link)
> +{
> +       return container_of(link, struct bpf_sk_msg_skb_link, link);
> +}
> +

[...]

> +       attach_type = attr->link_create.attach_type;
> +       bpf_link_init(&sk_link->link, link_type, &bpf_sk_msg_skb_link_ops, prog);
> +       sk_link->map = map;
> +       sk_link->attach_type = attach_type;
> +
> +       ret = bpf_link_prime(&sk_link->link, &link_primer);
> +       if (ret) {
> +               kfree(sk_link);
> +               goto out;
> +       }
> +
> +       ret = sock_map_prog_update(map, prog, NULL, attach_type);

Does anything prevent someone else do to remove this program from
user-space, bypassing the link? It's a guarantee of a link that
attachment won't be tampered with (except for SYS_ADMIN-only
force-detachment, which is a completely separate thing).

It feels like there should be some sort of protection for programs
attached through sockmap link here. Just like we have this for XDP,
for example, or any of cgroup BPF programs attached through BPF link.

> +       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;
> +}

[...]

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

[Yonghong Song]

On 3/22/24 11:45 AM, Andrii Nakryiko wrote:
> On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> 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            |  13 +++
>>   include/uapi/linux/bpf.h       |  10 ++
>>   kernel/bpf/syscall.c           |   4 +
>>   net/core/skmsg.c               | 164 +++++++++++++++++++++++++++++++++
>>   net/core/sock_map.c            |   6 +-
>>   tools/include/uapi/linux/bpf.h |  10 ++
>>   6 files changed, 203 insertions(+), 4 deletions(-)
>>
> [...]
>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 3c42b9f1bada..c5506cfca4f8 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1135,6 +1135,8 @@ 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,
>> +       BPF_LINK_TYPE_SK_SKB = 15,
> they are both "sockmap attachments", so maybe we should just have
> something like BPF_LINK_TYPE_SOCKMAP ?

Yes, we could do this. Basically it represents all programs
which can be attached to sockmap.

>
>>          __MAX_BPF_LINK_TYPE,
>>   };
>>
>> @@ -6718,6 +6720,14 @@ struct bpf_link_info {
>>                          __u32 ifindex;
>>                          __u32 attach_type;
>>                  } netkit;
>> +               struct {
>> +                       __u32 map_id;
>> +                       __u32 attach_type;
>> +               } skmsg;
>> +               struct {
>> +                       __u32 map_id;
>> +                       __u32 attach_type;
>> +               } skskb;
> and then this would be also just one struct, instead of two identical
> ones duplicated

Right, we could do one with name 'sockmap'.

>
>>          };
>>   } __attribute__((aligned(8)));
>>
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index ae2ff73bde7e..3d13eec5a30d 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -5213,6 +5213,10 @@ 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:
>> +       case BPF_PROG_TYPE_SK_SKB:
>> +               ret = bpf_sk_msg_skb_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..1aa900ad54d7 100644
>> --- a/net/core/skmsg.c
>> +++ b/net/core/skmsg.c
>> @@ -1256,3 +1256,167 @@ 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_sk_msg_skb_link {
>> +       struct bpf_link link;
>> +       struct bpf_map *map;
>> +       enum bpf_attach_type attach_type;
>> +};
>> +
>> +static DEFINE_MUTEX(link_mutex);
> maybe more specific name, sockmap_link_mutex? link_mutex sounds very generic

Good idea.

>
>> +
>> +static struct bpf_sk_msg_skb_link *bpf_sk_msg_skb_link(const struct bpf_link *link)
>> +{
>> +       return container_of(link, struct bpf_sk_msg_skb_link, link);
>> +}
>> +
> [...]
>
>> +       attach_type = attr->link_create.attach_type;
>> +       bpf_link_init(&sk_link->link, link_type, &bpf_sk_msg_skb_link_ops, prog);
>> +       sk_link->map = map;
>> +       sk_link->attach_type = attach_type;
>> +
>> +       ret = bpf_link_prime(&sk_link->link, &link_primer);
>> +       if (ret) {
>> +               kfree(sk_link);
>> +               goto out;
>> +       }
>> +
>> +       ret = sock_map_prog_update(map, prog, NULL, attach_type);
> Does anything prevent someone else do to remove this program from
> user-space, bypassing the link? It's a guarantee of a link that
> attachment won't be tampered with (except for SYS_ADMIN-only
> force-detachment, which is a completely separate thing).
>
> It feels like there should be some sort of protection for programs
> attached through sockmap link here. Just like we have this for XDP,
> for example, or any of cgroup BPF programs attached through BPF link.

Good point. I have a 'bpf_prog_inc(prog)' below, I could do a refcount increase
before sock_map_prog_update(), we then should be okay.

>
>> +       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;
>> +}
> [...]

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

[Andrii Nakryiko]

On Fri, Mar 22, 2024 at 12:18 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 3/22/24 11:45 AM, Andrii Nakryiko wrote:
> > On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> 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            |  13 +++
> >>   include/uapi/linux/bpf.h       |  10 ++
> >>   kernel/bpf/syscall.c           |   4 +
> >>   net/core/skmsg.c               | 164 +++++++++++++++++++++++++++++++++
> >>   net/core/sock_map.c            |   6 +-
> >>   tools/include/uapi/linux/bpf.h |  10 ++
> >>   6 files changed, 203 insertions(+), 4 deletions(-)
> >>
> > [...]
> >
> >> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >> index 3c42b9f1bada..c5506cfca4f8 100644
> >> --- a/include/uapi/linux/bpf.h
> >> +++ b/include/uapi/linux/bpf.h
> >> @@ -1135,6 +1135,8 @@ 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,
> >> +       BPF_LINK_TYPE_SK_SKB = 15,
> > they are both "sockmap attachments", so maybe we should just have
> > something like BPF_LINK_TYPE_SOCKMAP ?
>
> Yes, we could do this. Basically it represents all programs
> which can be attached to sockmap.
>
> >
> >>          __MAX_BPF_LINK_TYPE,
> >>   };
> >>
> >> @@ -6718,6 +6720,14 @@ struct bpf_link_info {
> >>                          __u32 ifindex;
> >>                          __u32 attach_type;
> >>                  } netkit;
> >> +               struct {
> >> +                       __u32 map_id;
> >> +                       __u32 attach_type;
> >> +               } skmsg;
> >> +               struct {
> >> +                       __u32 map_id;
> >> +                       __u32 attach_type;
> >> +               } skskb;
> > and then this would be also just one struct, instead of two identical
> > ones duplicated
>
> Right, we could do one with name 'sockmap'.
>
> >
> >>          };
> >>   } __attribute__((aligned(8)));
> >>
> >> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >> index ae2ff73bde7e..3d13eec5a30d 100644
> >> --- a/kernel/bpf/syscall.c
> >> +++ b/kernel/bpf/syscall.c
> >> @@ -5213,6 +5213,10 @@ 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:
> >> +       case BPF_PROG_TYPE_SK_SKB:
> >> +               ret = bpf_sk_msg_skb_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..1aa900ad54d7 100644
> >> --- a/net/core/skmsg.c
> >> +++ b/net/core/skmsg.c
> >> @@ -1256,3 +1256,167 @@ 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_sk_msg_skb_link {
> >> +       struct bpf_link link;
> >> +       struct bpf_map *map;
> >> +       enum bpf_attach_type attach_type;
> >> +};
> >> +
> >> +static DEFINE_MUTEX(link_mutex);
> > maybe more specific name, sockmap_link_mutex? link_mutex sounds very generic
>
> Good idea.
>
> >
> >> +
> >> +static struct bpf_sk_msg_skb_link *bpf_sk_msg_skb_link(const struct bpf_link *link)
> >> +{
> >> +       return container_of(link, struct bpf_sk_msg_skb_link, link);
> >> +}
> >> +
> > [...]
> >
> >> +       attach_type = attr->link_create.attach_type;
> >> +       bpf_link_init(&sk_link->link, link_type, &bpf_sk_msg_skb_link_ops, prog);
> >> +       sk_link->map = map;
> >> +       sk_link->attach_type = attach_type;
> >> +
> >> +       ret = bpf_link_prime(&sk_link->link, &link_primer);
> >> +       if (ret) {
> >> +               kfree(sk_link);
> >> +               goto out;
> >> +       }
> >> +
> >> +       ret = sock_map_prog_update(map, prog, NULL, attach_type);
> > Does anything prevent someone else do to remove this program from
> > user-space, bypassing the link? It's a guarantee of a link that
> > attachment won't be tampered with (except for SYS_ADMIN-only
> > force-detachment, which is a completely separate thing).
> >
> > It feels like there should be some sort of protection for programs
> > attached through sockmap link here. Just like we have this for XDP,
> > for example, or any of cgroup BPF programs attached through BPF link.
>
> Good point. I have a 'bpf_prog_inc(prog)' below, I could do a refcount increase
> before sock_map_prog_update(), we then should be okay.
>

My point was that once you attach a program to sockmap with
LINK_CREATE, someone else just doing bpf_prog_attac() shouldn't
replace this program anymore. BPF link preserves *attachment
lifetime*, not just the program lifetime.

> >
> >> +       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;
> >> +}
> > [...]

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

[Yonghong Song]

On 3/22/24 1:16 PM, Andrii Nakryiko wrote:
> On Fri, Mar 22, 2024 at 12:18 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> On 3/22/24 11:45 AM, Andrii Nakryiko wrote:
>>> On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> 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            |  13 +++
>>>>    include/uapi/linux/bpf.h       |  10 ++
>>>>    kernel/bpf/syscall.c           |   4 +
>>>>    net/core/skmsg.c               | 164 +++++++++++++++++++++++++++++++++
>>>>    net/core/sock_map.c            |   6 +-
>>>>    tools/include/uapi/linux/bpf.h |  10 ++
>>>>    6 files changed, 203 insertions(+), 4 deletions(-)
>>>>
>>> [...]
>>>
>>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>>>> index 3c42b9f1bada..c5506cfca4f8 100644
>>>> --- a/include/uapi/linux/bpf.h
>>>> +++ b/include/uapi/linux/bpf.h
>>>> @@ -1135,6 +1135,8 @@ 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,
>>>> +       BPF_LINK_TYPE_SK_SKB = 15,
>>> they are both "sockmap attachments", so maybe we should just have
>>> something like BPF_LINK_TYPE_SOCKMAP ?
>> Yes, we could do this. Basically it represents all programs
>> which can be attached to sockmap.
>>
>>>>           __MAX_BPF_LINK_TYPE,
>>>>    };
>>>>
>>>> @@ -6718,6 +6720,14 @@ struct bpf_link_info {
>>>>                           __u32 ifindex;
>>>>                           __u32 attach_type;
>>>>                   } netkit;
>>>> +               struct {
>>>> +                       __u32 map_id;
>>>> +                       __u32 attach_type;
>>>> +               } skmsg;
>>>> +               struct {
>>>> +                       __u32 map_id;
>>>> +                       __u32 attach_type;
>>>> +               } skskb;
>>> and then this would be also just one struct, instead of two identical
>>> ones duplicated
>> Right, we could do one with name 'sockmap'.
>>
>>>>           };
>>>>    } __attribute__((aligned(8)));
>>>>
>>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>>> index ae2ff73bde7e..3d13eec5a30d 100644
>>>> --- a/kernel/bpf/syscall.c
>>>> +++ b/kernel/bpf/syscall.c
>>>> @@ -5213,6 +5213,10 @@ 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:
>>>> +       case BPF_PROG_TYPE_SK_SKB:
>>>> +               ret = bpf_sk_msg_skb_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..1aa900ad54d7 100644
>>>> --- a/net/core/skmsg.c
>>>> +++ b/net/core/skmsg.c
>>>> @@ -1256,3 +1256,167 @@ 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_sk_msg_skb_link {
>>>> +       struct bpf_link link;
>>>> +       struct bpf_map *map;
>>>> +       enum bpf_attach_type attach_type;
>>>> +};
>>>> +
>>>> +static DEFINE_MUTEX(link_mutex);
>>> maybe more specific name, sockmap_link_mutex? link_mutex sounds very generic
>> Good idea.
>>
>>>> +
>>>> +static struct bpf_sk_msg_skb_link *bpf_sk_msg_skb_link(const struct bpf_link *link)
>>>> +{
>>>> +       return container_of(link, struct bpf_sk_msg_skb_link, link);
>>>> +}
>>>> +
>>> [...]
>>>
>>>> +       attach_type = attr->link_create.attach_type;
>>>> +       bpf_link_init(&sk_link->link, link_type, &bpf_sk_msg_skb_link_ops, prog);
>>>> +       sk_link->map = map;
>>>> +       sk_link->attach_type = attach_type;
>>>> +
>>>> +       ret = bpf_link_prime(&sk_link->link, &link_primer);
>>>> +       if (ret) {
>>>> +               kfree(sk_link);
>>>> +               goto out;
>>>> +       }
>>>> +
>>>> +       ret = sock_map_prog_update(map, prog, NULL, attach_type);
>>> Does anything prevent someone else do to remove this program from
>>> user-space, bypassing the link? It's a guarantee of a link that
>>> attachment won't be tampered with (except for SYS_ADMIN-only
>>> force-detachment, which is a completely separate thing).
>>>
>>> It feels like there should be some sort of protection for programs
>>> attached through sockmap link here. Just like we have this for XDP,
>>> for example, or any of cgroup BPF programs attached through BPF link.
>> Good point. I have a 'bpf_prog_inc(prog)' below, I could do a refcount increase
>> before sock_map_prog_update(), we then should be okay.
>>
> My point was that once you attach a program to sockmap with
> LINK_CREATE, someone else just doing bpf_prog_attac() shouldn't
> replace this program anymore. BPF link preserves *attachment
> lifetime*, not just the program lifetime.

I see. I briefly looked at xdp and cgroup where both support
link-based attachment and program base attachment.
For xdp, indeed if link attachment is already done,
prog attach is not allowed.
cgroup seems more complicated and need further study to
confirm whether BPF link preserves attachment in all
cases or not.
But any way, since bpf_link is a new feature for
sockmap. It makes sense to follow the xdp-link
style to disallow bpf_prog_attach once link is
created.

>
>>>> +       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;
>>>> +}
>>> [...]

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

[Andrii Nakryiko]

On Fri, Mar 22, 2024 at 2:34 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
> On 3/22/24 1:16 PM, Andrii Nakryiko wrote:
> > On Fri, Mar 22, 2024 at 12:18 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >>
> >> On 3/22/24 11:45 AM, Andrii Nakryiko wrote:
> >>> On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> 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            |  13 +++
> >>>>    include/uapi/linux/bpf.h       |  10 ++
> >>>>    kernel/bpf/syscall.c           |   4 +
> >>>>    net/core/skmsg.c               | 164 +++++++++++++++++++++++++++++++++
> >>>>    net/core/sock_map.c            |   6 +-
> >>>>    tools/include/uapi/linux/bpf.h |  10 ++
> >>>>    6 files changed, 203 insertions(+), 4 deletions(-)
> >>>>
> >>> [...]
> >>>
> >>>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>>> index 3c42b9f1bada..c5506cfca4f8 100644
> >>>> --- a/include/uapi/linux/bpf.h
> >>>> +++ b/include/uapi/linux/bpf.h
> >>>> @@ -1135,6 +1135,8 @@ 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,
> >>>> +       BPF_LINK_TYPE_SK_SKB = 15,
> >>> they are both "sockmap attachments", so maybe we should just have
> >>> something like BPF_LINK_TYPE_SOCKMAP ?
> >> Yes, we could do this. Basically it represents all programs
> >> which can be attached to sockmap.
> >>
> >>>>           __MAX_BPF_LINK_TYPE,
> >>>>    };
> >>>>
> >>>> @@ -6718,6 +6720,14 @@ struct bpf_link_info {
> >>>>                           __u32 ifindex;
> >>>>                           __u32 attach_type;
> >>>>                   } netkit;
> >>>> +               struct {
> >>>> +                       __u32 map_id;
> >>>> +                       __u32 attach_type;
> >>>> +               } skmsg;
> >>>> +               struct {
> >>>> +                       __u32 map_id;
> >>>> +                       __u32 attach_type;
> >>>> +               } skskb;
> >>> and then this would be also just one struct, instead of two identical
> >>> ones duplicated
> >> Right, we could do one with name 'sockmap'.
> >>
> >>>>           };
> >>>>    } __attribute__((aligned(8)));
> >>>>
> >>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >>>> index ae2ff73bde7e..3d13eec5a30d 100644
> >>>> --- a/kernel/bpf/syscall.c
> >>>> +++ b/kernel/bpf/syscall.c
> >>>> @@ -5213,6 +5213,10 @@ 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:
> >>>> +       case BPF_PROG_TYPE_SK_SKB:
> >>>> +               ret = bpf_sk_msg_skb_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..1aa900ad54d7 100644
> >>>> --- a/net/core/skmsg.c
> >>>> +++ b/net/core/skmsg.c
> >>>> @@ -1256,3 +1256,167 @@ 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_sk_msg_skb_link {
> >>>> +       struct bpf_link link;
> >>>> +       struct bpf_map *map;
> >>>> +       enum bpf_attach_type attach_type;
> >>>> +};
> >>>> +
> >>>> +static DEFINE_MUTEX(link_mutex);
> >>> maybe more specific name, sockmap_link_mutex? link_mutex sounds very generic
> >> Good idea.
> >>
> >>>> +
> >>>> +static struct bpf_sk_msg_skb_link *bpf_sk_msg_skb_link(const struct bpf_link *link)
> >>>> +{
> >>>> +       return container_of(link, struct bpf_sk_msg_skb_link, link);
> >>>> +}
> >>>> +
> >>> [...]
> >>>
> >>>> +       attach_type = attr->link_create.attach_type;
> >>>> +       bpf_link_init(&sk_link->link, link_type, &bpf_sk_msg_skb_link_ops, prog);
> >>>> +       sk_link->map = map;
> >>>> +       sk_link->attach_type = attach_type;
> >>>> +
> >>>> +       ret = bpf_link_prime(&sk_link->link, &link_primer);
> >>>> +       if (ret) {
> >>>> +               kfree(sk_link);
> >>>> +               goto out;
> >>>> +       }
> >>>> +
> >>>> +       ret = sock_map_prog_update(map, prog, NULL, attach_type);
> >>> Does anything prevent someone else do to remove this program from
> >>> user-space, bypassing the link? It's a guarantee of a link that
> >>> attachment won't be tampered with (except for SYS_ADMIN-only
> >>> force-detachment, which is a completely separate thing).
> >>>
> >>> It feels like there should be some sort of protection for programs
> >>> attached through sockmap link here. Just like we have this for XDP,
> >>> for example, or any of cgroup BPF programs attached through BPF link.
> >> Good point. I have a 'bpf_prog_inc(prog)' below, I could do a refcount increase
> >> before sock_map_prog_update(), we then should be okay.
> >>
> > My point was that once you attach a program to sockmap with
> > LINK_CREATE, someone else just doing bpf_prog_attac() shouldn't
> > replace this program anymore. BPF link preserves *attachment
> > lifetime*, not just the program lifetime.
>
> I see. I briefly looked at xdp and cgroup where both support
> link-based attachment and program base attachment.
> For xdp, indeed if link attachment is already done,
> prog attach is not allowed.
> cgroup seems more complicated and need further study to
> confirm whether BPF link preserves attachment in all
> cases or not.

it does, I implemented it, it was an explicit design decision


> But any way, since bpf_link is a new feature for
> sockmap. It makes sense to follow the xdp-link
> style to disallow bpf_prog_attach once link is
> created.
>
> >
> >>>> +       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;
> >>>> +}
> >>> [...]

[PATCH bpf-next v2 2/6] libbpf: Add new sec_def "sk_skb/verdict"

[Yonghong Song]

The new sec_def specifies sk_skb program type with
BPF_SK_SKB_VERDICT attachment type. This way, libbpf
will set expected_attach_type properly for the program.
This will make it easier for later bpf_link based
APIs for sk_skb programs.

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

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 3a756d61c120..7d413415d0d5 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9312,6 +9312,7 @@ static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("sockops",		SOCK_OPS, BPF_CGROUP_SOCK_OPS, SEC_ATTACHABLE_OPT),
 	SEC_DEF("sk_skb/stream_parser",	SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
 	SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, SEC_ATTACHABLE_OPT),
+	SEC_DEF("sk_skb/verdict",	SK_SKB, BPF_SK_SKB_VERDICT, SEC_ATTACHABLE_OPT),
 	SEC_DEF("sk_skb",		SK_SKB, 0, SEC_NONE),
 	SEC_DEF("sk_msg",		SK_MSG, BPF_SK_MSG_VERDICT, SEC_ATTACHABLE_OPT),
 	SEC_DEF("lirc_mode2",		LIRC_MODE2, BPF_LIRC_MODE2, SEC_ATTACHABLE_OPT),
-- 
2.43.0

Re: [PATCH bpf-next v2 2/6] libbpf: Add new sec_def "sk_skb/verdict"

[Andrii Nakryiko]

On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> The new sec_def specifies sk_skb program type with
> BPF_SK_SKB_VERDICT attachment type. This way, libbpf
> will set expected_attach_type properly for the program.
> This will make it easier for later bpf_link based
> APIs for sk_skb programs.
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c | 1 +
>  1 file changed, 1 insertion(+)
>

Seems like this was an omission and this program type is already
supported, right? Should we apply this to libbpf regardless of BPF
link stuff?

Acked-by: Andrii Nakryiko <andrii@kernel.org>


> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3a756d61c120..7d413415d0d5 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -9312,6 +9312,7 @@ static const struct bpf_sec_def section_defs[] = {
>         SEC_DEF("sockops",              SOCK_OPS, BPF_CGROUP_SOCK_OPS, SEC_ATTACHABLE_OPT),
>         SEC_DEF("sk_skb/stream_parser", SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
>         SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, SEC_ATTACHABLE_OPT),
> +       SEC_DEF("sk_skb/verdict",       SK_SKB, BPF_SK_SKB_VERDICT, SEC_ATTACHABLE_OPT),
>         SEC_DEF("sk_skb",               SK_SKB, 0, SEC_NONE),
>         SEC_DEF("sk_msg",               SK_MSG, BPF_SK_MSG_VERDICT, SEC_ATTACHABLE_OPT),
>         SEC_DEF("lirc_mode2",           LIRC_MODE2, BPF_LIRC_MODE2, SEC_ATTACHABLE_OPT),
> --
> 2.43.0
>
>

Re: [PATCH bpf-next v2 2/6] libbpf: Add new sec_def "sk_skb/verdict"

[Yonghong Song]

On 3/22/24 11:47 AM, Andrii Nakryiko wrote:
> On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>> The new sec_def specifies sk_skb program type with
>> BPF_SK_SKB_VERDICT attachment type. This way, libbpf
>> will set expected_attach_type properly for the program.
>> This will make it easier for later bpf_link based
>> APIs for sk_skb programs.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/lib/bpf/libbpf.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
> Seems like this was an omission and this program type is already
> supported, right? Should we apply this to libbpf regardless of BPF
> link stuff?

Yes. Feel freel to apply this if you want.

>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 3a756d61c120..7d413415d0d5 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -9312,6 +9312,7 @@ static const struct bpf_sec_def section_defs[] = {
>>          SEC_DEF("sockops",              SOCK_OPS, BPF_CGROUP_SOCK_OPS, SEC_ATTACHABLE_OPT),
>>          SEC_DEF("sk_skb/stream_parser", SK_SKB, BPF_SK_SKB_STREAM_PARSER, SEC_ATTACHABLE_OPT),
>>          SEC_DEF("sk_skb/stream_verdict",SK_SKB, BPF_SK_SKB_STREAM_VERDICT, SEC_ATTACHABLE_OPT),
>> +       SEC_DEF("sk_skb/verdict",       SK_SKB, BPF_SK_SKB_VERDICT, SEC_ATTACHABLE_OPT),
>>          SEC_DEF("sk_skb",               SK_SKB, 0, SEC_NONE),
>>          SEC_DEF("sk_msg",               SK_MSG, BPF_SK_MSG_VERDICT, SEC_ATTACHABLE_OPT),
>>          SEC_DEF("lirc_mode2",           LIRC_MODE2, BPF_LIRC_MODE2, SEC_ATTACHABLE_OPT),
>> --
>> 2.43.0
>>
>>

[PATCH bpf-next v2 3/6] libbpf: Add bpf_link support for BPF_PROG_TYPE_SK_{MSG,SKB}

[Yonghong Song]

Introduce two libbpf API functions bpf_program__attach_sk_msg()
and bpf_program__attach_sk_skb() which allow user to get a bpf_link
for their corresponding programs.

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

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7d413415d0d5..1b8e1f47a5e6 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -149,6 +149,8 @@ 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",
+	[BPF_LINK_TYPE_SK_SKB]			= "sk_skb",
 };
 
 static const char * const map_type_name[] = {
@@ -12493,6 +12495,18 @@ bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd)
 	return bpf_program_attach_fd(prog, netns_fd, "netns", NULL);
 }
 
+struct bpf_link *
+bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd)
+{
+	return bpf_program_attach_fd(prog, map_fd, "sk_msg", NULL);
+}
+
+struct bpf_link *
+bpf_program__attach_sk_skb(const struct bpf_program *prog, int map_fd)
+{
+	return bpf_program_attach_fd(prog, map_fd, "sk_skb", NULL);
+}
+
 struct bpf_link *bpf_program__attach_xdp(const struct bpf_program *prog, int ifindex)
 {
 	/* target_fd/target_ifindex use the same field in LINK_CREATE */
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 7b510761f545..4a2c04a9c6c3 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -784,6 +784,10 @@ bpf_program__attach_cgroup(const struct bpf_program *prog, int cgroup_fd);
 LIBBPF_API struct bpf_link *
 bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd);
 LIBBPF_API struct bpf_link *
+bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd);
+LIBBPF_API struct bpf_link *
+bpf_program__attach_sk_skb(const struct bpf_program *prog, int map_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_freplace(const struct bpf_program *prog,
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 86804fd90dd1..11a1ad798129 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -410,6 +410,8 @@ LIBBPF_1.3.0 {
 
 LIBBPF_1.4.0 {
 	global:
+		bpf_program__attach_sk_msg;
+		bpf_program__attach_sk_skb;
 		bpf_token_create;
 		btf__new_split;
 		btf_ext__raw_data;
-- 
2.43.0

Re: [PATCH bpf-next v2 3/6] libbpf: Add bpf_link support for BPF_PROG_TYPE_SK_{MSG,SKB}

[Andrii Nakryiko]

On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> Introduce two libbpf API functions bpf_program__attach_sk_msg()
> and bpf_program__attach_sk_skb() which allow user to get a bpf_link
> for their corresponding programs.
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---
>  tools/lib/bpf/libbpf.c   | 14 ++++++++++++++
>  tools/lib/bpf/libbpf.h   |  4 ++++
>  tools/lib/bpf/libbpf.map |  2 ++
>  3 files changed, 20 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 7d413415d0d5..1b8e1f47a5e6 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -149,6 +149,8 @@ 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",
> +       [BPF_LINK_TYPE_SK_SKB]                  = "sk_skb",
>  };
>
>  static const char * const map_type_name[] = {
> @@ -12493,6 +12495,18 @@ bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd)
>         return bpf_program_attach_fd(prog, netns_fd, "netns", NULL);
>  }
>
> +struct bpf_link *
> +bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd)
> +{
> +       return bpf_program_attach_fd(prog, map_fd, "sk_msg", NULL);
> +}
> +
> +struct bpf_link *
> +bpf_program__attach_sk_skb(const struct bpf_program *prog, int map_fd)
> +{
> +       return bpf_program_attach_fd(prog, map_fd, "sk_skb", NULL);
> +}
> +
>  struct bpf_link *bpf_program__attach_xdp(const struct bpf_program *prog, int ifindex)
>  {
>         /* target_fd/target_ifindex use the same field in LINK_CREATE */
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 7b510761f545..4a2c04a9c6c3 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -784,6 +784,10 @@ bpf_program__attach_cgroup(const struct bpf_program *prog, int cgroup_fd);
>  LIBBPF_API struct bpf_link *
>  bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd);
>  LIBBPF_API struct bpf_link *
> +bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_sk_skb(const struct bpf_program *prog, int map_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_freplace(const struct bpf_program *prog,
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 86804fd90dd1..11a1ad798129 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -410,6 +410,8 @@ LIBBPF_1.3.0 {
>
>  LIBBPF_1.4.0 {
>         global:
> +               bpf_program__attach_sk_msg;
> +               bpf_program__attach_sk_skb;

same suggestion, it seems like having a more generic
bpf_program__attach_sockmap() or something along those lines would be
better?

please also see what changes need to be done in bpf_link_create() API

>                 bpf_token_create;
>                 btf__new_split;
>                 btf_ext__raw_data;
> --
> 2.43.0
>

Re: [PATCH bpf-next v2 3/6] libbpf: Add bpf_link support for BPF_PROG_TYPE_SK_{MSG,SKB}

[Yonghong Song]

On 3/22/24 11:48 AM, Andrii Nakryiko wrote:
> On Tue, Mar 19, 2024 at 10:54 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>> Introduce two libbpf API functions bpf_program__attach_sk_msg()
>> and bpf_program__attach_sk_skb() which allow user to get a bpf_link
>> for their corresponding programs.
>>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>>   tools/lib/bpf/libbpf.c   | 14 ++++++++++++++
>>   tools/lib/bpf/libbpf.h   |  4 ++++
>>   tools/lib/bpf/libbpf.map |  2 ++
>>   3 files changed, 20 insertions(+)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 7d413415d0d5..1b8e1f47a5e6 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -149,6 +149,8 @@ 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",
>> +       [BPF_LINK_TYPE_SK_SKB]                  = "sk_skb",
>>   };
>>
>>   static const char * const map_type_name[] = {
>> @@ -12493,6 +12495,18 @@ bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd)
>>          return bpf_program_attach_fd(prog, netns_fd, "netns", NULL);
>>   }
>>
>> +struct bpf_link *
>> +bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd)
>> +{
>> +       return bpf_program_attach_fd(prog, map_fd, "sk_msg", NULL);
>> +}
>> +
>> +struct bpf_link *
>> +bpf_program__attach_sk_skb(const struct bpf_program *prog, int map_fd)
>> +{
>> +       return bpf_program_attach_fd(prog, map_fd, "sk_skb", NULL);
>> +}
>> +
>>   struct bpf_link *bpf_program__attach_xdp(const struct bpf_program *prog, int ifindex)
>>   {
>>          /* target_fd/target_ifindex use the same field in LINK_CREATE */
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 7b510761f545..4a2c04a9c6c3 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -784,6 +784,10 @@ bpf_program__attach_cgroup(const struct bpf_program *prog, int cgroup_fd);
>>   LIBBPF_API struct bpf_link *
>>   bpf_program__attach_netns(const struct bpf_program *prog, int netns_fd);
>>   LIBBPF_API struct bpf_link *
>> +bpf_program__attach_sk_msg(const struct bpf_program *prog, int map_fd);
>> +LIBBPF_API struct bpf_link *
>> +bpf_program__attach_sk_skb(const struct bpf_program *prog, int map_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_freplace(const struct bpf_program *prog,
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index 86804fd90dd1..11a1ad798129 100644
>> --- a/tools/lib/bpf/libbpf.map
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -410,6 +410,8 @@ LIBBPF_1.3.0 {
>>
>>   LIBBPF_1.4.0 {
>>          global:
>> +               bpf_program__attach_sk_msg;
>> +               bpf_program__attach_sk_skb;
> same suggestion, it seems like having a more generic
> bpf_program__attach_sockmap() or something along those lines would be
> better?

If we will have one link type, I think we should have one attach API
as well.

>
> please also see what changes need to be done in bpf_link_create() API

Will do.

>
>>                  bpf_token_create;
>>                  btf__new_split;
>>                  btf_ext__raw_data;
>> --
>> 2.43.0
>>

[PATCH bpf-next v2 4/6] bpftool: Add link dump support for BPF_LINK_TYPE_SK_{MSG,SKB}

[Yonghong Song]

An example output looks like:
  $ bpftool link
    1776: sk_skb  prog 49730
            map_id 0  attach_type sk_skb_verdict
            pids test_progs(8424)
    1777: sk_skb  prog 49755
            map_id 0  attach_type sk_skb_stream_verdict
            pids test_progs(8424)
    1778: sk_msg  prog 49770
            map_id 8208  attach_type sk_msg_verdict
            pids test_progs(8424)

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

diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index afde9d0c2ea1..c01d3c38cdf0 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -526,6 +526,14 @@ 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);
+		break;
+	case BPF_LINK_TYPE_SK_SKB:
+		jsonw_uint_field(json_wtr, "map_id", info->skskb.map_id);
+		show_link_attach_type_json(info->skskb.attach_type, json_wtr);
+		break;
 	case BPF_LINK_TYPE_XDP:
 		show_link_ifindex_json(info->xdp.ifindex, json_wtr);
 		break;
@@ -915,6 +923,16 @@ 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_SK_SKB:
+		printf("\n\t");
+		printf("map_id %u  ", info->skskb.map_id);
+		show_link_attach_type_plain(info->skskb.attach_type);
+		break;
 	case BPF_LINK_TYPE_XDP:
 		printf("\n\t");
 		show_link_ifindex_plain(info->xdp.ifindex);
-- 
2.43.0

Re: [PATCH bpf-next v2 4/6] bpftool: Add link dump support for BPF_LINK_TYPE_SK_{MSG,SKB}

[Quentin Monnet]

2024-03-19 17:54 UTC+0000 ~ Yonghong Song <yonghong.song@linux.dev>
> An example output looks like:
>   $ bpftool link
>     1776: sk_skb  prog 49730
>             map_id 0  attach_type sk_skb_verdict
>             pids test_progs(8424)
>     1777: sk_skb  prog 49755
>             map_id 0  attach_type sk_skb_stream_verdict
>             pids test_progs(8424)
>     1778: sk_msg  prog 49770
>             map_id 8208  attach_type sk_msg_verdict
>             pids test_progs(8424)
> 
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>

Reviewed-by: Quentin Monnet <qmo@kernel.org>

Thanks!

[PATCH bpf-next v2 5/6] selftests/bpf: Refactor out helper functions for a few tests

[Yonghong Song]

These helper functions will be used later new tests as well.
There are no functionality change.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 38 +++++++++++--------
 .../bpf/progs/test_skmsg_load_helpers.c       |  9 ++++-
 2 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 77e26ecffa9d..63fb2da7930a 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -475,30 +475,19 @@ static void test_sockmap_skb_verdict_fionread(bool pass_prog)
 		test_sockmap_drop_prog__destroy(drop);
 }
 
-static void test_sockmap_skb_verdict_peek(void)
+static void test_sockmap_skb_verdict_peek_helper(int map)
 {
-	int err, map, verdict, s, c1, p1, zero = 0, sent, recvd, avail;
-	struct test_sockmap_pass_prog *pass;
+	int err, s, c1, p1, zero = 0, sent, recvd, avail;
 	char snd[256] = "0123456789";
 	char rcv[256] = "0";
 
-	pass = test_sockmap_pass_prog__open_and_load();
-	if (!ASSERT_OK_PTR(pass, "open_and_load"))
-		return;
-	verdict = bpf_program__fd(pass->progs.prog_skb_verdict);
-	map = bpf_map__fd(pass->maps.sock_map_rx);
-
-	err = bpf_prog_attach(verdict, map, BPF_SK_SKB_STREAM_VERDICT, 0);
-	if (!ASSERT_OK(err, "bpf_prog_attach"))
-		goto out;
-
 	s = socket_loopback(AF_INET, SOCK_STREAM);
 	if (!ASSERT_GT(s, -1, "socket_loopback(s)"))
-		goto out;
+		return;
 
 	err = create_pair(s, AF_INET, SOCK_STREAM, &c1, &p1);
 	if (!ASSERT_OK(err, "create_pairs(s)"))
-		goto out;
+		return;
 
 	err = bpf_map_update_elem(map, &zero, &c1, BPF_NOEXIST);
 	if (!ASSERT_OK(err, "bpf_map_update_elem(c1)"))
@@ -520,6 +509,25 @@ static void test_sockmap_skb_verdict_peek(void)
 out_close:
 	close(c1);
 	close(p1);
+}
+
+static void test_sockmap_skb_verdict_peek(void)
+{
+	struct test_sockmap_pass_prog *pass;
+	int err, map, verdict;
+
+	pass = test_sockmap_pass_prog__open_and_load();
+	if (!ASSERT_OK_PTR(pass, "open_and_load"))
+		return;
+	verdict = bpf_program__fd(pass->progs.prog_skb_verdict);
+	map = bpf_map__fd(pass->maps.sock_map_rx);
+
+	err = bpf_prog_attach(verdict, map, BPF_SK_SKB_STREAM_VERDICT, 0);
+	if (!ASSERT_OK(err, "bpf_prog_attach"))
+		goto out;
+
+	test_sockmap_skb_verdict_peek_helper(map);
+
 out:
 	test_sockmap_pass_prog__destroy(pass);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c b/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
index 45e8fc75a739..b753672f04c9 100644
--- a/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
+++ b/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
@@ -24,8 +24,7 @@ struct {
 	__type(value, __u64);
 } socket_storage SEC(".maps");
 
-SEC("sk_msg")
-int prog_msg_verdict(struct sk_msg_md *msg)
+static int prog_msg_verdict_common(struct sk_msg_md *msg)
 {
 	struct task_struct *task = (struct task_struct *)bpf_get_current_task();
 	int verdict = SK_PASS;
@@ -44,4 +43,10 @@ int prog_msg_verdict(struct sk_msg_md *msg)
 	return verdict;
 }
 
+SEC("sk_msg")
+int prog_msg_verdict(struct sk_msg_md *msg)
+{
+	return prog_msg_verdict_common(msg);
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.43.0

[PATCH bpf-next v2 6/6] selftests/bpf: Add some tests with new bpf_program__attach_sk_{msg,skb}() APIs

[Yonghong Song]

Add a few more tests in sockmap_basic.c and sockmap_listen.c to
test bpf_link based APIs for SK_MSG and SK_SKB programs.
Link attach/detach/update are all tested.

All tests are passed.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 83 +++++++++++++++++++
 .../selftests/bpf/prog_tests/sockmap_listen.c | 38 +++++++++
 .../bpf/progs/test_skmsg_load_helpers.c       |  6 ++
 .../bpf/progs/test_sockmap_pass_prog.c        | 11 ++-
 .../progs/test_sockmap_skb_verdict_attach.c   |  2 +-
 5 files changed, 138 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 63fb2da7930a..85a97d0d20c6 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -131,6 +131,33 @@ 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 err, 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);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sk_msg"))
+		goto out;
+
+	err = bpf_link__update_program(link, skel->progs.prog_msg_verdict_clone);
+	if (!ASSERT_OK(err, "bpf_link__update_program"))
+		goto out;
+
+	bpf_link__detach(link);
+out:
+	test_skmsg_load_helpers__destroy(skel);
+}
+
 static void test_sockmap_update(enum bpf_map_type map_type)
 {
 	int err, prog, src;
@@ -298,6 +325,27 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
 	test_sockmap_skb_verdict_attach__destroy(skel);
 }
 
+static void test_sockmap_skb_verdict_attach_with_link(void)
+{
+	struct test_sockmap_skb_verdict_attach *skel;
+	struct bpf_program *prog;
+	struct bpf_link *link;
+	int map;
+
+	skel = test_sockmap_skb_verdict_attach__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+	prog = skel->progs.prog_skb_verdict;
+	map = bpf_map__fd(skel->maps.sock_map);
+	link = bpf_program__attach_sk_skb(prog, map);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sk_skb"))
+		goto out;
+
+	bpf_link__detach(link);
+out:
+	test_sockmap_skb_verdict_attach__destroy(skel);
+}
+
 static __u32 query_prog_id(int prog_fd)
 {
 	struct bpf_prog_info info = {};
@@ -532,6 +580,33 @@ static void test_sockmap_skb_verdict_peek(void)
 	test_sockmap_pass_prog__destroy(pass);
 }
 
+static void test_sockmap_skb_verdict_peek_with_link(void)
+{
+	struct test_sockmap_pass_prog *pass;
+	struct bpf_program *prog;
+	struct bpf_link *link;
+	int err, map;
+
+	pass = test_sockmap_pass_prog__open_and_load();
+	if (!ASSERT_OK_PTR(pass, "open_and_load"))
+		return;
+	prog = pass->progs.prog_skb_verdict;
+	map = bpf_map__fd(pass->maps.sock_map_rx);
+	link = bpf_program__attach_sk_skb(prog, map);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sk_skb"))
+		goto out;
+
+	err = bpf_link__update_program(link, pass->progs.prog_skb_verdict_clone);
+	if (!ASSERT_OK(err, "bpf_link__update_program"))
+		goto out;
+
+	test_sockmap_skb_verdict_peek_helper(map);
+	ASSERT_EQ(pass->bss->clone_called, 1, "clone_called");
+out:
+	bpf_link__detach(link);
+	test_sockmap_pass_prog__destroy(pass);
+}
+
 static void test_sockmap_unconnected_unix(void)
 {
 	int err, map, stream = 0, dgram = 0, zero = 0;
@@ -796,6 +871,8 @@ void test_sockmap_basic(void)
 		test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
 						BPF_SK_SKB_VERDICT);
 	}
+	if (test__start_subtest("sockmap skb_verdict attach_with_link"))
+		test_sockmap_skb_verdict_attach_with_link();
 	if (test__start_subtest("sockmap msg_verdict progs query"))
 		test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
 	if (test__start_subtest("sockmap stream_parser progs query"))
@@ -812,6 +889,8 @@ void test_sockmap_basic(void)
 		test_sockmap_skb_verdict_fionread(false);
 	if (test__start_subtest("sockmap skb_verdict msg_f_peek"))
 		test_sockmap_skb_verdict_peek();
+	if (test__start_subtest("sockmap skb_verdict msg_f_peek with link"))
+		test_sockmap_skb_verdict_peek_with_link();
 	if (test__start_subtest("sockmap unconnected af_unix"))
 		test_sockmap_unconnected_unix();
 	if (test__start_subtest("sockmap one socket to many map entries"))
@@ -820,4 +899,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 sockmap helpers with link"))
+		test_skmsg_helpers_with_link(BPF_MAP_TYPE_SOCKMAP);
+	if (test__start_subtest("sockhash sk_msg attach sockhash 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..426dc14912ee 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);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sk_msg"))
+		return;
+
+	redir_to_connected(family, sotype, sock_map, verdict_map, REDIR_EGRESS);
+
+	bpf_link__detach(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);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sk_msg"))
+		return;
+
+	redir_to_listening(family, sotype, sock_map, verdict_map, REDIR_EGRESS);
+
+	bpf_link__detach(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;
diff --git a/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c b/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
index b753672f04c9..2914fbf1b05b 100644
--- a/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
+++ b/tools/testing/selftests/bpf/progs/test_skmsg_load_helpers.c
@@ -49,4 +49,10 @@ int prog_msg_verdict(struct sk_msg_md *msg)
 	return prog_msg_verdict_common(msg);
 }
 
+SEC("sk_msg")
+int prog_msg_verdict_clone(struct sk_msg_md *msg)
+{
+	return prog_msg_verdict_common(msg);
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_pass_prog.c b/tools/testing/selftests/bpf/progs/test_sockmap_pass_prog.c
index 1d86a717a290..032c3c6310d4 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_pass_prog.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_pass_prog.c
@@ -23,10 +23,19 @@ struct {
 	__type(value, int);
 } sock_map_msg SEC(".maps");
 
-SEC("sk_skb")
+SEC("sk_skb/stream_verdict")
 int prog_skb_verdict(struct __sk_buff *skb)
 {
 	return SK_PASS;
 }
 
+int clone_called;
+
+SEC("sk_skb/stream_verdict")
+int prog_skb_verdict_clone(struct __sk_buff *skb)
+{
+	clone_called = 1;
+	return SK_PASS;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c b/tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c
index 3c69aa971738..d25b0bb30fc0 100644
--- a/tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_skb_verdict_attach.c
@@ -9,7 +9,7 @@ struct {
 	__type(value, __u64);
 } sock_map SEC(".maps");
 
-SEC("sk_skb")
+SEC("sk_skb/verdict")
 int prog_skb_verdict(struct __sk_buff *skb)
 {
 	return SK_DROP;
-- 
2.43.0