| Message ID | 3774367.1583430213@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] Mark AT_* path flags as deprecated and add missing RESOLVE_ flags | expand |
Hi David, > Do we want to do this? Or should we duplicate the RESOLVE_* flags to AT_* > flags so that existing *at() syscalls can make use of them? > > David > --- > commit 448731bf3b29f2b1f7c969d7efe1f0673ae13b5e > Author: David Howells <dhowells@redhat.com> > Date: Thu Mar 5 17:40:02 2020 +0000 > > Mark AT_* flags as deprecated and add missing RESOLVE_ flags > > It has been suggested that new path-using system calls should use RESOLVE_* > flags instead of AT_* flags, but the RESOLVE_* flag functions are not a > superset of the AT_* flag functions. So formalise this by: > > (1) In linux/fcntl.h, add a comment noting that the AT_* flags are > deprecated for new system calls and that RESOLVE_* flags should be > used instead. > > (2) Add some missing flags: > > RESOLVE_NO_TERMINAL_SYMLINKS for AT_SYMLINK_NOFOLLOW > RESOLVE_NO_TERMINAL_AUTOMOUNTS for AT_NO_AUTOMOUNT > RESOLVE_EMPTY_PATH for AT_EMPTY_PATH For me "TERMINAL" sounds strange here (I'm not a native speaker, so feel free to ignore me...). I'd use "BASENAME" instead. > (3) Make openat2() support RESOLVE_NO_TERMINAL_SYMLINKS. LOOKUP_OPEN > internally implies LOOKUP_AUTOMOUNT, and AT_EMPTY_PATH is probably not > worth supporting (maybe use dup2() instead?). > > Reported-by: Stefan Metzmacher <metze@samba.org> > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Aleksa Sarai <cyphar@cyphar.com> > > diff --git a/fs/open.c b/fs/open.c > index 0788b3715731..6946ad09b42b 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -977,7 +977,7 @@ inline struct open_how build_open_how(int flags, umode_t mode) > inline int build_open_flags(const struct open_how *how, struct open_flags *op) > { > int flags = how->flags; > - int lookup_flags = 0; > + int lookup_flags = LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT; > int acc_mode = ACC_MODE(flags); > > /* Must never be set by userspace */ > @@ -1055,8 +1055,8 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op) > > if (flags & O_DIRECTORY) > lookup_flags |= LOOKUP_DIRECTORY; > - if (!(flags & O_NOFOLLOW)) > - lookup_flags |= LOOKUP_FOLLOW; > + if (flags & O_NOFOLLOW) > + lookup_flags &= ~LOOKUP_FOLLOW; > > if (how->resolve & RESOLVE_NO_XDEV) > lookup_flags |= LOOKUP_NO_XDEV; > @@ -1068,6 +1068,8 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op) > lookup_flags |= LOOKUP_BENEATH; > if (how->resolve & RESOLVE_IN_ROOT) > lookup_flags |= LOOKUP_IN_ROOT; > + if (how->resolve & RESOLVE_NO_TERMINAL_SYMLINKS) > + lookup_flags &= ~LOOKUP_FOLLOW; Where's the RESOLVE_NO_TERMINAL_AUTOMOUNTS check? metze
Stefan Metzmacher <metze@samba.org> wrote:
> Where's the RESOLVE_NO_TERMINAL_AUTOMOUNTS check?
See:
(3) Make openat2() support RESOLVE_NO_TERMINAL_SYMLINKS. LOOKUP_OPEN
internally implies LOOKUP_AUTOMOUNT, and AT_EMPTY_PATH is probably not
worth supporting (maybe use dup2() instead?).
As things currently stand, automount following is explicitly forced on for
open and create. We can make it error out instead if this is desirable if
RESOLVE_NO_TERMINAL_AUTOMOUNTS in such a case.
David
On Thu, Mar 05, 2020 at 05:43:33PM +0000, David Howells wrote: > Do we want to do this? Or should we duplicate the RESOLVE_* flags to AT_* > flags so that existing *at() syscalls can make use of them? > > David > --- > commit 448731bf3b29f2b1f7c969d7efe1f0673ae13b5e > Author: David Howells <dhowells@redhat.com> > Date: Thu Mar 5 17:40:02 2020 +0000 > > Mark AT_* flags as deprecated and add missing RESOLVE_ flags > > It has been suggested that new path-using system calls should use RESOLVE_* > flags instead of AT_* flags, but the RESOLVE_* flag functions are not a > superset of the AT_* flag functions. So formalise this by: > > (1) In linux/fcntl.h, add a comment noting that the AT_* flags are > deprecated for new system calls and that RESOLVE_* flags should be > used instead. > > (2) Add some missing flags: > > RESOLVE_NO_TERMINAL_SYMLINKS for AT_SYMLINK_NOFOLLOW > RESOLVE_NO_TERMINAL_AUTOMOUNTS for AT_NO_AUTOMOUNT > RESOLVE_EMPTY_PATH for AT_EMPTY_PATH > > (3) Make openat2() support RESOLVE_NO_TERMINAL_SYMLINKS. LOOKUP_OPEN > internally implies LOOKUP_AUTOMOUNT, and AT_EMPTY_PATH is probably not > worth supporting (maybe use dup2() instead?). > > Reported-by: Stefan Metzmacher <metze@samba.org> > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Aleksa Sarai <cyphar@cyphar.com> > > diff --git a/fs/open.c b/fs/open.c > index 0788b3715731..6946ad09b42b 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -977,7 +977,7 @@ inline struct open_how build_open_how(int flags, umode_t mode) > inline int build_open_flags(const struct open_how *how, struct open_flags *op) > { > int flags = how->flags; > - int lookup_flags = 0; > + int lookup_flags = LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT; > int acc_mode = ACC_MODE(flags); > > /* Must never be set by userspace */ > @@ -1055,8 +1055,8 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op) > > if (flags & O_DIRECTORY) > lookup_flags |= LOOKUP_DIRECTORY; > - if (!(flags & O_NOFOLLOW)) > - lookup_flags |= LOOKUP_FOLLOW; > + if (flags & O_NOFOLLOW) > + lookup_flags &= ~LOOKUP_FOLLOW; Odd change. But I guess you're doing it for the sake of consistency because of how you treat NO_TERMINAL_SYMLINKS below. > > if (how->resolve & RESOLVE_NO_XDEV) > lookup_flags |= LOOKUP_NO_XDEV; > @@ -1068,6 +1068,8 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op) > lookup_flags |= LOOKUP_BENEATH; > if (how->resolve & RESOLVE_IN_ROOT) > lookup_flags |= LOOKUP_IN_ROOT; > + if (how->resolve & RESOLVE_NO_TERMINAL_SYMLINKS) > + lookup_flags &= ~LOOKUP_FOLLOW; > > op->lookup_flags = lookup_flags; > return 0; > diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h > index 7bcdcf4f6ab2..fd6ee34cce0f 100644 > --- a/include/linux/fcntl.h > +++ b/include/linux/fcntl.h > @@ -19,7 +19,8 @@ > /* List of all valid flags for the how->resolve argument: */ > #define VALID_RESOLVE_FLAGS \ > (RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \ > - RESOLVE_BENEATH | RESOLVE_IN_ROOT) > + RESOLVE_BENEATH | RESOLVE_IN_ROOT | RESOLVE_NO_TERMINAL_SYMLINKS | \ > + RESOLVE_NO_TERMINAL_AUTOMOUNTS | RESOLVE_EMPTY_PATH) > > /* List of all open_how "versions". */ > #define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */ > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h > index ca88b7bce553..9f5432dbd213 100644 > --- a/include/uapi/linux/fcntl.h > +++ b/include/uapi/linux/fcntl.h > @@ -84,9 +84,20 @@ > #define DN_ATTRIB 0x00000020 /* File changed attibutes */ > #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */ > > -#define AT_FDCWD -100 /* Special value used to indicate > - openat should use the current > - working directory. */ > + > +/* > + * Special dfd/dirfd file descriptor value used to indicate that *at() system > + * calls should use the current working directory for relative paths. > + */ > +#define AT_FDCWD -100 > + > +/* > + * Pathwalk control flags, used for the *at() syscalls. These should be > + * considered deprecated and should not be used for new system calls. The > + * RESOLVE_* flags in <linux/openat2.h> should be used instead. Agreed.
On Sat, Mar 07, 2020 at 12:41:16AM +1100, Aleksa Sarai wrote: > On 2020-03-05, David Howells <dhowells@redhat.com> wrote: > > Do we want to do this? Or should we duplicate the RESOLVE_* flags to AT_* > > flags so that existing *at() syscalls can make use of them? > > > > David > > --- > > commit 448731bf3b29f2b1f7c969d7efe1f0673ae13b5e > > Author: David Howells <dhowells@redhat.com> > > Date: Thu Mar 5 17:40:02 2020 +0000 > > > > Mark AT_* flags as deprecated and add missing RESOLVE_ flags > > > > It has been suggested that new path-using system calls should use RESOLVE_* > > flags instead of AT_* flags, but the RESOLVE_* flag functions are not a > > superset of the AT_* flag functions. So formalise this by: > > > > (1) In linux/fcntl.h, add a comment noting that the AT_* flags are > > deprecated for new system calls and that RESOLVE_* flags should be > > used instead. > > I wouldn't say it that way -- the RESOLVE_* flags should be used by > syscalls *where it makes sense to change the path resolution rules*. If > it makes more sense for them to have their own flag set, they should > arguably make a separate one (like renameat2 did -- though renameat2 can > never take AT_EMPTY_PATH because it isn't sufficiently extensible). Yeah, we should clearly state that they are not a replacement for _all_ the AT_* flags. I think it makes sense to think of RESOLVE_* flags as a superset of the path-resolution portions of AT_* flags. Maybe in openat2.h: /* * Flags available to syscalls wanting to modify how paths are resolved. * RESOLVE_* flags are intended to as a superset of those AT_* flags * concerned with path resolution. All syscalls modifying their path * resolution behavior are expected to use RESOLVE_* flags. */ Something like this (Native speaker can probably do this way nicer.)?
Christian Brauner <christian.brauner@ubuntu.com> wrote: > > + if (flags & O_NOFOLLOW) > > + lookup_flags &= ~LOOKUP_FOLLOW; > > Odd change. But I guess you're doing it for the sake of consistency > because of how you treat NO_TERMINAL_SYMLINKS below. Not really. The default is to follow. Both remove the LOOKUP_FOLLOW flag and neither set it. David
Aleksa Sarai <cyphar@cyphar.com> wrote: > But please (for now) also reserve RESOLVE_NO_AUTOMOUNTS, which would > apply the same restriction for all path components. I'm not going to do that for the moment. There will be objections if it isn't wired up for at least something - but at the moment Al doesn't want people going and making conflicting changes in fs/namei.c with what he's doing. David
diff --git a/fs/open.c b/fs/open.c index 0788b3715731..6946ad09b42b 100644 --- a/fs/open.c +++ b/fs/open.c @@ -977,7 +977,7 @@ inline struct open_how build_open_how(int flags, umode_t mode) inline int build_open_flags(const struct open_how *how, struct open_flags *op) { int flags = how->flags; - int lookup_flags = 0; + int lookup_flags = LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT; int acc_mode = ACC_MODE(flags); /* Must never be set by userspace */ @@ -1055,8 +1055,8 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op) if (flags & O_DIRECTORY) lookup_flags |= LOOKUP_DIRECTORY; - if (!(flags & O_NOFOLLOW)) - lookup_flags |= LOOKUP_FOLLOW; + if (flags & O_NOFOLLOW) + lookup_flags &= ~LOOKUP_FOLLOW; if (how->resolve & RESOLVE_NO_XDEV) lookup_flags |= LOOKUP_NO_XDEV; @@ -1068,6 +1068,8 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op) lookup_flags |= LOOKUP_BENEATH; if (how->resolve & RESOLVE_IN_ROOT) lookup_flags |= LOOKUP_IN_ROOT; + if (how->resolve & RESOLVE_NO_TERMINAL_SYMLINKS) + lookup_flags &= ~LOOKUP_FOLLOW; op->lookup_flags = lookup_flags; return 0; diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h index 7bcdcf4f6ab2..fd6ee34cce0f 100644 --- a/include/linux/fcntl.h +++ b/include/linux/fcntl.h @@ -19,7 +19,8 @@ /* List of all valid flags for the how->resolve argument: */ #define VALID_RESOLVE_FLAGS \ (RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \ - RESOLVE_BENEATH | RESOLVE_IN_ROOT) + RESOLVE_BENEATH | RESOLVE_IN_ROOT | RESOLVE_NO_TERMINAL_SYMLINKS | \ + RESOLVE_NO_TERMINAL_AUTOMOUNTS | RESOLVE_EMPTY_PATH) /* List of all open_how "versions". */ #define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */ diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index ca88b7bce553..9f5432dbd213 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -84,9 +84,20 @@ #define DN_ATTRIB 0x00000020 /* File changed attibutes */ #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */ -#define AT_FDCWD -100 /* Special value used to indicate - openat should use the current - working directory. */ + +/* + * Special dfd/dirfd file descriptor value used to indicate that *at() system + * calls should use the current working directory for relative paths. + */ +#define AT_FDCWD -100 + +/* + * Pathwalk control flags, used for the *at() syscalls. These should be + * considered deprecated and should not be used for new system calls. The + * RESOLVE_* flags in <linux/openat2.h> should be used instead. + * + * There are also system call-specific flags here (REMOVEDIR, STATX, RECURSIVE). + */ #define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */ #define AT_REMOVEDIR 0x200 /* Remove directory instead of unlinking file. */ diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h index 58b1eb711360..bb8d0a7f82c2 100644 --- a/include/uapi/linux/openat2.h +++ b/include/uapi/linux/openat2.h @@ -22,7 +22,10 @@ struct open_how { __u64 resolve; }; -/* how->resolve flags for openat2(2). */ +/* + * Path resolution paths to replace AT_* paths in all new syscalls that would + * use them. + */ #define RESOLVE_NO_XDEV 0x01 /* Block mount-point crossings (includes bind-mounts). */ #define RESOLVE_NO_MAGICLINKS 0x02 /* Block traversal through procfs-style @@ -35,5 +38,8 @@ struct open_how { #define RESOLVE_IN_ROOT 0x10 /* Make all jumps to "/" and ".." be scoped inside the dirfd (similar to chroot(2)). */ +#define RESOLVE_NO_TERMINAL_SYMLINKS 0x20 /* Don't follow terminal symlinks in the path */ +#define RESOLVE_NO_TERMINAL_AUTOMOUNTS 0x40 /* Don't follow terminal automounts in the path */ +#define RESOLVE_EMPTY_PATH 0x80 /* Permit a path of "" to indicate the dfd exactly */ #endif /* _UAPI_LINUX_OPENAT2_H */
Do we want to do this? Or should we duplicate the RESOLVE_* flags to AT_* flags so that existing *at() syscalls can make use of them? David --- commit 448731bf3b29f2b1f7c969d7efe1f0673ae13b5e Author: David Howells <dhowells@redhat.com> Date: Thu Mar 5 17:40:02 2020 +0000 Mark AT_* flags as deprecated and add missing RESOLVE_ flags It has been suggested that new path-using system calls should use RESOLVE_* flags instead of AT_* flags, but the RESOLVE_* flag functions are not a superset of the AT_* flag functions. So formalise this by: (1) In linux/fcntl.h, add a comment noting that the AT_* flags are deprecated for new system calls and that RESOLVE_* flags should be used instead. (2) Add some missing flags: RESOLVE_NO_TERMINAL_SYMLINKS for AT_SYMLINK_NOFOLLOW RESOLVE_NO_TERMINAL_AUTOMOUNTS for AT_NO_AUTOMOUNT RESOLVE_EMPTY_PATH for AT_EMPTY_PATH (3) Make openat2() support RESOLVE_NO_TERMINAL_SYMLINKS. LOOKUP_OPEN internally implies LOOKUP_AUTOMOUNT, and AT_EMPTY_PATH is probably not worth supporting (maybe use dup2() instead?). Reported-by: Stefan Metzmacher <metze@samba.org> Signed-off-by: David Howells <dhowells@redhat.com> cc: Aleksa Sarai <cyphar@cyphar.com>