On Wed, May 17, 2023 at 2:11 AM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > On Tue, May 16, 2023 at 11:02:42AM -0700, Andrii Nakryiko wrote: > > On Tue, May 16, 2023 at 2:07 AM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > > > On Mon, May 15, 2023 at 05:13:46PM -0700, Andrii Nakryiko wrote: > > > > Current UAPI of BPF_OBJ_PIN and BPF_OBJ_GET commands of bpf() syscall > > > > forces users to specify pinning location as a string-based absolute or > > > > relative (to current working directory) path. This has various > > > > implications related to security (e.g., symlink-based attacks), forces > > > > BPF FS to be exposed in the file system, which can cause races with > > > > other applications. > > > > > > > > One of the feedbacks we got from folks working with containers heavily > > > > was that inability to use purely FD-based location specification was an > > > > unfortunate limitation and hindrance for BPF_OBJ_PIN and BPF_OBJ_GET > > > > commands. This patch closes this oversight, adding path_fd field to > > > > > > Cool! > > > > > > > BPF_OBJ_PIN and BPF_OBJ_GET UAPI, following conventions established by > > > > *at() syscalls for dirfd + pathname combinations. > > > > > > > > This now allows interesting possibilities like working with detached BPF > > > > FS mount (e.g., to perform multiple pinnings without running a risk of > > > > someone interfering with them), and generally making pinning/getting > > > > more secure and not prone to any races and/or security attacks. > > > > > > > > This is demonstrated by a selftest added in subsequent patch that takes > > > > advantage of new mount APIs (fsopen, fsconfig, fsmount) to demonstrate > > > > creating detached BPF FS mount, pinning, and then getting BPF map out of > > > > it, all while never exposing this private instance of BPF FS to outside > > > > worlds. > > > > > > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > > > > --- > > > > include/linux/bpf.h | 4 ++-- > > > > include/uapi/linux/bpf.h | 5 +++++ > > > > kernel/bpf/inode.c | 16 ++++++++-------- > > > > kernel/bpf/syscall.c | 8 +++++--- > > > > tools/include/uapi/linux/bpf.h | 5 +++++ > > > > 5 files changed, 25 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > > index 36e4b2d8cca2..f58895830ada 100644 > > > > --- a/include/linux/bpf.h > > > > +++ b/include/linux/bpf.h > > > > @@ -2077,8 +2077,8 @@ struct file *bpf_link_new_file(struct bpf_link *link, int *reserved_fd); > > > > struct bpf_link *bpf_link_get_from_fd(u32 ufd); > > > > struct bpf_link *bpf_link_get_curr_or_next(u32 *id); > > > > > > > > -int bpf_obj_pin_user(u32 ufd, const char __user *pathname); > > > > -int bpf_obj_get_user(const char __user *pathname, int flags); > > > > +int bpf_obj_pin_user(u32 ufd, int path_fd, const char __user *pathname); > > > > +int bpf_obj_get_user(int path_fd, const char __user *pathname, int flags); > > > > > > > > #define BPF_ITER_FUNC_PREFIX "bpf_iter_" > > > > #define DEFINE_BPF_ITER_FUNC(target, args...) \ > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > index 1bb11a6ee667..db2870a52ce0 100644 > > > > --- a/include/uapi/linux/bpf.h > > > > +++ b/include/uapi/linux/bpf.h > > > > @@ -1420,6 +1420,11 @@ union bpf_attr { > > > > __aligned_u64 pathname; > > > > __u32 bpf_fd; > > > > __u32 file_flags; > > > > + /* same as dirfd in openat() syscall; see openat(2) > > > > + * manpage for details of dirfd/path_fd and pathname semantics; > > > > + * zero path_fd implies AT_FDCWD behavior > > > > + */ > > > > + __u32 path_fd; > > > > }; > > > > > > So 0 is a valid file descriptor and can trivially be created and made to > > > refer to any file. Is this a conscious decision to have a zero value > > > imply AT_FDCWD and have you done this somewhere else in bpf already? > > > Because that's contrary to how any file descriptor based apis work. > > > > > > How this is usually solved for extensible structs is to have a flag > > > field that raises a flag to indicate that the fd fiel is set and thus 0 > > > can be used as a valid value. > > > > > > The way you're doing it right now is very counterintuitive to userspace > > > and pretty much guaranteed to cause subtle bugs. > > > > Yes, it's a very bpf()-specific convention we've settled on a while > > ago. It allows a cleaner and simpler backwards compatibility story > > without having to introduce new flags every single time. Most of BPF > > UAPI by now dictates that (otherwise valid) FD 0 can't be used to pass > > it to bpf() syscall. Most of the time users will be blissfully unaware > > because libbpf and other BPF libraries are checking for fd == 0 and > > dup()'ing them to avoid ever returning FD 0 to the user. > > > > tl;dr, a conscious decision consistent with the rest of BPF UAPI. It > > is a bpf() peculiarity, yes. > > Adding fsdevel so we're aware of this quirk. > > So I'm not sure whether this was ever discussed on fsdevel when you took > the decision to treat fd 0 as AT_FDCWD or in general treat fd 0 as an > invalid value. > > If it was discussed then great but if not then I would like to make it > very clear that if in the future you decide to introduce custom > semantics for vfs provided infrastructure - especially when exposed to > userspace - that you please Cc us. Yep, I'll remember to cc linux-fsdevel@xxxxxxxxxxxxxxx for future patches touching on vfs-related concepts, no problem. I wasn't trying to sneak it in or anything, it just never occurred to me, sorry about that. > > You often make it very clear on the list that you don't like it when > anything that touches bpf code doesn't end up getting sent to the bpf > mailing list. It is exactly the same for us. That's a fair request, ack. > > This is not a rant I'm really just trying to make sure that we agree on > common ground when it comes to touching each others code or semantic > assumptions. > > I personally find this extremely weird to treat fd 0 as anything other > than a random fd number as it goes against any userspace assumptions and > drastically deviates from basically every file descriptor interface we > have. I mean, you're not just saying fd 0 is invalid you're even saying > it means AT_FDCWD. Agreed, I can see how this could have undesirable (not just surprising) implications if, say, open(O_PATH) returned fd=0. I just sent a v2 with a new flag that needs to be specified to be able to use path FD (and falling back to backwards-compatible AT_FDCWD behavior otherwise). > > For every other interface, including those that pass fds in structs > whose extensibility is premised on unknown fields being set to zero, > have ways to make fd 0 work just fine. You could've done that to without > inventing custom fd semantics.