| Message ID | 20231207185443.2297160-2-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | BPF token support in libbpf's BPF object | expand |
On Thu, Dec 07, 2023 at 10:54:36AM -0800, Andrii Nakryiko wrote: > It's quite confusing in practice when it's possible to successfully > create a BPF token from BPF FS that didn't have any of delegate_xxx > mount options set up. While it's not wrong, it's actually more > meaningful to reject BPF_TOKEN_CREATE with specific error code (-ENOENT) > to let user-space know that no token delegation is setup up. > > So, instead of creating empty BPF token that will be always ignored > because it doesn't have any of the allow_xxx bits set, reject it with > -ENOENT. If we ever need empty BPF token to be possible, we can support > that with extra flag passed into BPF_TOKEN_CREATE. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- Might consider EOPNOTSUPP (or whatever the correct way of spelling this is). Otherwise, Acked-by: Christian Brauner <brauner@kernel.org>
On Fri, Dec 8, 2023 at 1:49 PM Christian Brauner <brauner@kernel.org> wrote: > > On Thu, Dec 07, 2023 at 10:54:36AM -0800, Andrii Nakryiko wrote: > > It's quite confusing in practice when it's possible to successfully > > create a BPF token from BPF FS that didn't have any of delegate_xxx > > mount options set up. While it's not wrong, it's actually more > > meaningful to reject BPF_TOKEN_CREATE with specific error code (-ENOENT) > > to let user-space know that no token delegation is setup up. > > > > So, instead of creating empty BPF token that will be always ignored > > because it doesn't have any of the allow_xxx bits set, reject it with > > -ENOENT. If we ever need empty BPF token to be possible, we can support > > that with extra flag passed into BPF_TOKEN_CREATE. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > Might consider EOPNOTSUPP (or whatever the correct way of spelling this I thought about that, but it felt wrong to return "unsupported" error code, because BPF token is supported, it's just not setup/delegated on that particular instance of BPF FS. So in that sense it felt like "BPF token is not there" is more appropriate, which is why I went with -ENOENT. And also I was worried that we might add -EOPNOTSUPP for some unsupported combinations of flags or something, and then it will become confusing to detect when some functionality is really not supported, or if BPF token delegation isn't set on BPF FS. I hope that makes sense and is not too contrived an argument. > is). Otherwise, > Acked-by: Christian Brauner <brauner@kernel.org>
Andrii Nakryiko wrote: > It's quite confusing in practice when it's possible to successfully > create a BPF token from BPF FS that didn't have any of delegate_xxx > mount options set up. While it's not wrong, it's actually more > meaningful to reject BPF_TOKEN_CREATE with specific error code (-ENOENT) > to let user-space know that no token delegation is setup up. > > So, instead of creating empty BPF token that will be always ignored > because it doesn't have any of the allow_xxx bits set, reject it with > -ENOENT. If we ever need empty BPF token to be possible, we can support > that with extra flag passed into BPF_TOKEN_CREATE. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > kernel/bpf/token.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c > index 17212efcde60..a86fccd57e2d 100644 > --- a/kernel/bpf/token.c > +++ b/kernel/bpf/token.c > @@ -152,6 +152,15 @@ int bpf_token_create(union bpf_attr *attr) > goto out_path; > } > > + mnt_opts = path.dentry->d_sb->s_fs_info; > + if (mnt_opts->delegate_cmds == 0 && > + mnt_opts->delegate_maps == 0 && > + mnt_opts->delegate_progs == 0 && > + mnt_opts->delegate_attachs == 0) { > + err = -ENOENT; /* no BPF token delegation is set up */ > + goto out_path; > + } > + > mode = S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask()); > inode = bpf_get_inode(path.mnt->mnt_sb, NULL, mode); > if (IS_ERR(inode)) { > @@ -181,7 +190,6 @@ int bpf_token_create(union bpf_attr *attr) > /* remember bpffs owning userns for future ns_capable() checks */ > token->userns = get_user_ns(userns); > > - mnt_opts = path.dentry->d_sb->s_fs_info; > token->allowed_cmds = mnt_opts->delegate_cmds; > token->allowed_maps = mnt_opts->delegate_maps; > token->allowed_progs = mnt_opts->delegate_progs; > -- > 2.34.1 > > Acked-by: John Fastabend <john.fastabend@gmail.com>
diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c index 17212efcde60..a86fccd57e2d 100644 --- a/kernel/bpf/token.c +++ b/kernel/bpf/token.c @@ -152,6 +152,15 @@ int bpf_token_create(union bpf_attr *attr) goto out_path; } + mnt_opts = path.dentry->d_sb->s_fs_info; + if (mnt_opts->delegate_cmds == 0 && + mnt_opts->delegate_maps == 0 && + mnt_opts->delegate_progs == 0 && + mnt_opts->delegate_attachs == 0) { + err = -ENOENT; /* no BPF token delegation is set up */ + goto out_path; + } + mode = S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask()); inode = bpf_get_inode(path.mnt->mnt_sb, NULL, mode); if (IS_ERR(inode)) { @@ -181,7 +190,6 @@ int bpf_token_create(union bpf_attr *attr) /* remember bpffs owning userns for future ns_capable() checks */ token->userns = get_user_ns(userns); - mnt_opts = path.dentry->d_sb->s_fs_info; token->allowed_cmds = mnt_opts->delegate_cmds; token->allowed_maps = mnt_opts->delegate_maps; token->allowed_progs = mnt_opts->delegate_progs;
It's quite confusing in practice when it's possible to successfully create a BPF token from BPF FS that didn't have any of delegate_xxx mount options set up. While it's not wrong, it's actually more meaningful to reject BPF_TOKEN_CREATE with specific error code (-ENOENT) to let user-space know that no token delegation is setup up. So, instead of creating empty BPF token that will be always ignored because it doesn't have any of the allow_xxx bits set, reject it with -ENOENT. If we ever need empty BPF token to be possible, we can support that with extra flag passed into BPF_TOKEN_CREATE. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- kernel/bpf/token.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)