* [PATCH bpf-next 0/2] Update a struct_ops link through a pinned path
@ 2024-04-17 0:25 Kui-Feng Lee
2024-04-17 0:25 ` [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link Kui-Feng Lee
2024-04-17 0:25 ` [PATCH bpf-next 2/2] selftests/bpf: open a pinned path of a struct_ops link Kui-Feng Lee
0 siblings, 2 replies; 13+ messages in thread
From: Kui-Feng Lee @ 2024-04-17 0:25 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii
Cc: sinquersw, kuifeng, Kui-Feng Lee
Applications already have the ability to update a struct_ops link with
another struct_ops map. However, they were unable to open pinned paths
of the links. This implies that updating a link through its pinned
paths was not feasible. By allowing the "open" operator on pinned
paths, applications can pin a struct_ops link and update the link
through the pinned path later.
Kui-Feng Lee (2):
bpf: enable the "open" operator on a pinned path of a struct_osp link.
selftests/bpf: open a pinned path of a struct_ops link.
include/linux/bpf.h | 4 ++
kernel/bpf/bpf_struct_ops.c | 10 ++++
kernel/bpf/inode.c | 11 +++-
kernel/bpf/syscall.c | 14 ++++-
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 6 ++
.../bpf/prog_tests/test_struct_ops_module.c | 56 +++++++++++++++++++
6 files changed, 97 insertions(+), 4 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-17 0:25 [PATCH bpf-next 0/2] Update a struct_ops link through a pinned path Kui-Feng Lee
@ 2024-04-17 0:25 ` Kui-Feng Lee
2024-04-17 23:19 ` kernel test robot
` (2 more replies)
2024-04-17 0:25 ` [PATCH bpf-next 2/2] selftests/bpf: open a pinned path of a struct_ops link Kui-Feng Lee
1 sibling, 3 replies; 13+ messages in thread
From: Kui-Feng Lee @ 2024-04-17 0:25 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii
Cc: sinquersw, kuifeng, Kui-Feng Lee
Add the "open" operator for the inodes of BPF links to allow applications
to obtain a file descriptor of a struct_ops link from a pinned path.
Applications have the ability to update a struct_ops link with another
struct_ops map. However, they were unable to open pinned paths of the links
with this patch. This implies that updating a link through its pinned paths
was not feasible.
This patch adds the "open" operator to bpf_link_ops and uses bpf_link_ops
as the i_fop for inodes of struct_ops links. "open" will be called to open
the pinned path represented by an inode. Additionally, bpf_link_ops will be
used as the f->f_ops of the opened "file" to provide operators for the
"file".
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
include/linux/bpf.h | 4 ++++
kernel/bpf/bpf_struct_ops.c | 10 ++++++++++
kernel/bpf/inode.c | 11 ++++++++---
kernel/bpf/syscall.c | 14 +++++++++++++-
4 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5034c1b4ded7..23b831ec9b71 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2160,6 +2160,10 @@ extern const struct super_operations bpf_super_ops;
extern const struct file_operations bpf_map_fops;
extern const struct file_operations bpf_prog_fops;
extern const struct file_operations bpf_iter_fops;
+extern const struct file_operations bpf_link_fops;
+
+/* Required by bpf_link_open() */
+int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp);
#define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type) \
extern const struct bpf_prog_ops _name ## _prog_ops; \
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index 86c7884abaf8..8be4f755a182 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -1198,3 +1198,13 @@ void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map
info->btf_vmlinux_id = btf_obj_id(st_map->btf);
}
+
+int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
+{
+ struct bpf_struct_ops_link *link = inode->i_private;
+
+ /* Paired with bpf_link_put_direct() in bpf_link_release(). */
+ bpf_link_inc(&link->link);
+ filp->private_data = link;
+ return 0;
+}
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index af5d2ffadd70..b020d761ab0a 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry, umode_t mode, void *arg)
static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
{
+ const struct file_operations *fops;
struct bpf_link *link = arg;
- return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
- bpf_link_is_iter(link) ?
- &bpf_iter_fops : &bpffs_obj_fops);
+ if (bpf_link_is_iter(link))
+ fops = &bpf_iter_fops;
+ else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
+ fops = &bpf_link_fops;
+ else
+ fops = &bpffs_obj_fops;
+ return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops, fops);
}
static struct dentry *
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7d392ec83655..f66bc6215faa 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3108,7 +3108,19 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp)
}
#endif
-static const struct file_operations bpf_link_fops = {
+/* Support opening pinned links */
+static int bpf_link_open(struct inode *inode, struct file *filp)
+{
+ struct bpf_link *link = inode->i_private;
+
+ if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
+ return bpffs_struct_ops_link_open(inode, filp);
+
+ return -EOPNOTSUPP;
+}
+
+const struct file_operations bpf_link_fops = {
+ .open = bpf_link_open,
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_link_show_fdinfo,
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: open a pinned path of a struct_ops link.
2024-04-17 0:25 [PATCH bpf-next 0/2] Update a struct_ops link through a pinned path Kui-Feng Lee
2024-04-17 0:25 ` [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link Kui-Feng Lee
@ 2024-04-17 0:25 ` Kui-Feng Lee
1 sibling, 0 replies; 13+ messages in thread
From: Kui-Feng Lee @ 2024-04-17 0:25 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii
Cc: sinquersw, kuifeng, Kui-Feng Lee
Ensure that a pinned path of a struct_ops link can be opened to obtain a
file descriptor, which applications can then utilize to update the link.
Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 6 ++
.../bpf/prog_tests/test_struct_ops_module.c | 56 +++++++++++++++++++
2 files changed, 62 insertions(+)
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 39ad96a18123..c4acd4ec630c 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -579,6 +579,11 @@ static void bpf_dummy_unreg(void *kdata)
{
}
+static int bpf_dummy_update(void *kdata, void *old_kdata)
+{
+ return bpf_dummy_reg(kdata);
+}
+
static int bpf_testmod_test_1(void)
{
return 0;
@@ -606,6 +611,7 @@ struct bpf_struct_ops bpf_bpf_testmod_ops = {
.init_member = bpf_testmod_ops_init_member,
.reg = bpf_dummy_reg,
.unreg = bpf_dummy_unreg,
+ .update = bpf_dummy_update,
.cfi_stubs = &__bpf_testmod_ops,
.name = "bpf_testmod_ops",
.owner = THIS_MODULE,
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
index 7cf2b9ddd3e1..47b965c4c3e1 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c
@@ -160,6 +160,60 @@ static void test_struct_ops_incompatible(void)
struct_ops_module__destroy(skel);
}
+/* Applications should be able to open a pinned path of a struct_ops link
+ * to get a file descriptor of the link and to update the link through the
+ * file descriptor.
+ */
+static void test_struct_ops_pinning_and_open(void)
+{
+ DECLARE_LIBBPF_OPTS(bpf_link_update_opts, opts);
+ struct struct_ops_module *skel;
+ int err, link_fd = -1, map_fd;
+ struct bpf_link *link;
+
+ /* Create and pin a struct_ops link */
+ skel = struct_ops_module__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "open_and_load"))
+ return;
+
+ link = bpf_map__attach_struct_ops(skel->maps.testmod_1);
+ if (!ASSERT_OK_PTR(link, "attach_struct_ops"))
+ goto cleanup;
+
+ err = bpf_link__pin(link, "/sys/fs/bpf/test_struct_ops_pinning");
+ if (!ASSERT_OK(err, "bpf_link__pin"))
+ goto cleanup;
+
+ /* Open the pinned path */
+ link_fd = open("/sys/fs/bpf/test_struct_ops_pinning", O_RDONLY);
+ bpf_link__unpin(link);
+ if (!ASSERT_GE(link_fd, 0, "open_pinned"))
+ goto cleanup;
+
+ skel->bss->test_1_result = 0;
+ skel->bss->test_2_result = 0;
+
+ map_fd = bpf_map__fd(skel->maps.testmod_1);
+ if (!ASSERT_GE(map_fd, 0, "map_fd"))
+ goto cleanup;
+
+ /* Update the link. test_1 and test_2 should be called again. */
+ err = bpf_link_update(link_fd, map_fd, &opts);
+ if (!ASSERT_OK(err, "bpf_link_update"))
+ goto cleanup;
+
+ /* Check if test_1 and test_2 have been called */
+ ASSERT_EQ(skel->bss->test_1_result, 0xdeadbeef,
+ "bpf_link_update_test_1_result");
+ ASSERT_EQ(skel->bss->test_2_result, 5,
+ "bpf_link_update_test_2_result");
+
+cleanup:
+ close(link_fd);
+ bpf_link__destroy(link);
+ struct_ops_module__destroy(skel);
+}
+
void serial_test_struct_ops_module(void)
{
if (test__start_subtest("test_struct_ops_load"))
@@ -168,5 +222,7 @@ void serial_test_struct_ops_module(void)
test_struct_ops_not_zeroed();
if (test__start_subtest("test_struct_ops_incompatible"))
test_struct_ops_incompatible();
+ if (test__start_subtest("test_struct_ops_pinning_and_open"))
+ test_struct_ops_pinning_and_open();
}
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-17 0:25 ` [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link Kui-Feng Lee
@ 2024-04-17 23:19 ` kernel test robot
2024-04-18 6:13 ` kernel test robot
2024-04-20 0:05 ` Martin KaFai Lau
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-04-17 23:19 UTC (permalink / raw)
To: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii
Cc: llvm, oe-kbuild-all, sinquersw, kuifeng, Kui-Feng Lee
Hi Kui-Feng,
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/Kui-Feng-Lee/bpf-enable-the-open-operator-on-a-pinned-path-of-a-struct_osp-link/20240417-082736
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20240417002513.1534535-2-thinker.li%40gmail.com
patch subject: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240418/202404180747.NXWUtZTG-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 7089c359a3845323f6f30c44a47dd901f2edfe63)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240418/202404180747.NXWUtZTG-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/202404180747.NXWUtZTG-lkp@intel.com/
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: bpffs_struct_ops_link_open
>>> referenced by syscall.c
>>> kernel/bpf/syscall.o:(bpf_link_open) in archive vmlinux.a
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-17 0:25 ` [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link Kui-Feng Lee
2024-04-17 23:19 ` kernel test robot
@ 2024-04-18 6:13 ` kernel test robot
2024-04-20 0:05 ` Martin KaFai Lau
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-04-18 6:13 UTC (permalink / raw)
To: Kui-Feng Lee, bpf, ast, martin.lau, song, kernel-team, andrii
Cc: oe-kbuild-all, sinquersw, kuifeng, Kui-Feng Lee
Hi Kui-Feng,
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/Kui-Feng-Lee/bpf-enable-the-open-operator-on-a-pinned-path-of-a-struct_osp-link/20240417-082736
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20240417002513.1534535-2-thinker.li%40gmail.com
patch subject: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
config: arm-aspeed_g5_defconfig (https://download.01.org/0day-ci/archive/20240418/202404181413.1uMDy1xi-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240418/202404181413.1uMDy1xi-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/202404181413.1uMDy1xi-lkp@intel.com/
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: kernel/bpf/syscall.o: in function `bpf_link_open':
>> kernel/bpf/syscall.c:3117:(.text+0xb70): undefined reference to `bpffs_struct_ops_link_open'
vim +3117 kernel/bpf/syscall.c
3110
3111 /* Support opening pinned links */
3112 static int bpf_link_open(struct inode *inode, struct file *filp)
3113 {
3114 struct bpf_link *link = inode->i_private;
3115
3116 if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
> 3117 return bpffs_struct_ops_link_open(inode, filp);
3118
3119 return -EOPNOTSUPP;
3120 }
3121
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-17 0:25 ` [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link Kui-Feng Lee
2024-04-17 23:19 ` kernel test robot
2024-04-18 6:13 ` kernel test robot
@ 2024-04-20 0:05 ` Martin KaFai Lau
2024-04-22 17:12 ` Kui-Feng Lee
2 siblings, 1 reply; 13+ messages in thread
From: Martin KaFai Lau @ 2024-04-20 0:05 UTC (permalink / raw)
To: Kui-Feng Lee, andrii; +Cc: sinquersw, kuifeng, bpf, ast, song, kernel-team
On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
> +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
> +{
> + struct bpf_struct_ops_link *link = inode->i_private;
> +
> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
> + bpf_link_inc(&link->link);
> + filp->private_data = link;
> + return 0;
> +}
> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> index af5d2ffadd70..b020d761ab0a 100644
> --- a/kernel/bpf/inode.c
> +++ b/kernel/bpf/inode.c
> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry, umode_t mode, void *arg)
>
> static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
> {
> + const struct file_operations *fops;
> struct bpf_link *link = arg;
>
> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
> - bpf_link_is_iter(link) ?
> - &bpf_iter_fops : &bpffs_obj_fops);
> + if (bpf_link_is_iter(link))
> + fops = &bpf_iter_fops;
> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
Open a pinned link and then update should not be specific to struct_ops link.
e.g. should be useful to the cgroup link also?
Andrii, wdyt about supporting other link types also?
> + fops = &bpf_link_fops;
> + else
> + fops = &bpffs_obj_fops;
> + return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops, fops);
> }
>
> static struct dentry *
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 7d392ec83655..f66bc6215faa 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3108,7 +3108,19 @@ static void bpf_link_show_fdinfo(struct seq_file *m, struct file *filp)
> }
> #endif
>
> -static const struct file_operations bpf_link_fops = {
> +/* Support opening pinned links */
> +static int bpf_link_open(struct inode *inode, struct file *filp)
> +{
> + struct bpf_link *link = inode->i_private;
> +
> + if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
> + return bpffs_struct_ops_link_open(inode, filp);
> +
> + return -EOPNOTSUPP;
> +}
> +
> +const struct file_operations bpf_link_fops = {
> + .open = bpf_link_open,
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = bpf_link_show_fdinfo,
> #endif
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-20 0:05 ` Martin KaFai Lau
@ 2024-04-22 17:12 ` Kui-Feng Lee
2024-04-22 17:30 ` Kui-Feng Lee
0 siblings, 1 reply; 13+ messages in thread
From: Kui-Feng Lee @ 2024-04-22 17:12 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee, andrii
Cc: kuifeng, bpf, ast, song, kernel-team
On 4/19/24 17:05, Martin KaFai Lau wrote:
> On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
>> +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
>> +{
>> + struct bpf_struct_ops_link *link = inode->i_private;
>> +
>> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
>> + bpf_link_inc(&link->link);
>> + filp->private_data = link;
>> + return 0;
>> +}
>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
>> index af5d2ffadd70..b020d761ab0a 100644
>> --- a/kernel/bpf/inode.c
>> +++ b/kernel/bpf/inode.c
>> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry,
>> umode_t mode, void *arg)
>> static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
>> {
>> + const struct file_operations *fops;
>> struct bpf_link *link = arg;
>> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
>> - bpf_link_is_iter(link) ?
>> - &bpf_iter_fops : &bpffs_obj_fops);
>> + if (bpf_link_is_iter(link))
>> + fops = &bpf_iter_fops;
>> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
>
> Open a pinned link and then update should not be specific to struct_ops
> link. e.g. should be useful to the cgroup link also?
It could be. Here, I played safe in case it creates any unwanted side
effect for links of unknown types.
>
> Andrii, wdyt about supporting other link types also?
>
>> + fops = &bpf_link_fops;
>> + else
>> + fops = &bpffs_obj_fops;
>> + return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops, fops);
>> }
>> static struct dentry *
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 7d392ec83655..f66bc6215faa 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -3108,7 +3108,19 @@ static void bpf_link_show_fdinfo(struct
>> seq_file *m, struct file *filp)
>> }
>> #endif
>> -static const struct file_operations bpf_link_fops = {
>> +/* Support opening pinned links */
>> +static int bpf_link_open(struct inode *inode, struct file *filp)
>> +{
>> + struct bpf_link *link = inode->i_private;
>> +
>> + if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
>> + return bpffs_struct_ops_link_open(inode, filp);
>> +
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +const struct file_operations bpf_link_fops = {
>> + .open = bpf_link_open,
>> #ifdef CONFIG_PROC_FS
>> .show_fdinfo = bpf_link_show_fdinfo,
>> #endif
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-22 17:12 ` Kui-Feng Lee
@ 2024-04-22 17:30 ` Kui-Feng Lee
2024-04-22 23:43 ` Martin KaFai Lau
0 siblings, 1 reply; 13+ messages in thread
From: Kui-Feng Lee @ 2024-04-22 17:30 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee, andrii
Cc: kuifeng, bpf, ast, song, kernel-team
On 4/22/24 10:12, Kui-Feng Lee wrote:
>
>
> On 4/19/24 17:05, Martin KaFai Lau wrote:
>> On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
>>> +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
>>> +{
>>> + struct bpf_struct_ops_link *link = inode->i_private;
>>> +
>>> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
>>> + bpf_link_inc(&link->link);
>>> + filp->private_data = link;
>>> + return 0;
>>> +}
>>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
>>> index af5d2ffadd70..b020d761ab0a 100644
>>> --- a/kernel/bpf/inode.c
>>> +++ b/kernel/bpf/inode.c
>>> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry,
>>> umode_t mode, void *arg)
>>> static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
>>> {
>>> + const struct file_operations *fops;
>>> struct bpf_link *link = arg;
>>> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
>>> - bpf_link_is_iter(link) ?
>>> - &bpf_iter_fops : &bpffs_obj_fops);
>>> + if (bpf_link_is_iter(link))
>>> + fops = &bpf_iter_fops;
>>> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
>>
>> Open a pinned link and then update should not be specific to
>> struct_ops link. e.g. should be useful to the cgroup link also?
>
> It could be. Here, I played safe in case it creates any unwanted side
> effect for links of unknown types.
By the way, may I put it in a follow up patch if we want cgroup links?
>
>>
>> Andrii, wdyt about supporting other link types also?
>>
>>> + fops = &bpf_link_fops;
>>> + else
>>> + fops = &bpffs_obj_fops;
>>> + return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops, fops);
>>> }
>>> static struct dentry *
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index 7d392ec83655..f66bc6215faa 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -3108,7 +3108,19 @@ static void bpf_link_show_fdinfo(struct
>>> seq_file *m, struct file *filp)
>>> }
>>> #endif
>>> -static const struct file_operations bpf_link_fops = {
>>> +/* Support opening pinned links */
>>> +static int bpf_link_open(struct inode *inode, struct file *filp)
>>> +{
>>> + struct bpf_link *link = inode->i_private;
>>> +
>>> + if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
>>> + return bpffs_struct_ops_link_open(inode, filp);
>>> +
>>> + return -EOPNOTSUPP;
>>> +}
>>> +
>>> +const struct file_operations bpf_link_fops = {
>>> + .open = bpf_link_open,
>>> #ifdef CONFIG_PROC_FS
>>> .show_fdinfo = bpf_link_show_fdinfo,
>>> #endif
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-22 17:30 ` Kui-Feng Lee
@ 2024-04-22 23:43 ` Martin KaFai Lau
2024-04-23 17:16 ` Kui-Feng Lee
0 siblings, 1 reply; 13+ messages in thread
From: Martin KaFai Lau @ 2024-04-22 23:43 UTC (permalink / raw)
To: Kui-Feng Lee, Kui-Feng Lee; +Cc: andrii, kuifeng, bpf, ast, song, kernel-team
On 4/22/24 10:30 AM, Kui-Feng Lee wrote:
>
>
> On 4/22/24 10:12, Kui-Feng Lee wrote:
>>
>>
>> On 4/19/24 17:05, Martin KaFai Lau wrote:
>>> On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
>>>> +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
>>>> +{
>>>> + struct bpf_struct_ops_link *link = inode->i_private;
>>>> +
>>>> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
>>>> + bpf_link_inc(&link->link);
>>>> + filp->private_data = link;
>>>> + return 0;
>>>> +}
>>>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
>>>> index af5d2ffadd70..b020d761ab0a 100644
>>>> --- a/kernel/bpf/inode.c
>>>> +++ b/kernel/bpf/inode.c
>>>> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry, umode_t
>>>> mode, void *arg)
>>>> static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
>>>> {
>>>> + const struct file_operations *fops;
>>>> struct bpf_link *link = arg;
>>>> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
>>>> - bpf_link_is_iter(link) ?
>>>> - &bpf_iter_fops : &bpffs_obj_fops);
>>>> + if (bpf_link_is_iter(link))
>>>> + fops = &bpf_iter_fops;
>>>> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
>>>
>>> Open a pinned link and then update should not be specific to struct_ops link.
>>> e.g. should be useful to the cgroup link also?
>>
>> It could be. Here, I played safe in case it creates any unwanted side
>> effect for links of unknown types.
>
> By the way, may I put it in a follow up patch if we want cgroup links?
This does not feel right. It is not struct_ops specific.
Before we dive in further, there is BPF_OBJ_GET which can get a fd of a pinned
bpf obj (prog, map, and link). Take a look at bpf_link__open() in libbpf. Does
it work for the use case that needs to update the link?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-22 23:43 ` Martin KaFai Lau
@ 2024-04-23 17:16 ` Kui-Feng Lee
2024-04-24 0:29 ` Martin KaFai Lau
0 siblings, 1 reply; 13+ messages in thread
From: Kui-Feng Lee @ 2024-04-23 17:16 UTC (permalink / raw)
To: Martin KaFai Lau, Kui-Feng Lee
Cc: andrii, kuifeng, bpf, ast, song, kernel-team
On 4/22/24 16:43, Martin KaFai Lau wrote:
> On 4/22/24 10:30 AM, Kui-Feng Lee wrote:
>>
>>
>> On 4/22/24 10:12, Kui-Feng Lee wrote:
>>>
>>>
>>> On 4/19/24 17:05, Martin KaFai Lau wrote:
>>>> On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
>>>>> +int bpffs_struct_ops_link_open(struct inode *inode, struct file
>>>>> *filp)
>>>>> +{
>>>>> + struct bpf_struct_ops_link *link = inode->i_private;
>>>>> +
>>>>> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
>>>>> + bpf_link_inc(&link->link);
>>>>> + filp->private_data = link;
>>>>> + return 0;
>>>>> +}
>>>>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
>>>>> index af5d2ffadd70..b020d761ab0a 100644
>>>>> --- a/kernel/bpf/inode.c
>>>>> +++ b/kernel/bpf/inode.c
>>>>> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry,
>>>>> umode_t mode, void *arg)
>>>>> static int bpf_mklink(struct dentry *dentry, umode_t mode, void
>>>>> *arg)
>>>>> {
>>>>> + const struct file_operations *fops;
>>>>> struct bpf_link *link = arg;
>>>>> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
>>>>> - bpf_link_is_iter(link) ?
>>>>> - &bpf_iter_fops : &bpffs_obj_fops);
>>>>> + if (bpf_link_is_iter(link))
>>>>> + fops = &bpf_iter_fops;
>>>>> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
>>>>
>>>> Open a pinned link and then update should not be specific to
>>>> struct_ops link. e.g. should be useful to the cgroup link also?
>>>
>>> It could be. Here, I played safe in case it creates any unwanted side
>>> effect for links of unknown types.
>>
>> By the way, may I put it in a follow up patch if we want cgroup links?
>
> This does not feel right. It is not struct_ops specific.
>
> Before we dive in further, there is BPF_OBJ_GET which can get a fd of a
> pinned bpf obj (prog, map, and link). Take a look at bpf_link__open() in
> libbpf. Does it work for the use case that needs to update the link?
>
It should work.
So, this patch is not necessary. However, it is still a nice and
intuitive feature. WDYT?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-23 17:16 ` Kui-Feng Lee
@ 2024-04-24 0:29 ` Martin KaFai Lau
2024-04-25 0:17 ` Andrii Nakryiko
0 siblings, 1 reply; 13+ messages in thread
From: Martin KaFai Lau @ 2024-04-24 0:29 UTC (permalink / raw)
To: Kui-Feng Lee, Kui-Feng Lee; +Cc: andrii, kuifeng, bpf, ast, song, kernel-team
On 4/23/24 10:16 AM, Kui-Feng Lee wrote:
>
>
> On 4/22/24 16:43, Martin KaFai Lau wrote:
>> On 4/22/24 10:30 AM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 4/22/24 10:12, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 4/19/24 17:05, Martin KaFai Lau wrote:
>>>>> On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
>>>>>> +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
>>>>>> +{
>>>>>> + struct bpf_struct_ops_link *link = inode->i_private;
>>>>>> +
>>>>>> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
>>>>>> + bpf_link_inc(&link->link);
>>>>>> + filp->private_data = link;
>>>>>> + return 0;
>>>>>> +}
>>>>>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
>>>>>> index af5d2ffadd70..b020d761ab0a 100644
>>>>>> --- a/kernel/bpf/inode.c
>>>>>> +++ b/kernel/bpf/inode.c
>>>>>> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry, umode_t
>>>>>> mode, void *arg)
>>>>>> static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
>>>>>> {
>>>>>> + const struct file_operations *fops;
>>>>>> struct bpf_link *link = arg;
>>>>>> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
>>>>>> - bpf_link_is_iter(link) ?
>>>>>> - &bpf_iter_fops : &bpffs_obj_fops);
>>>>>> + if (bpf_link_is_iter(link))
>>>>>> + fops = &bpf_iter_fops;
>>>>>> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
>>>>>
>>>>> Open a pinned link and then update should not be specific to struct_ops
>>>>> link. e.g. should be useful to the cgroup link also?
>>>>
>>>> It could be. Here, I played safe in case it creates any unwanted side
>>>> effect for links of unknown types.
>>>
>>> By the way, may I put it in a follow up patch if we want cgroup links?
>>
>> This does not feel right. It is not struct_ops specific.
>>
>> Before we dive in further, there is BPF_OBJ_GET which can get a fd of a pinned
>> bpf obj (prog, map, and link). Take a look at bpf_link__open() in libbpf. Does
>> it work for the use case that needs to update the link?
>>
> It should work.
> So, this patch is not necessary. However, it is still a nice and
> intuitive feature. WDYT?
There is already BPF_OBJ_GET which works for all major bpf obj types (prog, map,
and link). Having open only works for link and only works for one link type
(struct_ops) is not very convincing.
Beside, I am not sure how the file flags (e.g. rdonly...etc) should be handled.
I don't know enough in this area, so I will defer to others to comment in
general the usefulness and the approach.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-24 0:29 ` Martin KaFai Lau
@ 2024-04-25 0:17 ` Andrii Nakryiko
2024-04-25 17:11 ` Kui-Feng Lee
0 siblings, 1 reply; 13+ messages in thread
From: Andrii Nakryiko @ 2024-04-25 0:17 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Kui-Feng Lee, Kui-Feng Lee, andrii, kuifeng, bpf, ast, song, kernel-team
On Tue, Apr 23, 2024 at 5:29 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 4/23/24 10:16 AM, Kui-Feng Lee wrote:
> >
> >
> > On 4/22/24 16:43, Martin KaFai Lau wrote:
> >> On 4/22/24 10:30 AM, Kui-Feng Lee wrote:
> >>>
> >>>
> >>> On 4/22/24 10:12, Kui-Feng Lee wrote:
> >>>>
> >>>>
> >>>> On 4/19/24 17:05, Martin KaFai Lau wrote:
> >>>>> On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
> >>>>>> +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
> >>>>>> +{
> >>>>>> + struct bpf_struct_ops_link *link = inode->i_private;
> >>>>>> +
> >>>>>> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
> >>>>>> + bpf_link_inc(&link->link);
> >>>>>> + filp->private_data = link;
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> >>>>>> index af5d2ffadd70..b020d761ab0a 100644
> >>>>>> --- a/kernel/bpf/inode.c
> >>>>>> +++ b/kernel/bpf/inode.c
> >>>>>> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry, umode_t
> >>>>>> mode, void *arg)
> >>>>>> static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
> >>>>>> {
> >>>>>> + const struct file_operations *fops;
> >>>>>> struct bpf_link *link = arg;
> >>>>>> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
> >>>>>> - bpf_link_is_iter(link) ?
> >>>>>> - &bpf_iter_fops : &bpffs_obj_fops);
> >>>>>> + if (bpf_link_is_iter(link))
> >>>>>> + fops = &bpf_iter_fops;
> >>>>>> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
> >>>>>
> >>>>> Open a pinned link and then update should not be specific to struct_ops
> >>>>> link. e.g. should be useful to the cgroup link also?
> >>>>
> >>>> It could be. Here, I played safe in case it creates any unwanted side
> >>>> effect for links of unknown types.
> >>>
> >>> By the way, may I put it in a follow up patch if we want cgroup links?
> >>
> >> This does not feel right. It is not struct_ops specific.
> >>
> >> Before we dive in further, there is BPF_OBJ_GET which can get a fd of a pinned
> >> bpf obj (prog, map, and link). Take a look at bpf_link__open() in libbpf. Does
> >> it work for the use case that needs to update the link?
> >>
> > It should work.
> > So, this patch is not necessary. However, it is still a nice and
> > intuitive feature. WDYT?
>
> There is already BPF_OBJ_GET which works for all major bpf obj types (prog, map,
> and link). Having open only works for link and only works for one link type
> (struct_ops) is not very convincing.
>
> Beside, I am not sure how the file flags (e.g. rdonly...etc) should be handled.
> I don't know enough in this area, so I will defer to others to comment in
> general the usefulness and the approach.
>
>
Didn't see this discussion before replying on the other patch. But I
agree with Martin, we already use the BPF_OBJ_GET method, and we don't
support directly open()'ing progs/maps, so I don't think we should do
this for links either.
pw-bot: cr
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link.
2024-04-25 0:17 ` Andrii Nakryiko
@ 2024-04-25 17:11 ` Kui-Feng Lee
0 siblings, 0 replies; 13+ messages in thread
From: Kui-Feng Lee @ 2024-04-25 17:11 UTC (permalink / raw)
To: Andrii Nakryiko, Martin KaFai Lau
Cc: Kui-Feng Lee, andrii, kuifeng, bpf, ast, song, kernel-team
On 4/24/24 17:17, Andrii Nakryiko wrote:
> On Tue, Apr 23, 2024 at 5:29 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> On 4/23/24 10:16 AM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 4/22/24 16:43, Martin KaFai Lau wrote:
>>>> On 4/22/24 10:30 AM, Kui-Feng Lee wrote:
>>>>>
>>>>>
>>>>> On 4/22/24 10:12, Kui-Feng Lee wrote:
>>>>>>
>>>>>>
>>>>>> On 4/19/24 17:05, Martin KaFai Lau wrote:
>>>>>>> On 4/16/24 5:25 PM, Kui-Feng Lee wrote:
>>>>>>>> +int bpffs_struct_ops_link_open(struct inode *inode, struct file *filp)
>>>>>>>> +{
>>>>>>>> + struct bpf_struct_ops_link *link = inode->i_private;
>>>>>>>> +
>>>>>>>> + /* Paired with bpf_link_put_direct() in bpf_link_release(). */
>>>>>>>> + bpf_link_inc(&link->link);
>>>>>>>> + filp->private_data = link;
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
>>>>>>>> index af5d2ffadd70..b020d761ab0a 100644
>>>>>>>> --- a/kernel/bpf/inode.c
>>>>>>>> +++ b/kernel/bpf/inode.c
>>>>>>>> @@ -360,11 +360,16 @@ static int bpf_mkmap(struct dentry *dentry, umode_t
>>>>>>>> mode, void *arg)
>>>>>>>> static int bpf_mklink(struct dentry *dentry, umode_t mode, void *arg)
>>>>>>>> {
>>>>>>>> + const struct file_operations *fops;
>>>>>>>> struct bpf_link *link = arg;
>>>>>>>> - return bpf_mkobj_ops(dentry, mode, arg, &bpf_link_iops,
>>>>>>>> - bpf_link_is_iter(link) ?
>>>>>>>> - &bpf_iter_fops : &bpffs_obj_fops);
>>>>>>>> + if (bpf_link_is_iter(link))
>>>>>>>> + fops = &bpf_iter_fops;
>>>>>>>> + else if (link->type == BPF_LINK_TYPE_STRUCT_OPS)
>>>>>>>
>>>>>>> Open a pinned link and then update should not be specific to struct_ops
>>>>>>> link. e.g. should be useful to the cgroup link also?
>>>>>>
>>>>>> It could be. Here, I played safe in case it creates any unwanted side
>>>>>> effect for links of unknown types.
>>>>>
>>>>> By the way, may I put it in a follow up patch if we want cgroup links?
>>>>
>>>> This does not feel right. It is not struct_ops specific.
>>>>
>>>> Before we dive in further, there is BPF_OBJ_GET which can get a fd of a pinned
>>>> bpf obj (prog, map, and link). Take a look at bpf_link__open() in libbpf. Does
>>>> it work for the use case that needs to update the link?
>>>>
>>> It should work.
>>> So, this patch is not necessary. However, it is still a nice and
>>> intuitive feature. WDYT?
>>
>> There is already BPF_OBJ_GET which works for all major bpf obj types (prog, map,
>> and link). Having open only works for link and only works for one link type
>> (struct_ops) is not very convincing.
>>
>> Beside, I am not sure how the file flags (e.g. rdonly...etc) should be handled.
>> I don't know enough in this area, so I will defer to others to comment in
>> general the usefulness and the approach.
>>
>>
>
> Didn't see this discussion before replying on the other patch. But I
> agree with Martin, we already use the BPF_OBJ_GET method, and we don't
> support directly open()'ing progs/maps, so I don't think we should do
> this for links either.
>
> pw-bot: cr
Understand! Let's drop this patch.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-04-25 17:11 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-17 0:25 [PATCH bpf-next 0/2] Update a struct_ops link through a pinned path Kui-Feng Lee
2024-04-17 0:25 ` [PATCH bpf-next 1/2] bpf: enable the "open" operator on a pinned path of a struct_osp link Kui-Feng Lee
2024-04-17 23:19 ` kernel test robot
2024-04-18 6:13 ` kernel test robot
2024-04-20 0:05 ` Martin KaFai Lau
2024-04-22 17:12 ` Kui-Feng Lee
2024-04-22 17:30 ` Kui-Feng Lee
2024-04-22 23:43 ` Martin KaFai Lau
2024-04-23 17:16 ` Kui-Feng Lee
2024-04-24 0:29 ` Martin KaFai Lau
2024-04-25 0:17 ` Andrii Nakryiko
2024-04-25 17:11 ` Kui-Feng Lee
2024-04-17 0:25 ` [PATCH bpf-next 2/2] selftests/bpf: open a pinned path of a struct_ops link Kui-Feng Lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).