* Re: [PATCH 07/14] fsinfo: Allow mount information to be queried [ver #18]
From: Miklos Szeredi @ 2020-03-10 9:04 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, Al Viro, Ian Kent, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak,
Jeff Layton, Linux API, linux-fsdevel, LSM, linux-kernel
In-Reply-To: <158376251286.344135.12815432977346939752.stgit@warthog.procyon.org.uk>
On Mon, Mar 9, 2020 at 3:02 PM David Howells <dhowells@redhat.com> wrote:
>
> Allow mount information, including information about the topology tree to
> be queried with the fsinfo() system call. Setting AT_FSINFO_QUERY_MOUNT
> allows overlapping mounts to be queried by indicating that the syscall
> should interpet the pathname as a number indicating the mount ID.
>
> To this end, a number of fsinfo() attributes are provided:
>
> (1) FSINFO_ATTR_MOUNT_INFO.
>
> This is a structure providing information about a mount, including:
>
> - Mounted superblock ID (mount ID uniquifier).
> - Mount ID (can be used with AT_FSINFO_QUERY_MOUNT).
> - Parent mount ID.
> - Mount attributes (eg. R/O, NOEXEC).
> - Mount change/notification counter.
>
> Note that the parent mount ID is overridden to the ID of the queried
> mount if the parent lies outside of the chroot or dfd tree.
>
> (2) FSINFO_ATTR_MOUNT_PATH.
>
> This a string providing information about a bind mount relative the
> the root that was bound off, though it may get overridden by the
> filesystem (NFS unconditionally sets it to "/", for example).
>
> (3) FSINFO_ATTR_MOUNT_POINT.
>
> This is a string indicating the name of the mountpoint within the
> parent mount, limited to the parent's mounted root and the chroot.
>
> (4) FSINFO_ATTR_MOUNT_POINT_FULL.
>
> This is a string indicating the full path of the mountpoint, limited to
> the chroot.
>
> (5) FSINFO_ATTR_MOUNT_CHILDREN.
>
> This produces an array of structures, one for each child and capped
> with one for the argument mount (checked after listing all the
> children). Each element contains the mount ID and the change counter
> of the respective mount object.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
> fs/d_path.c | 2
> fs/fsinfo.c | 14 +++
> fs/internal.h | 10 ++
> fs/namespace.c | 177 +++++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/fsinfo.h | 36 +++++++++
> samples/vfs/test-fsinfo.c | 43 ++++++++++
> 6 files changed, 281 insertions(+), 1 deletion(-)
>
> diff --git a/fs/d_path.c b/fs/d_path.c
> index 0f1fc1743302..4c203f64e45e 100644
> --- a/fs/d_path.c
> +++ b/fs/d_path.c
> @@ -229,7 +229,7 @@ static int prepend_unreachable(char **buffer, int *buflen)
> return prepend(buffer, buflen, "(unreachable)", 13);
> }
>
> -static void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
> +void get_fs_root_rcu(struct fs_struct *fs, struct path *root)
> {
> unsigned seq;
>
> diff --git a/fs/fsinfo.c b/fs/fsinfo.c
> index bafeb73feaf4..6d2bc03998e4 100644
> --- a/fs/fsinfo.c
> +++ b/fs/fsinfo.c
> @@ -236,6 +236,14 @@ static int fsinfo_generic_seq_read(struct path *path, struct fsinfo_context *ctx
> ret = sb->s_op->show_options(&m, path->mnt->mnt_root);
> break;
>
> + case FSINFO_ATTR_MOUNT_PATH:
> + if (sb->s_op->show_path) {
> + ret = sb->s_op->show_path(&m, path->mnt->mnt_root);
> + } else {
> + seq_dentry(&m, path->mnt->mnt_root, " \t\n\\");
> + }
> + break;
> +
> case FSINFO_ATTR_FS_STATISTICS:
> if (sb->s_op->show_stats)
> ret = sb->s_op->show_stats(&m, path->mnt->mnt_root);
> @@ -270,6 +278,12 @@ static const struct fsinfo_attribute fsinfo_common_attributes[] = {
>
> FSINFO_LIST (FSINFO_ATTR_FSINFO_ATTRIBUTES, (void *)123UL),
> FSINFO_VSTRUCT_N(FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO, (void *)123UL),
> +
> + FSINFO_VSTRUCT (FSINFO_ATTR_MOUNT_INFO, fsinfo_generic_mount_info),
> + FSINFO_STRING (FSINFO_ATTR_MOUNT_PATH, fsinfo_generic_seq_read),
> + FSINFO_STRING (FSINFO_ATTR_MOUNT_POINT, fsinfo_generic_mount_point),
> + FSINFO_STRING (FSINFO_ATTR_MOUNT_POINT_FULL, fsinfo_generic_mount_point_full),
> + FSINFO_LIST (FSINFO_ATTR_MOUNT_CHILDREN, fsinfo_generic_mount_children),
> {}
> };
>
> diff --git a/fs/internal.h b/fs/internal.h
> index abbd5299e7dc..1a318dc85f2f 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -15,6 +15,7 @@ struct mount;
> struct shrink_control;
> struct fs_context;
> struct user_namespace;
> +struct fsinfo_context;
>
> /*
> * block_dev.c
> @@ -47,6 +48,11 @@ extern int __block_write_begin_int(struct page *page, loff_t pos, unsigned len,
> */
> extern void __init chrdev_init(void);
>
> +/*
> + * d_path.c
> + */
> +extern void get_fs_root_rcu(struct fs_struct *fs, struct path *root);
> +
> /*
> * fs_context.c
> */
> @@ -93,6 +99,10 @@ extern void __mnt_drop_write_file(struct file *);
> extern void dissolve_on_fput(struct vfsmount *);
> extern int lookup_mount_object(struct path *, int, struct path *);
> extern int fsinfo_generic_mount_source(struct path *, struct fsinfo_context *);
> +extern int fsinfo_generic_mount_info(struct path *, struct fsinfo_context *);
> +extern int fsinfo_generic_mount_point(struct path *, struct fsinfo_context *);
> +extern int fsinfo_generic_mount_point_full(struct path *, struct fsinfo_context *);
> +extern int fsinfo_generic_mount_children(struct path *, struct fsinfo_context *);
>
> /*
> * fs_struct.c
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 54e8eb93fdd6..a6cb8c6b004f 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -4149,4 +4149,181 @@ int lookup_mount_object(struct path *root, int mnt_id, struct path *_mntpt)
> goto out_unlock;
> }
>
> +/*
> + * Retrieve information about the nominated mount.
> + */
> +int fsinfo_generic_mount_info(struct path *path, struct fsinfo_context *ctx)
> +{
> + struct fsinfo_mount_info *p = ctx->buffer;
> + struct super_block *sb;
> + struct mount *m;
> + struct path root;
> + unsigned int flags;
> +
> + m = real_mount(path->mnt);
> + sb = m->mnt.mnt_sb;
> +
> + p->sb_unique_id = sb->s_unique_id;
> + p->mnt_unique_id = m->mnt_unique_id;
> + p->mnt_id = m->mnt_id;
> + p->parent_id = m->mnt_parent->mnt_id;
> +
> + get_fs_root(current->fs, &root);
> + if (path->mnt == root.mnt) {
> + p->parent_id = p->mnt_id;
> + } else {
> + rcu_read_lock();
> + if (!are_paths_connected(&root, path))
> + p->parent_id = p->mnt_id;
> + rcu_read_unlock();
> + }
> + if (IS_MNT_SHARED(m))
> + p->group_id = m->mnt_group_id;
> + if (IS_MNT_SLAVE(m)) {
> + int master = m->mnt_master->mnt_group_id;
> + int dom = get_dominating_id(m, &root);
This isn't safe without namespace_sem or mount_lock.
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH 00/14] VFS: Filesystem information [ver #18]
From: Miklos Szeredi @ 2020-03-10 9:18 UTC (permalink / raw)
To: David Howells
Cc: Linus Torvalds, Al Viro, Theodore Ts'o, Stefan Metzmacher,
Andreas Dilger, linux-ext4, Aleksa Sarai, Trond Myklebust,
Anna Schumaker, Linux NFS list, Linux API, Ian Kent,
Miklos Szeredi, Christian Brauner, Jann Horn, Darrick J. Wong,
Karel Zak, Jeff Layton, linux-fsdevel, LSM, linux-kernel
In-Reply-To: <537182.1583794373@warthog.procyon.org.uk>
On Mon, Mar 9, 2020 at 11:53 PM David Howells <dhowells@redhat.com> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > > (1) It can be targetted. It makes it easy to query directly by path or
> > > fd, but can also query by mount ID or fscontext fd. procfs and sysfs
> > > cannot do three of these things easily.
> >
> > See above: with the addition of open(path, O_PATH) it can do all of these.
>
> That's a horrible interface. To query a file by path, you have to do:
>
> fd = open(path, O_PATH);
> sprintf(procpath, "/proc/self/fdmount/%u/<attr>");
> fd2 = open(procpath, O_RDONLY);
> read(fd2, ...);
> close(fd2);
> close(fd);
>
> See point (3) about efficiency also. You're having to open *two* files.
I completely agree, opening two files is surely going to kill
performance of application needing to retrieve a billion mount
attributes per second.</sarcasm>
> > > (2) Easier to provide LSM oversight. Is the accessing process allowed to
> > > query information pertinent to a particular file?
> >
> > Not quite sure why this would be easier for a new ad-hoc interface than for
> > the well established filesystem API.
>
> You're right. That's why fsinfo() uses standard pathwalk where possible,
> e.g.:
>
> fsinfo(AT_FDCWD, "/path/to/file", ...);
>
> or a fairly standard fd-querying interface:
>
> fsinfo(fd, "", { resolve_flags = RESOLVE_EMPTY_PATH }, ...);
>
> to query an open file descriptor. These are well-established filesystem APIs.
Yes. The problem is with the "..." part where you pass random
structures to a function. That's useful sometimes, but at the very
least it breaks type safety, and not what I would call a "clean" API.
> > Now onto the advantages of a filesystem based API:
> >
> > - immediately usable from all programming languages, including scripts
>
> This is not true. You can't open O_PATH from shell scripts, so you can't
> query things by path that you can't or shouldn't open (dev file paths, for
> example; symlinks).
Yes. However, you just wrote the core of a utility that could do this
(in 6 lines, no less). Now try that feat with fsinfo(2)!
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH 02/14] fsinfo: Add fsinfo() syscall to query filesystem information [ver #18]
From: Christian Brauner @ 2020-03-10 9:31 UTC (permalink / raw)
To: David Howells
Cc: torvalds, viro, linux-api, raven, mszeredi, christian, jannh,
darrick.wong, kzak, jlayton, linux-fsdevel, linux-security-module,
linux-kernel
In-Reply-To: <158376246603.344135.4335596732820276494.stgit@warthog.procyon.org.uk>
On Mon, Mar 09, 2020 at 02:01:06PM +0000, David Howells wrote:
> Add a system call to allow filesystem information to be queried. A request
> value can be given to indicate the desired attribute. Support is provided
> for enumerating multi-value attributes.
>
> ===============
> NEW SYSTEM CALL
> ===============
>
> The new system call looks like:
>
> int ret = fsinfo(int dfd,
> const char *pathname,
> const struct fsinfo_params *params,
> size_t params_size,
> void *result_buffer,
> size_t result_buf_size);
>
> The params parameter optionally points to a block of parameters:
>
> struct fsinfo_params {
> __u32 resolve_flags;
> __u32 flags;
> __u32 request;
> __u32 Nth;
> __u32 Mth;
> };
>
> If params is NULL, the default is that params->request is
> FSINFO_ATTR_STATFS and all the other fields are 0. params_size indicates
> the size of the parameter struct. If the parameter block is short compared
> to what the kernel expects, the missing length will be set to 0; if the
> parameter block is longer, an error will be given if the excess is not all
> zeros.
>
> The object to be queried is specified as follows - part param->flags
> indicates the type of reference:
>
> (1) FSINFO_FLAGS_QUERY_PATH - dfd, pathname and at_flags indicate a
> filesystem object to query. There is no separate system call
> providing an analogue of lstat() - RESOLVE_NO_TRAILING_SYMLINKS should
> be set in at_flags instead. RESOLVE_NO_TRAILING_AUTOMOUNTS can also
> be used to an allow automount point to be queried without triggering
> it.
>
> (2) FSINFO_FLAGS_QUERY_FD - dfd indicates a file descriptor pointing to
> the filesystem object to query. pathname should be NULL.
>
> (3) FSINFO_FLAGS_QUERY_MOUNT - pathname indicates the numeric ID of the
> mountpoint to query as a string. dfd is used to constrain which
> mounts can be accessed. If dfd is AT_FDCWD, the mount must be within
> the subtree rooted at chroot, otherwise the mount must be within the
> subtree rooted at the directory specified by dfd.
>
> (4) In the future FSINFO_FLAGS_QUERY_FSCONTEXT will be added - dfd will
> indicate a context handle fd obtained from fsopen() or fspick(),
> allowing that to be queried before the target superblock is attached
> to the filesystem or even created.
>
> params->request indicates the attribute/attributes to be queried. This can
> be one of:
>
> FSINFO_ATTR_STATFS - statfs-style info
> FSINFO_ATTR_IDS - Filesystem IDs
> FSINFO_ATTR_LIMITS - Filesystem limits
> FSINFO_ATTR_SUPPORTS - Support for statx, ioctl, etc.
> FSINFO_ATTR_TIMESTAMP_INFO - Inode timestamp info
> FSINFO_ATTR_VOLUME_ID - Volume ID (string)
> FSINFO_ATTR_VOLUME_UUID - Volume UUID
> FSINFO_ATTR_VOLUME_NAME - Volume name (string)
> FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO - Information about attr Nth
> FSINFO_ATTR_FSINFO_ATTRIBUTES - List of supported attrs
>
> Some attributes (such as the servers backing a network filesystem) can have
> multiple values. These can be enumerated by setting params->Nth and
> params->Mth to 0, 1, ... until ENODATA is returned.
>
> result_buffer and result_buf_size point to the reply buffer. The buffer is
> filled up to the specified size, even if this means truncating the reply.
> The size of the full reply is returned, irrespective of the amount data
> that was copied. In future versions, this will allow extra fields to be
> tacked on to the end of the reply, but anyone not expecting them will only
> get the subset they're expecting. If either buffer of result_buf_size are
> 0, no copy will take place and the data size will be returned.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: linux-api@vger.kernel.org
You're missing to wire-up the syscall into the arm64 unistd32.h table
and this is all in one patch. I'd rather do it like we have done for all
other syscalls recently, and split this into:
- actual syscall implementation
- final wiring-up patch
Will make it easier to apply and spot merge conflicts when multiple
syscalls are proposed. I'm going to respond to this mail here with two
patches. One could replace this one I'm responding to and the other one
should probably go on top of the series.
(Please note that the same missing arm64 unistd32.h handling also likely
affects the watch syscalls as I haven't seen them in there when I added
fsinfo().)
^ permalink raw reply
* [PATCH v19 14/14] arch: wire up fsinfo syscall
From: Christian Brauner @ 2020-03-10 9:32 UTC (permalink / raw)
To: christian.brauner
Cc: christian, darrick.wong, dhowells, jannh, jlayton, kzak,
linux-api, linux-fsdevel, linux-kernel, linux-security-module,
mszeredi, raven, torvalds, viro
In-Reply-To: <20200310093241.1143777-1-christian.brauner@ubuntu.com>
This wires up the fsinfo() syscall for all architectures.
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 ++
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
include/linux/syscalls.h | 4 ++++
include/uapi/asm-generic/unistd.h | 4 +++-
20 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 7c0115af9010..4d0b07dde12d 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -479,3 +479,4 @@
548 common pidfd_getfd sys_pidfd_getfd
549 common watch_mount sys_watch_mount
550 common watch_sb sys_watch_sb
+551 common fsinfo sys_fsinfo
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index f256f009a89f..fdda8382b420 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -453,3 +453,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index bc0f923e0e04..388eeb71cff0 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -38,7 +38,7 @@
#define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)
#define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800)
-#define __NR_compat_syscalls 441
+#define __NR_compat_syscalls 442
#endif
#define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index c1c61635f89c..1f7d2c8d481a 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -883,6 +883,8 @@ __SYSCALL(__NR_clone3, sys_clone3)
__SYSCALL(__NR_openat2, sys_openat2)
#define __NR_pidfd_getfd 438
__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
+#define __NR_fsinfo 441
+__SYSCALL(__NR_fsinfo, sys_fsinfo)
/*
* Please add new compat syscalls above this comment and update
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index a4dafc659647..2316e60e031a 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -360,3 +360,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index 893fb4151547..efc2723ca91f 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -439,3 +439,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 54aaf0d40c64..745c0f462fce 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -445,3 +445,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index fd34dd0efed0..499f83562a8c 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -378,3 +378,4 @@
438 n32 pidfd_getfd sys_pidfd_getfd
439 n32 watch_mount sys_watch_mount
440 n32 watch_sb sys_watch_sb
+441 n32 fsinfo sys_fsinfo
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index db0f4c0a0a0b..b3188bc3ab3c 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -354,3 +354,4 @@
438 n64 pidfd_getfd sys_pidfd_getfd
439 n64 watch_mount sys_watch_mount
440 n64 watch_sb sys_watch_sb
+441 n64 fsinfo sys_fsinfo
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index ce2e1326de8f..1a3e8ed5e538 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -427,3 +427,4 @@
438 o32 pidfd_getfd sys_pidfd_getfd
439 o32 watch_mount sys_watch_mount
440 o32 watch_sb sys_watch_sb
+441 o32 fsinfo sys_fsinfo
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 6e4a7c08b64b..2572c215d861 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -437,3 +437,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 08943f3b8206..39d7ac7e918c 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -521,3 +521,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index b3b8529d2b74..ae4cefd3dd1b 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -442,3 +442,4 @@
438 common pidfd_getfd sys_pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo sys_fsinfo
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index 89307a20657c..05945b9aee4b 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -442,3 +442,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 4ff841a00450..b71b34d4b45c 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -485,3 +485,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index e2731d295f88..e118ba9aca4c 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -444,3 +444,4 @@
438 i386 pidfd_getfd sys_pidfd_getfd __ia32_sys_pidfd_getfd
439 i386 watch_mount sys_watch_mount __ia32_sys_watch_mount
440 i386 watch_sb sys_watch_sb __ia32_sys_watch_sb
+441 i386 fsinfo sys_fsinfo __ia32_sys_fsinfo
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index f4391176102c..067f247471d0 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -361,6 +361,7 @@
438 common pidfd_getfd __x64_sys_pidfd_getfd
439 common watch_mount __x64_sys_watch_mount
440 common watch_sb __x64_sys_watch_sb
+441 common fsinfo __x64_sys_fsinfo
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 8e7d731ed6cf..e1ec25099d10 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -410,3 +410,4 @@
438 common pidfd_getfd sys_pidfd_getfd
439 common watch_mount sys_watch_mount
440 common watch_sb sys_watch_sb
+441 common fsinfo sys_fsinfo
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index c84440d57f52..76064c0807e5 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -47,6 +47,7 @@ struct stat64;
struct statfs;
struct statfs64;
struct statx;
+struct fsinfo_params;
struct __sysctl_args;
struct sysinfo;
struct timespec;
@@ -1007,6 +1008,9 @@ asmlinkage long sys_watch_mount(int dfd, const char __user *path,
unsigned int at_flags, int watch_fd, int watch_id);
asmlinkage long sys_watch_sb(int dfd, const char __user *path,
unsigned int at_flags, int watch_fd, int watch_id);
+asmlinkage long sys_fsinfo(int dfd, const char __user *pathname,
+ struct fsinfo_params __user *params, size_t params_size,
+ void __user *result_buffer, size_t result_buf_size);
/*
* Architecture-specific system calls
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 5bff318b7ffa..7d764f86d3f5 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -859,9 +859,11 @@ __SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
__SYSCALL(__NR_watch_mount, sys_watch_mount)
#define __NR_watch_sb 440
__SYSCALL(__NR_watch_sb, sys_watch_sb)
+#define __NR_fsinfo 441
+__SYSCALL(__NR_fsinfo, sys_fsinfo)
#undef __NR_syscalls
-#define __NR_syscalls 441
+#define __NR_syscalls 442
/*
* 32 bit systems traditionally used different
--
2.25.1
^ permalink raw reply related
* [PATCH v19 01/14] fsinfo: Add fsinfo() syscall to query filesystem information
From: Christian Brauner @ 2020-03-10 9:32 UTC (permalink / raw)
To: christian.brauner
Cc: christian, darrick.wong, dhowells, jannh, jlayton, kzak,
linux-api, linux-fsdevel, linux-kernel, linux-security-module,
mszeredi, raven, torvalds, viro
In-Reply-To: <20200310093116.ylq6vaunr6js4eyy@wittgenstein>
From: David Howells <dhowells@redhat.com>
Add a system call to allow filesystem information to be queried. A request
value can be given to indicate the desired attribute. Support is provided
for enumerating multi-value attributes.
===============
NEW SYSTEM CALL
===============
The new system call looks like:
int ret = fsinfo(int dfd,
const char *pathname,
const struct fsinfo_params *params,
size_t params_size,
void *result_buffer,
size_t result_buf_size);
The params parameter optionally points to a block of parameters:
struct fsinfo_params {
__u32 resolve_flags;
__u32 flags;
__u32 request;
__u32 Nth;
__u32 Mth;
};
If params is NULL, the default is that params->request is
FSINFO_ATTR_STATFS and all the other fields are 0. params_size indicates
the size of the parameter struct. If the parameter block is short compared
to what the kernel expects, the missing length will be set to 0; if the
parameter block is longer, an error will be given if the excess is not all
zeros.
The object to be queried is specified as follows - part param->flags
indicates the type of reference:
(1) FSINFO_FLAGS_QUERY_PATH - dfd, pathname and at_flags indicate a
filesystem object to query. There is no separate system call
providing an analogue of lstat() - RESOLVE_NO_TRAILING_SYMLINKS should
be set in at_flags instead. RESOLVE_NO_TRAILING_AUTOMOUNTS can also
be used to an allow automount point to be queried without triggering
it.
(2) FSINFO_FLAGS_QUERY_FD - dfd indicates a file descriptor pointing to
the filesystem object to query. pathname should be NULL.
(3) FSINFO_FLAGS_QUERY_MOUNT - pathname indicates the numeric ID of the
mountpoint to query as a string. dfd is used to constrain which
mounts can be accessed. If dfd is AT_FDCWD, the mount must be within
the subtree rooted at chroot, otherwise the mount must be within the
subtree rooted at the directory specified by dfd.
(4) In the future FSINFO_FLAGS_QUERY_FSCONTEXT will be added - dfd will
indicate a context handle fd obtained from fsopen() or fspick(),
allowing that to be queried before the target superblock is attached
to the filesystem or even created.
params->request indicates the attribute/attributes to be queried. This can
be one of:
FSINFO_ATTR_STATFS - statfs-style info
FSINFO_ATTR_IDS - Filesystem IDs
FSINFO_ATTR_LIMITS - Filesystem limits
FSINFO_ATTR_SUPPORTS - Support for statx, ioctl, etc.
FSINFO_ATTR_TIMESTAMP_INFO - Inode timestamp info
FSINFO_ATTR_VOLUME_ID - Volume ID (string)
FSINFO_ATTR_VOLUME_UUID - Volume UUID
FSINFO_ATTR_VOLUME_NAME - Volume name (string)
FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO - Information about attr Nth
FSINFO_ATTR_FSINFO_ATTRIBUTES - List of supported attrs
Some attributes (such as the servers backing a network filesystem) can have
multiple values. These can be enumerated by setting params->Nth and
params->Mth to 0, 1, ... until ENODATA is returned.
result_buffer and result_buf_size point to the reply buffer. The buffer is
filled up to the specified size, even if this means truncating the reply.
The size of the full reply is returned, irrespective of the amount data
that was copied. In future versions, this will allow extra fields to be
tacked on to the end of the reply, but anyone not expecting them will only
get the subset they're expecting. If either buffer of result_buf_size are
0, no copy will take place and the data size will be returned.
[christian.brauner@ubuntu.com: split out syscall wire-up]
Signed-off-by: David Howells <dhowells@redhat.com>
cc: linux-api@vger.kernel.org
---
fs/Kconfig | 7 +
fs/Makefile | 1 +
fs/fsinfo.c | 582 +++++++++++++++++++++++++++++++++
include/linux/fs.h | 4 +
include/linux/fsinfo.h | 73 +++++
include/uapi/linux/fsinfo.h | 186 +++++++++++
kernel/sys_ni.c | 1 +
samples/vfs/Makefile | 5 +
samples/vfs/test-fsinfo.c | 633 ++++++++++++++++++++++++++++++++++++
9 files changed, 1492 insertions(+)
create mode 100644 fs/fsinfo.c
create mode 100644 include/linux/fsinfo.h
create mode 100644 include/uapi/linux/fsinfo.h
create mode 100644 samples/vfs/test-fsinfo.c
diff --git a/fs/Kconfig b/fs/Kconfig
index fef1365c23a5..01d0d436b3cd 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -15,6 +15,13 @@ config VALIDATE_FS_PARSER
Enable this to perform validation of the parameter description for a
filesystem when it is registered.
+config FSINFO
+ bool "Enable the fsinfo() system call"
+ help
+ Enable the file system information querying system call to allow
+ comprehensive information to be retrieved about a filesystem,
+ superblock or mount object.
+
if BLOCK
config FS_IOMAP
diff --git a/fs/Makefile b/fs/Makefile
index 4477757780d0..b6bf2424c7f7 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_COREDUMP) += coredump.o
obj-$(CONFIG_SYSCTL) += drop_caches.o
obj-$(CONFIG_FHANDLE) += fhandle.o
+obj-$(CONFIG_FSINFO) += fsinfo.o
obj-y += iomap/
obj-y += quota/
diff --git a/fs/fsinfo.c b/fs/fsinfo.c
new file mode 100644
index 000000000000..b7b81e9d7e21
--- /dev/null
+++ b/fs/fsinfo.c
@@ -0,0 +1,582 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Filesystem information query.
+ *
+ * Copyright (C) 2020 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+#include <linux/syscalls.h>
+#include <linux/fs.h>
+#include <linux/file.h>
+#include <linux/mount.h>
+#include <linux/namei.h>
+#include <linux/statfs.h>
+#include <linux/security.h>
+#include <linux/uaccess.h>
+#include <linux/fsinfo.h>
+#include <uapi/linux/mount.h>
+#include "internal.h"
+
+/**
+ * fsinfo_string - Store a NUL-terminated string as an fsinfo attribute value.
+ * @s: The string to store (may be NULL)
+ * @ctx: The parameter context
+ */
+int fsinfo_string(const char *s, struct fsinfo_context *ctx)
+{
+ unsigned int len;
+ char *p = ctx->buffer;
+ int ret = 0;
+
+ if (s) {
+ len = min_t(size_t, strlen(s), ctx->buf_size - 1);
+ if (!ctx->want_size_only) {
+ memcpy(p, s, len);
+ p[len] = 0;
+ }
+ ret = len;
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL(fsinfo_string);
+
+/*
+ * Get basic filesystem stats from statfs.
+ */
+static int fsinfo_generic_statfs(struct path *path, struct fsinfo_context *ctx)
+{
+ struct fsinfo_statfs *p = ctx->buffer;
+ struct kstatfs buf;
+ int ret;
+
+ ret = vfs_statfs(path, &buf);
+ if (ret < 0)
+ return ret;
+
+ p->f_blocks.lo = buf.f_blocks;
+ p->f_bfree.lo = buf.f_bfree;
+ p->f_bavail.lo = buf.f_bavail;
+ p->f_files.lo = buf.f_files;
+ p->f_ffree.lo = buf.f_ffree;
+ p->f_favail.lo = buf.f_ffree;
+ p->f_bsize = buf.f_bsize;
+ p->f_frsize = buf.f_frsize;
+ return sizeof(*p);
+}
+
+static int fsinfo_generic_ids(struct path *path, struct fsinfo_context *ctx)
+{
+ struct fsinfo_ids *p = ctx->buffer;
+ struct super_block *sb;
+ struct kstatfs buf;
+ int ret;
+
+ ret = vfs_statfs(path, &buf);
+ if (ret < 0 && ret != -ENOSYS)
+ return ret;
+ if (ret == 0)
+ memcpy(&p->f_fsid, &buf.f_fsid, sizeof(p->f_fsid));
+
+ sb = path->dentry->d_sb;
+ p->f_fstype = sb->s_magic;
+ p->f_dev_major = MAJOR(sb->s_dev);
+ p->f_dev_minor = MINOR(sb->s_dev);
+ p->f_sb_id = sb->s_unique_id;
+ strlcpy(p->f_fs_name, sb->s_type->name, sizeof(p->f_fs_name));
+ return sizeof(*p);
+}
+
+int fsinfo_generic_limits(struct path *path, struct fsinfo_context *ctx)
+{
+ struct fsinfo_limits *p = ctx->buffer;
+ struct super_block *sb = path->dentry->d_sb;
+
+ p->max_file_size.hi = 0;
+ p->max_file_size.lo = sb->s_maxbytes;
+ p->max_ino.hi = 0;
+ p->max_ino.lo = UINT_MAX;
+ p->max_hard_links = sb->s_max_links;
+ p->max_uid = UINT_MAX;
+ p->max_gid = UINT_MAX;
+ p->max_projid = UINT_MAX;
+ p->max_filename_len = NAME_MAX;
+ p->max_symlink_len = PATH_MAX;
+ p->max_xattr_name_len = XATTR_NAME_MAX;
+ p->max_xattr_body_len = XATTR_SIZE_MAX;
+ p->max_dev_major = 0xffffff;
+ p->max_dev_minor = 0xff;
+ return sizeof(*p);
+}
+EXPORT_SYMBOL(fsinfo_generic_limits);
+
+int fsinfo_generic_supports(struct path *path, struct fsinfo_context *ctx)
+{
+ struct fsinfo_supports *p = ctx->buffer;
+ struct super_block *sb = path->dentry->d_sb;
+
+ p->stx_mask = STATX_BASIC_STATS;
+ if (sb->s_d_op && sb->s_d_op->d_automount)
+ p->stx_attributes |= STATX_ATTR_AUTOMOUNT;
+ return sizeof(*p);
+}
+EXPORT_SYMBOL(fsinfo_generic_supports);
+
+static const struct fsinfo_timestamp_info fsinfo_default_timestamp_info = {
+ .atime = {
+ .minimum = S64_MIN,
+ .maximum = S64_MAX,
+ .gran_mantissa = 1,
+ .gran_exponent = 0,
+ },
+ .mtime = {
+ .minimum = S64_MIN,
+ .maximum = S64_MAX,
+ .gran_mantissa = 1,
+ .gran_exponent = 0,
+ },
+ .ctime = {
+ .minimum = S64_MIN,
+ .maximum = S64_MAX,
+ .gran_mantissa = 1,
+ .gran_exponent = 0,
+ },
+ .btime = {
+ .minimum = S64_MIN,
+ .maximum = S64_MAX,
+ .gran_mantissa = 1,
+ .gran_exponent = 0,
+ },
+};
+
+int fsinfo_generic_timestamp_info(struct path *path, struct fsinfo_context *ctx)
+{
+ struct fsinfo_timestamp_info *p = ctx->buffer;
+ struct super_block *sb = path->dentry->d_sb;
+ s8 exponent;
+
+ *p = fsinfo_default_timestamp_info;
+
+ if (sb->s_time_gran < 1000000000) {
+ if (sb->s_time_gran < 1000)
+ exponent = -9;
+ else if (sb->s_time_gran < 1000000)
+ exponent = -6;
+ else
+ exponent = -3;
+
+ p->atime.gran_exponent = exponent;
+ p->mtime.gran_exponent = exponent;
+ p->ctime.gran_exponent = exponent;
+ p->btime.gran_exponent = exponent;
+ }
+
+ return sizeof(*p);
+}
+EXPORT_SYMBOL(fsinfo_generic_timestamp_info);
+
+static int fsinfo_generic_volume_uuid(struct path *path, struct fsinfo_context *ctx)
+{
+ struct fsinfo_volume_uuid *p = ctx->buffer;
+ struct super_block *sb = path->dentry->d_sb;
+
+ memcpy(p, &sb->s_uuid, sizeof(*p));
+ return sizeof(*p);
+}
+
+static int fsinfo_generic_volume_id(struct path *path, struct fsinfo_context *ctx)
+{
+ return fsinfo_string(path->dentry->d_sb->s_id, ctx);
+}
+
+static const struct fsinfo_attribute fsinfo_common_attributes[] = {
+ FSINFO_VSTRUCT (FSINFO_ATTR_STATFS, fsinfo_generic_statfs),
+ FSINFO_VSTRUCT (FSINFO_ATTR_IDS, fsinfo_generic_ids),
+ FSINFO_VSTRUCT (FSINFO_ATTR_LIMITS, fsinfo_generic_limits),
+ FSINFO_VSTRUCT (FSINFO_ATTR_SUPPORTS, fsinfo_generic_supports),
+ FSINFO_VSTRUCT (FSINFO_ATTR_TIMESTAMP_INFO, fsinfo_generic_timestamp_info),
+ FSINFO_STRING (FSINFO_ATTR_VOLUME_ID, fsinfo_generic_volume_id),
+ FSINFO_VSTRUCT (FSINFO_ATTR_VOLUME_UUID, fsinfo_generic_volume_uuid),
+
+ FSINFO_LIST (FSINFO_ATTR_FSINFO_ATTRIBUTES, (void *)123UL),
+ FSINFO_VSTRUCT_N(FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO, (void *)123UL),
+ {}
+};
+
+/*
+ * Determine an attribute's minimum buffer size and, if the buffer is large
+ * enough, get the attribute value.
+ */
+static int fsinfo_get_this_attribute(struct path *path,
+ struct fsinfo_context *ctx,
+ const struct fsinfo_attribute *attr)
+{
+ int buf_size;
+
+ if (ctx->Nth != 0 && !(attr->flags & (FSINFO_FLAGS_N | FSINFO_FLAGS_NM)))
+ return -ENODATA;
+ if (ctx->Mth != 0 && !(attr->flags & FSINFO_FLAGS_NM))
+ return -ENODATA;
+
+ switch (attr->type) {
+ case FSINFO_TYPE_VSTRUCT:
+ ctx->clear_tail = true;
+ buf_size = attr->size;
+ break;
+ case FSINFO_TYPE_STRING:
+ case FSINFO_TYPE_OPAQUE:
+ case FSINFO_TYPE_LIST:
+ buf_size = 4096;
+ break;
+ default:
+ return -ENOPKG;
+ }
+
+ if (ctx->buf_size < buf_size)
+ return buf_size;
+
+ return attr->get(path, ctx);
+}
+
+static void fsinfo_attributes_insert(struct fsinfo_context *ctx,
+ const struct fsinfo_attribute *attr)
+{
+ __u32 *p = ctx->buffer;
+ unsigned int i;
+
+ if (ctx->usage >= ctx->buf_size ||
+ ctx->buf_size - ctx->usage < sizeof(__u32)) {
+ ctx->usage += sizeof(__u32);
+ return;
+ }
+
+ for (i = 0; i < ctx->usage / sizeof(__u32); i++)
+ if (p[i] == attr->attr_id)
+ return;
+
+ p[i] = attr->attr_id;
+ ctx->usage += sizeof(__u32);
+}
+
+static int fsinfo_list_attributes(struct path *path,
+ struct fsinfo_context *ctx,
+ const struct fsinfo_attribute *attributes)
+{
+ const struct fsinfo_attribute *a;
+
+ for (a = attributes; a->get; a++)
+ fsinfo_attributes_insert(ctx, a);
+ return -EOPNOTSUPP; /* We want to go through all the lists */
+}
+
+static int fsinfo_get_attribute_info(struct path *path,
+ struct fsinfo_context *ctx,
+ const struct fsinfo_attribute *attributes)
+{
+ const struct fsinfo_attribute *a;
+ struct fsinfo_attribute_info *p = ctx->buffer;
+
+ if (!ctx->buf_size)
+ return sizeof(*p);
+
+ for (a = attributes; a->get; a++) {
+ if (a->attr_id == ctx->Nth) {
+ p->attr_id = a->attr_id;
+ p->type = a->type;
+ p->flags = a->flags;
+ p->size = a->size;
+ p->size = a->size;
+ return sizeof(*p);
+ }
+ }
+ return -EOPNOTSUPP; /* We want to go through all the lists */
+}
+
+/**
+ * fsinfo_get_attribute - Look up and handle an attribute
+ * @path: The object to query
+ * @params: Parameters to define a request and place to store result
+ * @attributes: List of attributes to search.
+ *
+ * Look through a list of attributes for one that matches the requested
+ * attribute then call the handler for it.
+ */
+int fsinfo_get_attribute(struct path *path, struct fsinfo_context *ctx,
+ const struct fsinfo_attribute *attributes)
+{
+ const struct fsinfo_attribute *a;
+
+ switch (ctx->requested_attr) {
+ case FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO:
+ return fsinfo_get_attribute_info(path, ctx, attributes);
+ case FSINFO_ATTR_FSINFO_ATTRIBUTES:
+ return fsinfo_list_attributes(path, ctx, attributes);
+ default:
+ for (a = attributes; a->get; a++)
+ if (a->attr_id == ctx->requested_attr)
+ return fsinfo_get_this_attribute(path, ctx, a);
+ return -EOPNOTSUPP;
+ }
+}
+EXPORT_SYMBOL(fsinfo_get_attribute);
+
+/**
+ * generic_fsinfo - Handle an fsinfo attribute generically
+ * @path: The object to query
+ * @params: Parameters to define a request and place to store result
+ */
+static int fsinfo_call(struct path *path, struct fsinfo_context *ctx)
+{
+ int ret;
+
+ if (path->dentry->d_sb->s_op->fsinfo) {
+ ret = path->dentry->d_sb->s_op->fsinfo(path, ctx);
+ if (ret != -EOPNOTSUPP)
+ return ret;
+ }
+ ret = fsinfo_get_attribute(path, ctx, fsinfo_common_attributes);
+ if (ret != -EOPNOTSUPP)
+ return ret;
+
+ switch (ctx->requested_attr) {
+ case FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO:
+ return -ENODATA;
+ case FSINFO_ATTR_FSINFO_ATTRIBUTES:
+ return ctx->usage;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+/**
+ * vfs_fsinfo - Retrieve filesystem information
+ * @path: The object to query
+ * @params: Parameters to define a request and place to store result
+ *
+ * Get an attribute on a filesystem or an object within a filesystem. The
+ * filesystem attribute to be queried is indicated by @ctx->requested_attr, and
+ * if it's a multi-valued attribute, the particular value is selected by
+ * @ctx->Nth and then @ctx->Mth.
+ *
+ * For common attributes, a value may be fabricated if it is not supported by
+ * the filesystem.
+ *
+ * On success, the size of the attribute's value is returned (0 is a valid
+ * size). A buffer will have been allocated and will be pointed to by
+ * @ctx->buffer. The caller must free this with kvfree().
+ *
+ * Errors can also be returned: -ENOMEM if a buffer cannot be allocated, -EPERM
+ * or -EACCES if permission is denied by the LSM, -EOPNOTSUPP if an attribute
+ * doesn't exist for the specified object or -ENODATA if the attribute exists,
+ * but the Nth,Mth value does not exist. -EMSGSIZE indicates that the value is
+ * unmanageable internally and -ENOPKG indicates other internal failure.
+ *
+ * Errors such as -EIO may also come from attempts to access media or servers
+ * to obtain the requested information if it's not immediately to hand.
+ *
+ * [*] Note that the caller may set @ctx->want_size_only if it only wants the
+ * size of the value and not the data. If this is set, a buffer may not be
+ * allocated under some circumstances. This is intended for size query by
+ * userspace.
+ *
+ * [*] Note that @ctx->clear_tail will be returned set if the data should be
+ * padded out with zeros when writing it to userspace.
+ */
+static int vfs_fsinfo(struct path *path, struct fsinfo_context *ctx)
+{
+ struct dentry *dentry = path->dentry;
+ int ret;
+
+ ret = security_sb_statfs(dentry);
+ if (ret)
+ return ret;
+
+ /* Call the handler to find out the buffer size required. */
+ ctx->buf_size = 0;
+ ret = fsinfo_call(path, ctx);
+ if (ret < 0 || ctx->want_size_only)
+ return ret;
+ ctx->buf_size = ret;
+
+ do {
+ /* Allocate a buffer of the requested size. */
+ if (ctx->buf_size > INT_MAX)
+ return -EMSGSIZE;
+ ctx->buffer = kvzalloc(ctx->buf_size, GFP_KERNEL);
+ if (!ctx->buffer)
+ return -ENOMEM;
+
+ ctx->usage = 0;
+ ctx->skip = 0;
+ ret = fsinfo_call(path, ctx);
+ if (IS_ERR_VALUE((long)ret))
+ return ret;
+ if ((unsigned int)ret <= ctx->buf_size)
+ return ret; /* It fitted */
+
+ /* We need to resize the buffer */
+ ctx->buf_size = roundup(ret, PAGE_SIZE);
+ kvfree(ctx->buffer);
+ ctx->buffer = NULL;
+ } while (!signal_pending(current));
+
+ return -ERESTARTSYS;
+}
+
+static int vfs_fsinfo_path(int dfd, const char __user *pathname,
+ unsigned int resolve_flags, struct fsinfo_context *ctx)
+{
+ struct path path;
+ unsigned lookup_flags = LOOKUP_FOLLOW | LOOKUP_AUTOMOUNT;
+ int ret = -EINVAL;
+
+ if (resolve_flags & ~VALID_RESOLVE_FLAGS)
+ return -EINVAL;
+
+ if (resolve_flags & RESOLVE_NO_XDEV)
+ lookup_flags |= LOOKUP_NO_XDEV;
+ if (resolve_flags & RESOLVE_NO_MAGICLINKS)
+ lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ if (resolve_flags & RESOLVE_NO_SYMLINKS)
+ lookup_flags |= LOOKUP_NO_SYMLINKS;
+ if (resolve_flags & RESOLVE_BENEATH)
+ lookup_flags |= LOOKUP_BENEATH;
+ if (resolve_flags & RESOLVE_IN_ROOT)
+ lookup_flags |= LOOKUP_IN_ROOT;
+ if (resolve_flags & RESOLVE_NO_TRAILING_SYMLINKS)
+ lookup_flags &= ~LOOKUP_FOLLOW;
+ if (resolve_flags & RESOLVE_NO_TRAILING_AUTOMOUNTS)
+ lookup_flags &= ~LOOKUP_AUTOMOUNT;
+ if (resolve_flags & RESOLVE_EMPTY_PATH)
+ lookup_flags |= LOOKUP_EMPTY;
+
+retry:
+ ret = user_path_at(dfd, pathname, lookup_flags, &path);
+ if (ret)
+ goto out;
+
+ ret = vfs_fsinfo(&path, ctx);
+ path_put(&path);
+ if (retry_estale(ret, lookup_flags)) {
+ lookup_flags |= LOOKUP_REVAL;
+ goto retry;
+ }
+out:
+ return ret;
+}
+
+static int vfs_fsinfo_fd(unsigned int fd, struct fsinfo_context *ctx)
+{
+ struct fd f = fdget_raw(fd);
+ int ret = -EBADF;
+
+ if (f.file) {
+ ret = vfs_fsinfo(&f.file->f_path, ctx);
+ fdput(f);
+ }
+ return ret;
+}
+
+/**
+ * sys_fsinfo - System call to get filesystem information
+ * @dfd: Base directory to pathwalk from or fd referring to filesystem.
+ * @pathname: Filesystem to query or NULL.
+ * @params: Parameters to define request (NULL: FSINFO_ATTR_STATFS).
+ * @params_size: Size of parameter buffer.
+ * @result_buffer: Result buffer.
+ * @result_buf_size: Size of result buffer.
+ *
+ * Get information on a filesystem. The filesystem attribute to be queried is
+ * indicated by @_params->request, and some of the attributes can have multiple
+ * values, indexed by @_params->Nth and @_params->Mth. If @_params is NULL,
+ * then the 0th fsinfo_attr_statfs attribute is queried. If an attribute does
+ * not exist, EOPNOTSUPP is returned; if the Nth,Mth value does not exist,
+ * ENODATA is returned.
+ *
+ * On success, the size of the attribute's value is returned. If
+ * @result_buf_size is 0 or @result_buffer is NULL, only the size is returned.
+ * If the size of the value is larger than @result_buf_size, it will be
+ * truncated by the copy. If the size of the value is smaller than
+ * @result_buf_size then the excess buffer space will be cleared. The full
+ * size of the value will be returned, irrespective of how much data is
+ * actually placed in the buffer.
+ */
+SYSCALL_DEFINE6(fsinfo,
+ int, dfd,
+ const char __user *, pathname,
+ const struct fsinfo_params __user *, params,
+ size_t, params_size,
+ void __user *, result_buffer,
+ size_t, result_buf_size)
+{
+ struct fsinfo_context ctx;
+ struct fsinfo_params user_params;
+ unsigned int result_size;
+ void *r;
+ int ret;
+
+ if ((!params && params_size) ||
+ ( params && !params_size) ||
+ (!result_buffer && result_buf_size) ||
+ ( result_buffer && !result_buf_size))
+ return -EINVAL;
+ if (result_buf_size > UINT_MAX)
+ return -EOVERFLOW;
+
+ memset(&ctx, 0, sizeof(ctx));
+ ctx.requested_attr = FSINFO_ATTR_STATFS;
+ ctx.flags = FSINFO_FLAGS_QUERY_PATH;
+ ctx.want_size_only = (result_buf_size == 0);
+
+ if (params) {
+ ret = copy_struct_from_user(&user_params, sizeof(user_params),
+ params, params_size);
+ if (ret < 0)
+ return ret;
+ if (user_params.flags & ~FSINFO_FLAGS_QUERY_MASK)
+ return -EINVAL;
+ ctx.flags = user_params.flags;
+ ctx.requested_attr = user_params.request;
+ ctx.Nth = user_params.Nth;
+ ctx.Mth = user_params.Mth;
+ }
+
+ switch (ctx.flags & FSINFO_FLAGS_QUERY_MASK) {
+ case FSINFO_FLAGS_QUERY_PATH:
+ ret = vfs_fsinfo_path(dfd, pathname, user_params.resolve_flags, &ctx);
+ break;
+ case FSINFO_FLAGS_QUERY_FD:
+ if (pathname)
+ return -EINVAL;
+ ret = vfs_fsinfo_fd(dfd, &ctx);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (ret < 0)
+ goto error;
+
+ r = ctx.buffer + ctx.skip;
+ result_size = min_t(size_t, ret, result_buf_size);
+ if (result_size > 0 &&
+ copy_to_user(result_buffer, r, result_size) != 0) {
+ ret = -EFAULT;
+ goto error;
+ }
+
+ /* Clear any part of the buffer that we won't fill if we're putting a
+ * struct in there. Strings, opaque objects and arrays are expected to
+ * be variable length.
+ */
+ if (ctx.clear_tail &&
+ result_buf_size > result_size &&
+ clear_user(result_buffer + result_size,
+ result_buf_size - result_size) != 0) {
+ ret = -EFAULT;
+ goto error;
+ }
+
+error:
+ kvfree(ctx.buffer);
+ return ret;
+}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9181cfcd5265..39178f89a6ad 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -69,6 +69,7 @@ struct fsverity_info;
struct fsverity_operations;
struct fs_context;
struct fs_parameter_spec;
+struct fsinfo_context;
extern void __init inode_init(void);
extern void __init inode_init_early(void);
@@ -1964,6 +1965,9 @@ struct super_operations {
int (*thaw_super) (struct super_block *);
int (*unfreeze_fs) (struct super_block *);
int (*statfs) (struct dentry *, struct kstatfs *);
+#ifdef CONFIG_FSINFO
+ int (*fsinfo)(struct path *, struct fsinfo_context *);
+#endif
int (*remount_fs) (struct super_block *, int *, char *);
void (*umount_begin) (struct super_block *);
diff --git a/include/linux/fsinfo.h b/include/linux/fsinfo.h
new file mode 100644
index 000000000000..bf806669b4fb
--- /dev/null
+++ b/include/linux/fsinfo.h
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Filesystem information query
+ *
+ * Copyright (C) 2020 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#ifndef _LINUX_FSINFO_H
+#define _LINUX_FSINFO_H
+
+#ifdef CONFIG_FSINFO
+
+#include <uapi/linux/fsinfo.h>
+
+struct path;
+
+#define FSINFO_NORMAL_ATTR_MAX_SIZE 4096
+
+struct fsinfo_context {
+ __u32 flags; /* [in] FSINFO_FLAGS_* */
+ __u32 requested_attr; /* [in] What is being asking for */
+ __u32 Nth; /* [in] Instance of it (some may have multiple) */
+ __u32 Mth; /* [in] Subinstance */
+ bool want_size_only; /* [in] Just want to know the size, not the data */
+ bool clear_tail; /* [out] T if tail of buffer should be cleared */
+ unsigned int skip; /* [out] Number of bytes to skip in buffer */
+ unsigned int usage; /* [tmp] Amount of buffer used (if large) */
+ unsigned int buf_size; /* [tmp] Size of ->buffer[] */
+ void *buffer; /* [out] The reply buffer */
+};
+
+/*
+ * A filesystem information attribute definition.
+ */
+struct fsinfo_attribute {
+ unsigned int attr_id; /* The ID of the attribute */
+ enum fsinfo_value_type type:8; /* The type of the attribute's value(s) */
+ unsigned int flags:8;
+ unsigned int size:16; /* - Value size (FSINFO_STRUCT/LIST) */
+ int (*get)(struct path *path, struct fsinfo_context *params);
+};
+
+#define __FSINFO(A, T, S, G, F) \
+ { .attr_id = A, .type = T, .flags = F, .size = S, .get = G }
+
+#define _FSINFO(A, T, S, G) __FSINFO(A, T, S, G, 0)
+#define _FSINFO_N(A, T, S, G) __FSINFO(A, T, S, G, FSINFO_FLAGS_N)
+#define _FSINFO_NM(A, T, S, G) __FSINFO(A, T, S, G, FSINFO_FLAGS_NM)
+
+#define _FSINFO_VSTRUCT(A,S,G) _FSINFO (A, FSINFO_TYPE_VSTRUCT, sizeof(S), G)
+#define _FSINFO_VSTRUCT_N(A,S,G) _FSINFO_N (A, FSINFO_TYPE_VSTRUCT, sizeof(S), G)
+#define _FSINFO_VSTRUCT_NM(A,S,G) _FSINFO_NM(A, FSINFO_TYPE_VSTRUCT, sizeof(S), G)
+
+#define FSINFO_VSTRUCT(A,G) _FSINFO_VSTRUCT (A, A##__STRUCT, G)
+#define FSINFO_VSTRUCT_N(A,G) _FSINFO_VSTRUCT_N (A, A##__STRUCT, G)
+#define FSINFO_VSTRUCT_NM(A,G) _FSINFO_VSTRUCT_NM(A, A##__STRUCT, G)
+#define FSINFO_STRING(A,G) _FSINFO (A, FSINFO_TYPE_STRING, 0, G)
+#define FSINFO_STRING_N(A,G) _FSINFO_N (A, FSINFO_TYPE_STRING, 0, G)
+#define FSINFO_STRING_NM(A,G) _FSINFO_NM(A, FSINFO_TYPE_STRING, 0, G)
+#define FSINFO_OPAQUE(A,G) _FSINFO (A, FSINFO_TYPE_OPAQUE, 0, G)
+#define FSINFO_LIST(A,G) _FSINFO (A, FSINFO_TYPE_LIST, sizeof(A##__STRUCT), G)
+#define FSINFO_LIST_N(A,G) _FSINFO_N (A, FSINFO_TYPE_LIST, sizeof(A##__STRUCT), G)
+
+extern int fsinfo_string(const char *, struct fsinfo_context *);
+extern int fsinfo_generic_timestamp_info(struct path *, struct fsinfo_context *);
+extern int fsinfo_generic_supports(struct path *, struct fsinfo_context *);
+extern int fsinfo_generic_limits(struct path *, struct fsinfo_context *);
+extern int fsinfo_get_attribute(struct path *, struct fsinfo_context *,
+ const struct fsinfo_attribute *);
+
+#endif /* CONFIG_FSINFO */
+
+#endif /* _LINUX_FSINFO_H */
diff --git a/include/uapi/linux/fsinfo.h b/include/uapi/linux/fsinfo.h
new file mode 100644
index 000000000000..b56ebd525b03
--- /dev/null
+++ b/include/uapi/linux/fsinfo.h
@@ -0,0 +1,186 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/* fsinfo() definitions.
+ *
+ * Copyright (C) 2020 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+#ifndef _UAPI_LINUX_FSINFO_H
+#define _UAPI_LINUX_FSINFO_H
+
+#include <linux/types.h>
+#include <linux/socket.h>
+#include <linux/openat2.h>
+
+/*
+ * The filesystem attributes that can be requested. Note that some attributes
+ * may have multiple instances which can be switched in the parameter block.
+ */
+#define FSINFO_ATTR_STATFS 0x00 /* statfs()-style state */
+#define FSINFO_ATTR_IDS 0x01 /* Filesystem IDs */
+#define FSINFO_ATTR_LIMITS 0x02 /* Filesystem limits */
+#define FSINFO_ATTR_SUPPORTS 0x03 /* What's supported in statx, iocflags, ... */
+#define FSINFO_ATTR_TIMESTAMP_INFO 0x04 /* Inode timestamp info */
+#define FSINFO_ATTR_VOLUME_ID 0x05 /* Volume ID (string) */
+#define FSINFO_ATTR_VOLUME_UUID 0x06 /* Volume UUID (LE uuid) */
+#define FSINFO_ATTR_VOLUME_NAME 0x07 /* Volume name (string) */
+
+#define FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO 0x100 /* Information about attr N (for path) */
+#define FSINFO_ATTR_FSINFO_ATTRIBUTES 0x101 /* List of supported attrs (for path) */
+
+/*
+ * Optional fsinfo() parameter structure.
+ *
+ * If this is not given, it is assumed that fsinfo_attr_statfs instance 0,0 is
+ * desired.
+ */
+struct fsinfo_params {
+ __u32 flags; /* Flags controlling fsinfo() specifically */
+#define FSINFO_FLAGS_QUERY_MASK 0x0007 /* What object should fsinfo() query? */
+#define FSINFO_FLAGS_QUERY_PATH 0x0000 /* - path, specified by dirfd,pathname,AT_EMPTY_PATH */
+#define FSINFO_FLAGS_QUERY_FD 0x0001 /* - fd specified by dirfd */
+ __u32 resolve_flags; /* RESOLVE_* flags */
+ __u32 request; /* ID of requested attribute */
+ __u32 Nth; /* Instance of it (some may have multiple) */
+ __u32 Mth; /* Subinstance of Nth instance */
+};
+
+enum fsinfo_value_type {
+ FSINFO_TYPE_VSTRUCT = 0, /* Version-lengthed struct (up to 4096 bytes) */
+ FSINFO_TYPE_STRING = 1, /* NUL-term var-length string (up to 4095 chars) */
+ FSINFO_TYPE_OPAQUE = 2, /* Opaque blob (unlimited size) */
+ FSINFO_TYPE_LIST = 3, /* List of ints/structs (unlimited size) */
+};
+
+/*
+ * Information struct for fsinfo(FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO).
+ *
+ * This gives information about the attributes supported by fsinfo for the
+ * given path.
+ */
+struct fsinfo_attribute_info {
+ unsigned int attr_id; /* The ID of the attribute */
+ enum fsinfo_value_type type; /* The type of the attribute's value(s) */
+ unsigned int flags;
+#define FSINFO_FLAGS_N 0x01 /* - Attr has a set of values */
+#define FSINFO_FLAGS_NM 0x02 /* - Attr has a set of sets of values */
+ unsigned int size; /* - Value size (FSINFO_STRUCT/FSINFO_LIST) */
+};
+
+#define FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO__STRUCT struct fsinfo_attribute_info
+#define FSINFO_ATTR_FSINFO_ATTRIBUTES__STRUCT __u32
+
+struct fsinfo_u128 {
+#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
+ __u64 hi;
+ __u64 lo;
+#elif defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
+ __u64 lo;
+ __u64 hi;
+#endif
+};
+
+/*
+ * Information struct for fsinfo(FSINFO_ATTR_STATFS).
+ * - This gives extended filesystem information.
+ */
+struct fsinfo_statfs {
+ struct fsinfo_u128 f_blocks; /* Total number of blocks in fs */
+ struct fsinfo_u128 f_bfree; /* Total number of free blocks */
+ struct fsinfo_u128 f_bavail; /* Number of free blocks available to ordinary user */
+ struct fsinfo_u128 f_files; /* Total number of file nodes in fs */
+ struct fsinfo_u128 f_ffree; /* Number of free file nodes */
+ struct fsinfo_u128 f_favail; /* Number of file nodes available to ordinary user */
+ __u64 f_bsize; /* Optimal block size */
+ __u64 f_frsize; /* Fragment size */
+};
+
+#define FSINFO_ATTR_STATFS__STRUCT struct fsinfo_statfs
+
+/*
+ * Information struct for fsinfo(FSINFO_ATTR_IDS).
+ *
+ * List of basic identifiers as is normally found in statfs().
+ */
+struct fsinfo_ids {
+ char f_fs_name[15 + 1]; /* Filesystem name */
+ __u64 f_fsid; /* Short 64-bit Filesystem ID (as statfs) */
+ __u64 f_sb_id; /* Internal superblock ID for sbnotify()/mntnotify() */
+ __u32 f_fstype; /* Filesystem type from linux/magic.h [uncond] */
+ __u32 f_dev_major; /* As st_dev_* from struct statx [uncond] */
+ __u32 f_dev_minor;
+ __u32 __padding[1];
+};
+
+#define FSINFO_ATTR_IDS__STRUCT struct fsinfo_ids
+
+/*
+ * Information struct for fsinfo(FSINFO_ATTR_LIMITS).
+ *
+ * List of supported filesystem limits.
+ */
+struct fsinfo_limits {
+ struct fsinfo_u128 max_file_size; /* Maximum file size */
+ struct fsinfo_u128 max_ino; /* Maximum inode number */
+ __u64 max_uid; /* Maximum UID supported */
+ __u64 max_gid; /* Maximum GID supported */
+ __u64 max_projid; /* Maximum project ID supported */
+ __u64 max_hard_links; /* Maximum number of hard links on a file */
+ __u64 max_xattr_body_len; /* Maximum xattr content length */
+ __u32 max_xattr_name_len; /* Maximum xattr name length */
+ __u32 max_filename_len; /* Maximum filename length */
+ __u32 max_symlink_len; /* Maximum symlink content length */
+ __u32 max_dev_major; /* Maximum device major representable */
+ __u32 max_dev_minor; /* Maximum device minor representable */
+ __u32 __padding[1];
+};
+
+#define FSINFO_ATTR_LIMITS__STRUCT struct fsinfo_limits
+
+/*
+ * Information struct for fsinfo(FSINFO_ATTR_SUPPORTS).
+ *
+ * What's supported in various masks, such as statx() attribute and mask bits
+ * and IOC flags.
+ */
+struct fsinfo_supports {
+ __u64 stx_attributes; /* What statx::stx_attributes are supported */
+ __u32 stx_mask; /* What statx::stx_mask bits are supported */
+ __u32 fs_ioc_getflags; /* What FS_IOC_GETFLAGS may return */
+ __u32 fs_ioc_setflags_set; /* What FS_IOC_SETFLAGS may set */
+ __u32 fs_ioc_setflags_clear; /* What FS_IOC_SETFLAGS may clear */
+ __u32 win_file_attrs; /* What DOS/Windows FILE_* attributes are supported */
+ __u32 __padding[1];
+};
+
+#define FSINFO_ATTR_SUPPORTS__STRUCT struct fsinfo_supports
+
+struct fsinfo_timestamp_one {
+ __s64 minimum; /* Minimum timestamp value in seconds */
+ __s64 maximum; /* Maximum timestamp value in seconds */
+ __u16 gran_mantissa; /* Granularity(secs) = mant * 10^exp */
+ __s8 gran_exponent;
+ __u8 __padding[5];
+};
+
+/*
+ * Information struct for fsinfo(FSINFO_ATTR_TIMESTAMP_INFO).
+ */
+struct fsinfo_timestamp_info {
+ struct fsinfo_timestamp_one atime; /* Access time */
+ struct fsinfo_timestamp_one mtime; /* Modification time */
+ struct fsinfo_timestamp_one ctime; /* Change time */
+ struct fsinfo_timestamp_one btime; /* Birth/creation time */
+};
+
+#define FSINFO_ATTR_TIMESTAMP_INFO__STRUCT struct fsinfo_timestamp_info
+
+/*
+ * Information struct for fsinfo(FSINFO_ATTR_VOLUME_UUID).
+ */
+struct fsinfo_volume_uuid {
+ __u8 uuid[16];
+};
+
+#define FSINFO_ATTR_VOLUME_UUID__STRUCT struct fsinfo_volume_uuid
+
+#endif /* _UAPI_LINUX_FSINFO_H */
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 0ce01f86e5db..519317f3904c 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -51,6 +51,7 @@ COND_SYSCALL_COMPAT(io_pgetevents);
COND_SYSCALL(io_uring_setup);
COND_SYSCALL(io_uring_enter);
COND_SYSCALL(io_uring_register);
+COND_SYSCALL(fsinfo);
/* fs/xattr.c */
diff --git a/samples/vfs/Makefile b/samples/vfs/Makefile
index 65acdde5c117..9159ad1d7fc5 100644
--- a/samples/vfs/Makefile
+++ b/samples/vfs/Makefile
@@ -1,10 +1,15 @@
# SPDX-License-Identifier: GPL-2.0-only
# List of programs to build
+
hostprogs := \
+ test-fsinfo \
test-fsmount \
test-statx
always-y := $(hostprogs)
+HOSTCFLAGS_test-fsinfo.o += -I$(objtree)/usr/include
+HOSTLDLIBS_test-fsinfo += -static -lm
+
HOSTCFLAGS_test-fsmount.o += -I$(objtree)/usr/include
HOSTCFLAGS_test-statx.o += -I$(objtree)/usr/include
diff --git a/samples/vfs/test-fsinfo.c b/samples/vfs/test-fsinfo.c
new file mode 100644
index 000000000000..67aebf9fc9d8
--- /dev/null
+++ b/samples/vfs/test-fsinfo.c
@@ -0,0 +1,633 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Test the fsinfo() system call
+ *
+ * Copyright (C) 2020 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ */
+
+#define _GNU_SOURCE
+#define _ATFILE_SOURCE
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <unistd.h>
+#include <ctype.h>
+#include <errno.h>
+#include <time.h>
+#include <math.h>
+#include <fcntl.h>
+#include <sys/syscall.h>
+#include <linux/fsinfo.h>
+#include <linux/socket.h>
+#include <sys/stat.h>
+#include <arpa/inet.h>
+
+#ifndef __NR_fsinfo
+#define __NR_fsinfo -1
+#endif
+
+static bool debug = 0;
+static bool list_last;
+
+static __attribute__((unused))
+ssize_t fsinfo(int dfd, const char *filename,
+ struct fsinfo_params *params, size_t params_size,
+ void *result_buffer, size_t result_buf_size)
+{
+ return syscall(__NR_fsinfo, dfd, filename,
+ params, params_size,
+ result_buffer, result_buf_size);
+}
+
+struct fsinfo_attribute {
+ unsigned int attr_id;
+ enum fsinfo_value_type type;
+ unsigned int size;
+ const char *name;
+ void (*dump)(void *reply, unsigned int size);
+};
+
+static const struct fsinfo_attribute fsinfo_attributes[];
+
+static ssize_t get_fsinfo(const char *, const char *, struct fsinfo_params *, void **);
+
+static void dump_hex(unsigned int *data, int from, int to)
+{
+ unsigned offset, print_offset = 1, col = 0;
+
+ from /= 4;
+ to = (to + 3) / 4;
+
+ for (offset = from; offset < to; offset++) {
+ if (print_offset) {
+ printf("%04x: ", offset * 8);
+ print_offset = 0;
+ }
+ printf("%08x", data[offset]);
+ col++;
+ if ((col & 3) == 0) {
+ printf("\n");
+ print_offset = 1;
+ } else {
+ printf(" ");
+ }
+ }
+
+ if (!print_offset)
+ printf("\n");
+}
+
+static void dump_attribute_info(void *reply, unsigned int size)
+{
+ struct fsinfo_attribute_info *attr_info = reply;
+ const struct fsinfo_attribute *attr;
+ char type[32], val_size[32];
+
+ switch (attr_info->type) {
+ case FSINFO_TYPE_VSTRUCT: strcpy(type, "V-STRUCT"); break;
+ case FSINFO_TYPE_STRING: strcpy(type, "STRING"); break;
+ case FSINFO_TYPE_OPAQUE: strcpy(type, "OPAQUE"); break;
+ case FSINFO_TYPE_LIST: strcpy(type, "LIST"); break;
+ default:
+ sprintf(type, "type-%x", attr_info->type);
+ break;
+ }
+
+ if (attr_info->flags & FSINFO_FLAGS_N)
+ strcat(type, " x N");
+ else if (attr_info->flags & FSINFO_FLAGS_NM)
+ strcat(type, " x NM");
+
+ for (attr = fsinfo_attributes; attr->name; attr++)
+ if (attr->attr_id == attr_info->attr_id)
+ break;
+
+ if (attr_info->size)
+ sprintf(val_size, "%u", attr_info->size);
+ else
+ strcpy(val_size, "-");
+
+ printf("%8x %-12s %08x %5s %s\n",
+ attr_info->attr_id,
+ type,
+ attr_info->flags,
+ val_size,
+ attr->name ? attr->name : "");
+}
+
+static void dump_fsinfo_generic_statfs(void *reply, unsigned int size)
+{
+ struct fsinfo_statfs *f = reply;
+
+ printf("\n");
+ printf("\tblocks : n=%llu fr=%llu av=%llu\n",
+ (unsigned long long)f->f_blocks.lo,
+ (unsigned long long)f->f_bfree.lo,
+ (unsigned long long)f->f_bavail.lo);
+
+ printf("\tfiles : n=%llu fr=%llu av=%llu\n",
+ (unsigned long long)f->f_files.lo,
+ (unsigned long long)f->f_ffree.lo,
+ (unsigned long long)f->f_favail.lo);
+ printf("\tbsize : %llu\n", f->f_bsize);
+ printf("\tfrsize : %llu\n", f->f_frsize);
+}
+
+static void dump_fsinfo_generic_ids(void *reply, unsigned int size)
+{
+ struct fsinfo_ids *f = reply;
+
+ printf("\n");
+ printf("\tdev : %02x:%02x\n", f->f_dev_major, f->f_dev_minor);
+ printf("\tfs : type=%x name=%s\n", f->f_fstype, f->f_fs_name);
+ printf("\tfsid : %llx\n", (unsigned long long)f->f_fsid);
+ printf("\tsbid : %llx\n", (unsigned long long)f->f_sb_id);
+}
+
+static void dump_fsinfo_generic_limits(void *reply, unsigned int size)
+{
+ struct fsinfo_limits *f = reply;
+
+ printf("\n");
+ printf("\tmax file size: %llx%016llx\n",
+ (unsigned long long)f->max_file_size.hi,
+ (unsigned long long)f->max_file_size.lo);
+ printf("\tmax ino : %llx%016llx\n",
+ (unsigned long long)f->max_ino.hi,
+ (unsigned long long)f->max_ino.lo);
+ printf("\tmax ids : u=%llx g=%llx p=%llx\n",
+ (unsigned long long)f->max_uid,
+ (unsigned long long)f->max_gid,
+ (unsigned long long)f->max_projid);
+ printf("\tmax dev : maj=%x min=%x\n",
+ f->max_dev_major, f->max_dev_minor);
+ printf("\tmax links : %llx\n",
+ (unsigned long long)f->max_hard_links);
+ printf("\tmax xattr : n=%x b=%llx\n",
+ f->max_xattr_name_len,
+ (unsigned long long)f->max_xattr_body_len);
+ printf("\tmax len : file=%x sym=%x\n",
+ f->max_filename_len, f->max_symlink_len);
+}
+
+static void dump_fsinfo_generic_supports(void *reply, unsigned int size)
+{
+ struct fsinfo_supports *f = reply;
+
+ printf("\n");
+ printf("\tstx_attr : %llx\n", (unsigned long long)f->stx_attributes);
+ printf("\tstx_mask : %x\n", f->stx_mask);
+ printf("\tfs_ioc_*flags: get=%x set=%x clr=%x\n",
+ f->fs_ioc_getflags, f->fs_ioc_setflags_set, f->fs_ioc_setflags_clear);
+ printf("\twin_fattrs : %x\n", f->win_file_attrs);
+}
+
+static void print_time(struct fsinfo_timestamp_one *t, char stamp)
+{
+ printf("\t%ctime : gran=%gs range=%llx-%llx\n",
+ stamp,
+ t->gran_mantissa * pow(10., t->gran_exponent),
+ (long long)t->minimum,
+ (long long)t->maximum);
+}
+
+static void dump_fsinfo_generic_timestamp_info(void *reply, unsigned int size)
+{
+ struct fsinfo_timestamp_info *f = reply;
+
+ printf("\n");
+ print_time(&f->atime, 'a');
+ print_time(&f->mtime, 'm');
+ print_time(&f->ctime, 'c');
+ print_time(&f->btime, 'b');
+}
+
+static void dump_fsinfo_generic_volume_uuid(void *reply, unsigned int size)
+{
+ struct fsinfo_volume_uuid *f = reply;
+
+ printf("%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x"
+ "-%02x%02x%02x%02x%02x%02x\n",
+ f->uuid[ 0], f->uuid[ 1],
+ f->uuid[ 2], f->uuid[ 3],
+ f->uuid[ 4], f->uuid[ 5],
+ f->uuid[ 6], f->uuid[ 7],
+ f->uuid[ 8], f->uuid[ 9],
+ f->uuid[10], f->uuid[11],
+ f->uuid[12], f->uuid[13],
+ f->uuid[14], f->uuid[15]);
+}
+
+static void dump_string(void *reply, unsigned int size)
+{
+ char *s = reply, *p;
+ bool nl = false, last_nl = false;
+
+ p = s;
+ if (size >= 4096) {
+ size = 4096;
+ p[4092] = '.';
+ p[4093] = '.';
+ p[4094] = '.';
+ p[4095] = 0;
+ } else {
+ p[size] = 0;
+ }
+
+ for (p = s; *p; p++) {
+ if (*p == '\n') {
+ last_nl = nl = true;
+ continue;
+ }
+ last_nl = false;
+ if (!isprint(*p) && *p != '\t')
+ *p = '?';
+ }
+
+ if (nl)
+ putchar('\n');
+ printf("%s", s);
+ if (!last_nl)
+ putchar('\n');
+}
+
+#define dump_fsinfo_meta_attribute_info (void *)0x123
+#define dump_fsinfo_meta_attributes (void *)0x123
+
+/*
+ *
+ */
+#define __FSINFO(A, T, S, G, F, N) \
+ { .attr_id = A, .type = T, .size = S, .name = N, .dump = dump_##G }
+
+#define _FSINFO(A,T,S,G,N) __FSINFO(A, T, S, G, 0, N)
+#define _FSINFO_N(A,T,S,G,N) __FSINFO(A, T, S, G, FSINFO_FLAGS_N, N)
+#define _FSINFO_NM(A,T,S,G,N) __FSINFO(A, T, S, G, FSINFO_FLAGS_NM, N)
+
+#define _FSINFO_VSTRUCT(A,S,G,N) _FSINFO (A, FSINFO_TYPE_VSTRUCT, sizeof(S), G, N)
+#define _FSINFO_VSTRUCT_N(A,S,G,N) _FSINFO_N (A, FSINFO_TYPE_VSTRUCT, sizeof(S), G, N)
+#define _FSINFO_VSTRUCT_NM(A,S,G,N) _FSINFO_NM(A, FSINFO_TYPE_VSTRUCT, sizeof(S), G, N)
+
+#define FSINFO_VSTRUCT(A,G) _FSINFO_VSTRUCT (A, A##__STRUCT, G, #A)
+#define FSINFO_VSTRUCT_N(A,G) _FSINFO_VSTRUCT_N (A, A##__STRUCT, G, #A)
+#define FSINFO_VSTRUCT_NM(A,G) _FSINFO_VSTRUCT_NM(A, A##__STRUCT, G, #A)
+#define FSINFO_STRING(A,G) _FSINFO (A, FSINFO_TYPE_STRING, 0, G, #A)
+#define FSINFO_STRING_N(A,G) _FSINFO_N (A, FSINFO_TYPE_STRING, 0, G, #A)
+#define FSINFO_STRING_NM(A,G) _FSINFO_NM(A, FSINFO_TYPE_STRING, 0, G, #A)
+#define FSINFO_OPAQUE(A,G) _FSINFO (A, FSINFO_TYPE_OPAQUE, 0, G, #A)
+#define FSINFO_LIST(A,G) _FSINFO (A, FSINFO_TYPE_LIST, sizeof(A##__STRUCT), G, #A)
+#define FSINFO_LIST_N(A,G) _FSINFO_N (A, FSINFO_TYPE_LIST, sizeof(A##__STRUCT), G, #A)
+
+static const struct fsinfo_attribute fsinfo_attributes[] = {
+ FSINFO_VSTRUCT (FSINFO_ATTR_STATFS, fsinfo_generic_statfs),
+ FSINFO_VSTRUCT (FSINFO_ATTR_IDS, fsinfo_generic_ids),
+ FSINFO_VSTRUCT (FSINFO_ATTR_LIMITS, fsinfo_generic_limits),
+ FSINFO_VSTRUCT (FSINFO_ATTR_SUPPORTS, fsinfo_generic_supports),
+ FSINFO_VSTRUCT (FSINFO_ATTR_TIMESTAMP_INFO, fsinfo_generic_timestamp_info),
+ FSINFO_STRING (FSINFO_ATTR_VOLUME_ID, string),
+ FSINFO_VSTRUCT (FSINFO_ATTR_VOLUME_UUID, fsinfo_generic_volume_uuid),
+ FSINFO_STRING (FSINFO_ATTR_VOLUME_NAME, string),
+ FSINFO_VSTRUCT_N(FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO, fsinfo_meta_attribute_info),
+ FSINFO_LIST (FSINFO_ATTR_FSINFO_ATTRIBUTES, fsinfo_meta_attributes),
+ {}
+};
+
+static void dump_value(unsigned int attr_id,
+ const struct fsinfo_attribute *attr,
+ const struct fsinfo_attribute_info *attr_info,
+ void *reply, unsigned int size)
+{
+ if (!attr || !attr->dump) {
+ printf("<no dumper>\n");
+ return;
+ }
+
+ if (attr->type == FSINFO_TYPE_VSTRUCT && size < attr->size) {
+ printf("<short data %u/%u>\n", size, attr->size);
+ return;
+ }
+
+ attr->dump(reply, size);
+}
+
+static void dump_list(unsigned int attr_id,
+ const struct fsinfo_attribute *attr,
+ const struct fsinfo_attribute_info *attr_info,
+ void *reply, unsigned int size)
+{
+ size_t elem_size = attr_info->size;
+ unsigned int ix = 0;
+
+ printf("\n");
+ if (!attr || !attr->dump) {
+ printf("<no dumper>\n");
+ return;
+ }
+
+ if (attr->type == FSINFO_TYPE_VSTRUCT && size < attr->size) {
+ printf("<short data %u/%u>\n", size, attr->size);
+ return;
+ }
+
+ list_last = false;
+ while (size >= elem_size) {
+ printf("\t[%02x] ", ix);
+ if (size == elem_size)
+ list_last = true;
+ attr->dump(reply, size);
+ reply += elem_size;
+ size -= elem_size;
+ ix++;
+ }
+}
+
+/*
+ * Call fsinfo, expanding the buffer as necessary.
+ */
+static ssize_t get_fsinfo(const char *file, const char *name,
+ struct fsinfo_params *params, void **_r)
+{
+ ssize_t ret;
+ size_t buf_size = 4096;
+ void *r;
+
+ for (;;) {
+ r = malloc(buf_size);
+ if (!r) {
+ perror("malloc");
+ exit(1);
+ }
+ memset(r, 0xbd, buf_size);
+
+ errno = 0;
+ ret = fsinfo(AT_FDCWD, file, params, sizeof(*params), r, buf_size - 1);
+ if (ret == -1)
+ goto error;
+
+ if (ret <= buf_size - 1)
+ break;
+ buf_size = (ret + 4096 - 1) & ~(4096 - 1);
+ }
+
+ if (debug)
+ printf("fsinfo(%s,%s,%u,%u) = %zd\n",
+ file, name, params->Nth, params->Mth, ret);
+
+ ((char *)r)[ret] = 0;
+ *_r = r;
+ return ret;
+
+error:
+ *_r = NULL;
+ free(r);
+ if (debug)
+ printf("fsinfo(%s,%s,%u,%u) = %m\n",
+ file, name, params->Nth, params->Mth);
+ return ret;
+}
+
+/*
+ * Try one subinstance of an attribute.
+ */
+static int try_one(const char *file, struct fsinfo_params *params,
+ const struct fsinfo_attribute_info *attr_info, bool raw)
+{
+ const struct fsinfo_attribute *attr;
+ const char *name;
+ size_t size = 4096;
+ char namebuf[32];
+ void *r;
+
+ for (attr = fsinfo_attributes; attr->name; attr++) {
+ if (attr->attr_id == params->request) {
+ name = attr->name;
+ if (strncmp(name, "fsinfo_generic_", 15) == 0)
+ name += 15;
+ goto found;
+ }
+ }
+
+ sprintf(namebuf, "<unknown-%x>", params->request);
+ name = namebuf;
+ attr = NULL;
+
+found:
+ size = get_fsinfo(file, name, params, &r);
+
+ if (size == -1) {
+ if (errno == ENODATA) {
+ if (!(attr_info->flags & (FSINFO_FLAGS_N | FSINFO_FLAGS_NM)) &&
+ params->Nth == 0 && params->Mth == 0) {
+ fprintf(stderr,
+ "Unexpected ENODATA (0x%x{%u}{%u})\n",
+ params->request, params->Nth, params->Mth);
+ exit(1);
+ }
+ free(r);
+ return (params->Mth == 0) ? 2 : 1;
+ }
+ if (errno == EOPNOTSUPP) {
+ if (params->Nth > 0 || params->Mth > 0) {
+ fprintf(stderr,
+ "Should return -ENODATA (0x%x{%u}{%u})\n",
+ params->request, params->Nth, params->Mth);
+ exit(1);
+ }
+ //printf("\e[33m%s\e[m: <not supported>\n",
+ // fsinfo_attr_names[attr]);
+ free(r);
+ return 2;
+ }
+ perror(file);
+ exit(1);
+ }
+
+ if (raw) {
+ if (size > 4096)
+ size = 4096;
+ dump_hex(r, 0, size);
+ free(r);
+ return 0;
+ }
+
+ switch (attr_info->flags & (FSINFO_FLAGS_N | FSINFO_FLAGS_NM)) {
+ case 0:
+ printf("\e[33m%s\e[m: ", name);
+ break;
+ case FSINFO_FLAGS_N:
+ printf("\e[33m%s{%u}\e[m: ", name, params->Nth);
+ break;
+ case FSINFO_FLAGS_NM:
+ printf("\e[33m%s{%u,%u}\e[m: ", name, params->Nth, params->Mth);
+ break;
+ }
+
+ switch (attr_info->type) {
+ case FSINFO_TYPE_VSTRUCT:
+ case FSINFO_TYPE_STRING:
+ dump_value(params->request, attr, attr_info, r, size);
+ free(r);
+ return 0;
+
+ case FSINFO_TYPE_LIST:
+ dump_list(params->request, attr, attr_info, r, size);
+ free(r);
+ return 0;
+
+ case FSINFO_TYPE_OPAQUE:
+ free(r);
+ return 0;
+
+ default:
+ fprintf(stderr, "Fishy about %u 0x%x,%x,%x\n",
+ params->request, attr_info->type, attr_info->flags, attr_info->size);
+ exit(1);
+ }
+}
+
+static int cmp_u32(const void *a, const void *b)
+{
+ return *(const int *)a - *(const int *)b;
+}
+
+/*
+ *
+ */
+int main(int argc, char **argv)
+{
+ struct fsinfo_attribute_info attr_info;
+ struct fsinfo_params params = {
+ .resolve_flags = RESOLVE_NO_TRAILING_SYMLINKS,
+ .flags = FSINFO_FLAGS_QUERY_PATH,
+ };
+ unsigned int *attrs, ret, nr, i;
+ bool meta = false;
+ int raw = 0, opt, Nth, Mth;
+
+ while ((opt = getopt(argc, argv, "Madlr"))) {
+ switch (opt) {
+ case 'M':
+ meta = true;
+ continue;
+ case 'a':
+ params.resolve_flags |= RESOLVE_NO_TRAILING_AUTOMOUNTS;
+ params.flags = FSINFO_FLAGS_QUERY_PATH;
+ continue;
+ case 'd':
+ debug = true;
+ continue;
+ case 'l':
+ params.resolve_flags &= ~RESOLVE_NO_TRAILING_SYMLINKS;
+ params.flags = FSINFO_FLAGS_QUERY_PATH;
+ continue;
+ case 'r':
+ raw = 1;
+ continue;
+ }
+ break;
+ }
+
+ argc -= optind;
+ argv += optind;
+
+ if (argc != 1) {
+ printf("Format: test-fsinfo [-Madlr] <path>\n");
+ exit(2);
+ }
+
+ /* Retrieve a list of supported attribute IDs */
+ params.request = FSINFO_ATTR_FSINFO_ATTRIBUTES;
+ params.Nth = 0;
+ params.Mth = 0;
+ ret = get_fsinfo(argv[0], "attributes", ¶ms, (void **)&attrs);
+ if (ret == -1) {
+ fprintf(stderr, "Unable to get attribute list: %m\n");
+ exit(1);
+ }
+
+ if (ret % sizeof(attrs[0])) {
+ fprintf(stderr, "Bad length of attribute list (0x%x)\n", ret);
+ exit(2);
+ }
+
+ nr = ret / sizeof(attrs[0]);
+ qsort(attrs, nr, sizeof(attrs[0]), cmp_u32);
+
+ if (meta) {
+ printf("ATTR ID TYPE FLAGS SIZE NAME\n");
+ printf("======== ============ ======== ===== =========\n");
+ for (i = 0; i < nr; i++) {
+ params.request = FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO;
+ params.Nth = attrs[i];
+ params.Mth = 0;
+ ret = fsinfo(AT_FDCWD, argv[0],
+ ¶ms, sizeof(params),
+ &attr_info, sizeof(attr_info));
+ if (ret == -1) {
+ fprintf(stderr, "Can't get info for attribute %x: %m\n", attrs[i]);
+ exit(1);
+ }
+
+ dump_attribute_info(&attr_info, ret);
+ }
+ exit(0);
+ }
+
+ for (i = 0; i < nr; i++) {
+ params.request = FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO;
+ params.Nth = attrs[i];
+ params.Mth = 0;
+ ret = fsinfo(AT_FDCWD, argv[0],
+ ¶ms, sizeof(params),
+ &attr_info, sizeof(attr_info));
+ if (ret == -1) {
+ fprintf(stderr, "Can't get info for attribute %x: %m\n", attrs[i]);
+ exit(1);
+ }
+
+ if (attrs[i] == FSINFO_ATTR_FSINFO_ATTRIBUTE_INFO ||
+ attrs[i] == FSINFO_ATTR_FSINFO_ATTRIBUTES)
+ continue;
+
+ if (attrs[i] != attr_info.attr_id) {
+ fprintf(stderr, "ID for %03x returned %03x\n",
+ attrs[i], attr_info.attr_id);
+ break;
+ }
+ Nth = 0;
+ do {
+ Mth = 0;
+ do {
+ params.request = attrs[i];
+ params.Nth = Nth;
+ params.Mth = Mth;
+
+ switch (try_one(argv[0], ¶ms, &attr_info, raw)) {
+ case 0:
+ continue;
+ case 1:
+ goto done_M;
+ case 2:
+ goto done_N;
+ }
+ } while (++Mth < 100);
+
+ done_M:
+ if (Mth >= 100) {
+ fprintf(stderr, "Fishy: Mth %x[%u][%u]\n", attrs[i], Nth, Mth);
+ break;
+ }
+
+ } while (++Nth < 100);
+
+ done_N:
+ if (Nth >= 100) {
+ fprintf(stderr, "Fishy: Nth %x[%u]\n", attrs[i], Nth);
+ break;
+ }
+ }
+
+ return 0;
+}
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-10 15:45 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <416690.1583771540@warthog.procyon.org.uk>
On 3/9/20 12:32 PM, David Howells wrote:
> Waiman Long <longman@redhat.com> wrote:
>
>> + tmpbuf = kmalloc(tbuflen, GFP_KERNEL);
> This would probably be better off using kvmalloc() - otherwise big objects
> have to be constructed from runs of contiguous pages. But since all we're
> doing is buffering for userspace, we don't care about that.
>
> If you agree, we can address it with an additional patch.
>
> David
That is certainly fine with me. I don't care if the pages are contiguous
or not. Will add a patch 3 for that as suggested.
Thanks,
Longman
^ permalink raw reply
* Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-10 15:58 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <a4c92057-c364-965c-a251-02cbe46229b6@redhat.com>
On 3/10/20 11:45 AM, Waiman Long wrote:
> On 3/9/20 12:32 PM, David Howells wrote:
>> Waiman Long <longman@redhat.com> wrote:
>>
>>> + tmpbuf = kmalloc(tbuflen, GFP_KERNEL);
>> This would probably be better off using kvmalloc() - otherwise big objects
>> have to be constructed from runs of contiguous pages. But since all we're
>> doing is buffering for userspace, we don't care about that.
>>
>> If you agree, we can address it with an additional patch.
>>
>> David
> That is certainly fine with me. I don't care if the pages are contiguous
> or not. Will add a patch 3 for that as suggested.
That is not as simple as I thought. First of that, there is not an
equivalent kzvfree() helper to clear the buffer first before clearing.
Of course, I can do that manually.
With patch 2, the allocated buffer length will be max(1024, keylen). The
security code uses kmalloc() for allocation. If we use kvalloc() here,
perhaps we should also use that for allocation that can be potentially
large like that in big_key. What do you think?
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: David Howells @ 2020-03-10 17:12 UTC (permalink / raw)
To: Waiman Long
Cc: dhowells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
Mimi Zohar, keyrings, linux-kernel, linux-security-module,
linux-integrity, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
Eric Biggers, Chris von Recklinghausen
In-Reply-To: <da226448-4b76-0456-4c29-742a1a24fe79@redhat.com>
Waiman Long <longman@redhat.com> wrote:
> That is not as simple as I thought. First of that, there is not an
> equivalent kzvfree() helper to clear the buffer first before clearing.
> Of course, I can do that manually.
Yeah, the actual substance of vfree() may get deferred. It may be worth
adding a kvzfree() that switches between kzfree() and memset(),vfree().
> With patch 2, the allocated buffer length will be max(1024, keylen). The
> security code uses kmalloc() for allocation. If we use kvalloc() here,
> perhaps we should also use that for allocation that can be potentially
> large like that in big_key. What do you think?
Not for big_key: if it's larger than BIG_KEY_FILE_THRESHOLD (~1KiB) it gets
written encrypted into shmem so that it can be swapped out to disk when not in
use.
However, other cases, sure - just be aware that on a 32-bit system,
vmalloc/vmap space is a strictly limited resource.
David
^ permalink raw reply
* Re: [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes
From: Daniel Colascione @ 2020-03-10 18:09 UTC (permalink / raw)
To: Casey Schaufler, Sandeep Patil
Cc: Paul Moore, LSM List, Linux FS Devel, Al Viro, SElinux list, kvm,
Nick Kralevich, Stephen Smalley
In-Reply-To: <CAKOZueuus6fVqrKsfNgSYGo-kXJ3f6Mv_NJZStY1Uo934=SjDw@mail.gmail.com>
On Thu, Feb 20, 2020 at 10:50 AM Daniel Colascione <dancol@google.com> wrote:
>
> On Thu, Feb 20, 2020 at 10:11 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> > On 2/17/2020 4:14 PM, Paul Moore wrote:
> > > On Thu, Feb 13, 2020 at 2:41 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > >> Add support for labeling and controlling access to files attached to anon
> > >> inodes. Introduce extended interfaces for creating such files to permit
> > >> passing a related file as an input to decide how to label the anon
> > >> inode. Define a security hook for initializing the anon inode security
> > >> attributes. Security attributes are either inherited from a related file
> > >> or determined based on some combination of the creating task and policy
> > >> (in the case of SELinux, using type_transition rules). As an
> > >> example user of the inheritance support, convert kvm to use the new
> > >> interface for passing the related file so that the anon inode can inherit
> > >> the security attributes of /dev/kvm and provide consistent access control
> > >> for subsequent ioctl operations. Other users of anon inodes, including
> > >> userfaultfd, will default to the transition-based mechanism instead.
> > >>
> > >> Compared to the series in
> > >> https://lore.kernel.org/selinux/20200211225547.235083-1-dancol@google.com/,
> > >> this approach differs in that it does not require creation of a separate
> > >> anonymous inode for each file (instead storing the per-instance security
> > >> information in the file security blob), it applies labeling and control
> > >> to all users of anonymous inodes rather than requiring opt-in via a new
> > >> flag, it supports labeling based on a related inode if provided,
> > >> it relies on type transitions to compute the label of the anon inode
> > >> when there is no related inode, and it does not require introducing a new
> > >> security class for each user of anonymous inodes.
> > >>
> > >> On the other hand, the approach in this patch does expose the name passed
> > >> by the creator of the anon inode to the policy (an indirect mapping could
> > >> be provided within SELinux if these names aren't considered to be stable),
> > >> requires the definition of type_transition rules to distinguish userfaultfd
> > >> inodes from proc inodes based on type since they share the same class,
> > >> doesn't support denying the creation of anonymous inodes (making the hook
> > >> added by this patch return something other than void is problematic due to
> > >> it being called after the file is already allocated and error handling in
> > >> the callers can't presently account for this scenario and end up calling
> > >> release methods multiple times), and may be more expensive
> > >> (security_transition_sid overhead on each anon inode allocation).
> > >>
> > >> We are primarily posting this RFC patch now so that the two different
> > >> approaches can be concretely compared. We anticipate a hybrid of the
> > >> two approaches being the likely outcome in the end. In particular
> > >> if support for allocating a separate inode for each of these files
> > >> is acceptable, then we would favor storing the security information
> > >> in the inode security blob and using it instead of the file security
> > >> blob.
> > > Bringing this back up in hopes of attracting some attention from the
> > > fs-devel crowd and Al. As Stephen already mentioned, from a SELinux
> > > perspective we would prefer to attach the security blob to the inode
> > > as opposed to the file struct; does anyone have any objections to
> > > that?
> >
> > Sorry for the delay - been sick the past few days.
> >
> > I agree that the inode is a better place than the file for information
> > about the inode. This is especially true for Smack, which uses
> > multiple extended attributes in some cases. I don't believe that any
> > except the access label will be relevant to anonymous inodes, but
> > I can imagine security modules with policies that would.
> >
> > I am always an advocate of full xattr support. It goes a long
> > way in reducing the number and complexity of special case interfaces.
>
> It sounds like we have broad consensus on using the inode to hold
> security information, implying that anon_inodes should create new
> inodes. Do any of the VFS people want to object?
Ping?
^ permalink raw reply
* Re: [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes
From: Stephen Smalley @ 2020-03-10 18:26 UTC (permalink / raw)
To: Daniel Colascione
Cc: Casey Schaufler, Sandeep Patil, Paul Moore, LSM List,
Linux FS Devel, Al Viro, SElinux list, kvm, Nick Kralevich,
Stephen Smalley
In-Reply-To: <CAKOZuetUvu=maOmHXjCqkHaYEN5Sf+pKBc3BZ+qpy1tE1NJ9xQ@mail.gmail.com>
On Tue, Mar 10, 2020 at 2:11 PM Daniel Colascione <dancol@google.com> wrote:
>
> On Thu, Feb 20, 2020 at 10:50 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Thu, Feb 20, 2020 at 10:11 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > >
> > > On 2/17/2020 4:14 PM, Paul Moore wrote:
> > > > On Thu, Feb 13, 2020 at 2:41 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > >> Add support for labeling and controlling access to files attached to anon
> > > >> inodes. Introduce extended interfaces for creating such files to permit
> > > >> passing a related file as an input to decide how to label the anon
> > > >> inode. Define a security hook for initializing the anon inode security
> > > >> attributes. Security attributes are either inherited from a related file
> > > >> or determined based on some combination of the creating task and policy
> > > >> (in the case of SELinux, using type_transition rules). As an
> > > >> example user of the inheritance support, convert kvm to use the new
> > > >> interface for passing the related file so that the anon inode can inherit
> > > >> the security attributes of /dev/kvm and provide consistent access control
> > > >> for subsequent ioctl operations. Other users of anon inodes, including
> > > >> userfaultfd, will default to the transition-based mechanism instead.
> > > >>
> > > >> Compared to the series in
> > > >> https://lore.kernel.org/selinux/20200211225547.235083-1-dancol@google.com/,
> > > >> this approach differs in that it does not require creation of a separate
> > > >> anonymous inode for each file (instead storing the per-instance security
> > > >> information in the file security blob), it applies labeling and control
> > > >> to all users of anonymous inodes rather than requiring opt-in via a new
> > > >> flag, it supports labeling based on a related inode if provided,
> > > >> it relies on type transitions to compute the label of the anon inode
> > > >> when there is no related inode, and it does not require introducing a new
> > > >> security class for each user of anonymous inodes.
> > > >>
> > > >> On the other hand, the approach in this patch does expose the name passed
> > > >> by the creator of the anon inode to the policy (an indirect mapping could
> > > >> be provided within SELinux if these names aren't considered to be stable),
> > > >> requires the definition of type_transition rules to distinguish userfaultfd
> > > >> inodes from proc inodes based on type since they share the same class,
> > > >> doesn't support denying the creation of anonymous inodes (making the hook
> > > >> added by this patch return something other than void is problematic due to
> > > >> it being called after the file is already allocated and error handling in
> > > >> the callers can't presently account for this scenario and end up calling
> > > >> release methods multiple times), and may be more expensive
> > > >> (security_transition_sid overhead on each anon inode allocation).
> > > >>
> > > >> We are primarily posting this RFC patch now so that the two different
> > > >> approaches can be concretely compared. We anticipate a hybrid of the
> > > >> two approaches being the likely outcome in the end. In particular
> > > >> if support for allocating a separate inode for each of these files
> > > >> is acceptable, then we would favor storing the security information
> > > >> in the inode security blob and using it instead of the file security
> > > >> blob.
> > > > Bringing this back up in hopes of attracting some attention from the
> > > > fs-devel crowd and Al. As Stephen already mentioned, from a SELinux
> > > > perspective we would prefer to attach the security blob to the inode
> > > > as opposed to the file struct; does anyone have any objections to
> > > > that?
> > >
> > > Sorry for the delay - been sick the past few days.
> > >
> > > I agree that the inode is a better place than the file for information
> > > about the inode. This is especially true for Smack, which uses
> > > multiple extended attributes in some cases. I don't believe that any
> > > except the access label will be relevant to anonymous inodes, but
> > > I can imagine security modules with policies that would.
> > >
> > > I am always an advocate of full xattr support. It goes a long
> > > way in reducing the number and complexity of special case interfaces.
> >
> > It sounds like we have broad consensus on using the inode to hold
> > security information, implying that anon_inodes should create new
> > inodes. Do any of the VFS people want to object?
>
> Ping?
I'd recommend refreshing your patch series to incorporate feedback on
the previous version and re-post,
including viro and linux-fsdevel on the cc, and see if they have any
comments on it.
^ permalink raw reply
* Re: [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes
From: Daniel Colascione @ 2020-03-10 21:50 UTC (permalink / raw)
To: Stephen Smalley
Cc: Casey Schaufler, Sandeep Patil, Paul Moore, LSM List,
Linux FS Devel, Al Viro, SElinux list, kvm, Nick Kralevich,
Stephen Smalley
In-Reply-To: <CAEjxPJ4+NM6-tfOeZ6UQfas6=KxtBTAk6f23GEyLomFn3K3qew@mail.gmail.com>
On Tue, Mar 10, 2020 at 11:25 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Mar 10, 2020 at 2:11 PM Daniel Colascione <dancol@google.com> wrote:
> >
> > On Thu, Feb 20, 2020 at 10:50 AM Daniel Colascione <dancol@google.com> wrote:
> > >
> > > On Thu, Feb 20, 2020 at 10:11 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > >
> > > > On 2/17/2020 4:14 PM, Paul Moore wrote:
> > > > > On Thu, Feb 13, 2020 at 2:41 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > > >> Add support for labeling and controlling access to files attached to anon
> > > > >> inodes. Introduce extended interfaces for creating such files to permit
> > > > >> passing a related file as an input to decide how to label the anon
> > > > >> inode. Define a security hook for initializing the anon inode security
> > > > >> attributes. Security attributes are either inherited from a related file
> > > > >> or determined based on some combination of the creating task and policy
> > > > >> (in the case of SELinux, using type_transition rules). As an
> > > > >> example user of the inheritance support, convert kvm to use the new
> > > > >> interface for passing the related file so that the anon inode can inherit
> > > > >> the security attributes of /dev/kvm and provide consistent access control
> > > > >> for subsequent ioctl operations. Other users of anon inodes, including
> > > > >> userfaultfd, will default to the transition-based mechanism instead.
> > > > >>
> > > > >> Compared to the series in
> > > > >> https://lore.kernel.org/selinux/20200211225547.235083-1-dancol@google.com/,
> > > > >> this approach differs in that it does not require creation of a separate
> > > > >> anonymous inode for each file (instead storing the per-instance security
> > > > >> information in the file security blob), it applies labeling and control
> > > > >> to all users of anonymous inodes rather than requiring opt-in via a new
> > > > >> flag, it supports labeling based on a related inode if provided,
> > > > >> it relies on type transitions to compute the label of the anon inode
> > > > >> when there is no related inode, and it does not require introducing a new
> > > > >> security class for each user of anonymous inodes.
> > > > >>
> > > > >> On the other hand, the approach in this patch does expose the name passed
> > > > >> by the creator of the anon inode to the policy (an indirect mapping could
> > > > >> be provided within SELinux if these names aren't considered to be stable),
> > > > >> requires the definition of type_transition rules to distinguish userfaultfd
> > > > >> inodes from proc inodes based on type since they share the same class,
> > > > >> doesn't support denying the creation of anonymous inodes (making the hook
> > > > >> added by this patch return something other than void is problematic due to
> > > > >> it being called after the file is already allocated and error handling in
> > > > >> the callers can't presently account for this scenario and end up calling
> > > > >> release methods multiple times), and may be more expensive
> > > > >> (security_transition_sid overhead on each anon inode allocation).
> > > > >>
> > > > >> We are primarily posting this RFC patch now so that the two different
> > > > >> approaches can be concretely compared. We anticipate a hybrid of the
> > > > >> two approaches being the likely outcome in the end. In particular
> > > > >> if support for allocating a separate inode for each of these files
> > > > >> is acceptable, then we would favor storing the security information
> > > > >> in the inode security blob and using it instead of the file security
> > > > >> blob.
> > > > > Bringing this back up in hopes of attracting some attention from the
> > > > > fs-devel crowd and Al. As Stephen already mentioned, from a SELinux
> > > > > perspective we would prefer to attach the security blob to the inode
> > > > > as opposed to the file struct; does anyone have any objections to
> > > > > that?
> > > >
> > > > Sorry for the delay - been sick the past few days.
> > > >
> > > > I agree that the inode is a better place than the file for information
> > > > about the inode. This is especially true for Smack, which uses
> > > > multiple extended attributes in some cases. I don't believe that any
> > > > except the access label will be relevant to anonymous inodes, but
> > > > I can imagine security modules with policies that would.
> > > >
> > > > I am always an advocate of full xattr support. It goes a long
> > > > way in reducing the number and complexity of special case interfaces.
> > >
> > > It sounds like we have broad consensus on using the inode to hold
> > > security information, implying that anon_inodes should create new
> > > inodes. Do any of the VFS people want to object?
> >
> > Ping?
>
> I'd recommend refreshing your patch series to incorporate feedback on
> the previous version and re-post,
> including viro and linux-fsdevel on the cc, and see if they have any
> comments on it.
I don't think there's anything in the patch series that needs to
change right now. AFAICT, we're still just waiting on comment from the
VFS people, who should be on this thread. Did I miss something?
^ permalink raw reply
* [PATCH] tomoyo: Use scnprintf() for avoiding potential buffer overflow
From: Takashi Iwai @ 2020-03-11 9:36 UTC (permalink / raw)
To: Kentaro Takeda, Tetsuo Handa; +Cc: linux-security-module
Since snprintf() returns the would-be-output size instead of the
actual output size, the succeeding calls may go beyond the given
buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
security/tomoyo/audit.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/security/tomoyo/audit.c b/security/tomoyo/audit.c
index 3c96e8402e94..aedc93959067 100644
--- a/security/tomoyo/audit.c
+++ b/security/tomoyo/audit.c
@@ -162,7 +162,7 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
tomoyo_convert_time(ktime_get_real_seconds(), &stamp);
- pos = snprintf(buffer, tomoyo_buffer_len - 1,
+ pos = scnprintf(buffer, tomoyo_buffer_len - 1,
"#%04u/%02u/%02u %02u:%02u:%02u# profile=%u mode=%s granted=%s (global-pid=%u) task={ pid=%u ppid=%u uid=%u gid=%u euid=%u egid=%u suid=%u sgid=%u fsuid=%u fsgid=%u }",
stamp.year, stamp.month, stamp.day, stamp.hour,
stamp.min, stamp.sec, r->profile, tomoyo_mode[r->mode],
@@ -193,7 +193,7 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
dev = stat->dev;
mode = stat->mode;
if (i & 1) {
- pos += snprintf(buffer + pos,
+ pos += scnprintf(buffer + pos,
tomoyo_buffer_len - 1 - pos,
" path%u.parent={ uid=%u gid=%u ino=%lu perm=0%o }",
(i >> 1) + 1,
@@ -203,7 +203,7 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
stat->mode & S_IALLUGO);
continue;
}
- pos += snprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
+ pos += scnprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
" path%u={ uid=%u gid=%u ino=%lu major=%u minor=%u perm=0%o type=%s",
(i >> 1) + 1,
from_kuid(&init_user_ns, stat->uid),
@@ -213,12 +213,12 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
mode & S_IALLUGO, tomoyo_filetype(mode));
if (S_ISCHR(mode) || S_ISBLK(mode)) {
dev = stat->rdev;
- pos += snprintf(buffer + pos,
+ pos += scnprintf(buffer + pos,
tomoyo_buffer_len - 1 - pos,
" dev_major=%u dev_minor=%u",
MAJOR(dev), MINOR(dev));
}
- pos += snprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
+ pos += scnprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
" }");
}
no_obj_info:
@@ -276,17 +276,17 @@ char *tomoyo_init_log(struct tomoyo_request_info *r, int len, const char *fmt,
if (!buf)
goto out;
len--;
- pos = snprintf(buf, len, "%s", header);
+ pos = scnprintf(buf, len, "%s", header);
if (realpath) {
struct linux_binprm *bprm = r->ee->bprm;
- pos += snprintf(buf + pos, len - pos,
+ pos += scnprintf(buf + pos, len - pos,
" exec={ realpath=\"%s\" argc=%d envc=%d %s }",
realpath, bprm->argc, bprm->envc, bprm_info);
} else if (symlink)
- pos += snprintf(buf + pos, len - pos, " symlink.target=\"%s\"",
+ pos += scnprintf(buf + pos, len - pos, " symlink.target=\"%s\"",
symlink);
- pos += snprintf(buf + pos, len - pos, "\n%s\n", domainname);
+ pos += scnprintf(buf + pos, len - pos, "\n%s\n", domainname);
vsnprintf(buf + pos, len - pos, fmt, args);
out:
kfree(realpath);
--
2.16.4
^ permalink raw reply related
* Re: [PATCH] tomoyo: Use scnprintf() for avoiding potential buffer overflow
From: Tetsuo Handa @ 2020-03-11 10:20 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Kentaro Takeda, linux-security-module
In-Reply-To: <20200311093627.25409-1-tiwai@suse.de>
On 2020/03/11 18:36, Takashi Iwai wrote:
> Since snprintf() returns the would-be-output size instead of the
> actual output size, the succeeding calls may go beyond the given
> buffer limit. Fix it by replacing with scnprintf().
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> security/tomoyo/audit.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
Thanks for a patch. But current code will simply hit WARN_ON_ONCE() in vsnprintf()
if the would-be-output size went beyond the given buffer limit, and we have never
hit that warning from this function. That is, the buffer limit is large enough,
and the last byte is guaranteed to be '\0'.
^ permalink raw reply
* Re: [PATCH] tomoyo: Use scnprintf() for avoiding potential buffer overflow
From: Takashi Iwai @ 2020-03-11 11:06 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Kentaro Takeda, linux-security-module
In-Reply-To: <fbe60125-091d-c536-36a0-5cfab2eff707@i-love.sakura.ne.jp>
On Wed, 11 Mar 2020 11:20:44 +0100,
Tetsuo Handa wrote:
>
> On 2020/03/11 18:36, Takashi Iwai wrote:
> > Since snprintf() returns the would-be-output size instead of the
> > actual output size, the succeeding calls may go beyond the given
> > buffer limit. Fix it by replacing with scnprintf().
> >
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > security/tomoyo/audit.c | 18 +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
>
> Thanks for a patch. But current code will simply hit WARN_ON_ONCE() in vsnprintf()
> if the would-be-output size went beyond the given buffer limit, and we have never
> hit that warning from this function. That is, the buffer limit is large enough,
> and the last byte is guaranteed to be '\0'.
Right, I don't think this actually hitting the overflow, either.
If the code is intended to rely on the sanity check in vsnprintf(),
it's fine. But I find it a bit fragile and would prefer more explicit
check in the caller side instead.
thanks,
Takashi
^ permalink raw reply
* Re: [RFC PATCH] security,anon_inodes,kvm: enable security support for anon inodes
From: Stephen Smalley @ 2020-03-11 13:31 UTC (permalink / raw)
To: Daniel Colascione
Cc: Casey Schaufler, Sandeep Patil, Paul Moore, LSM List,
Linux FS Devel, Al Viro, SElinux list, kvm, Nick Kralevich,
Stephen Smalley
In-Reply-To: <CAKOZuevcz+fvfhRXPx2iZGtkk6+FjVj3ZSaGGT8DfwsOJR0k3A@mail.gmail.com>
On Tue, Mar 10, 2020 at 5:51 PM Daniel Colascione <dancol@google.com> wrote:
>
> On Tue, Mar 10, 2020 at 11:25 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Tue, Mar 10, 2020 at 2:11 PM Daniel Colascione <dancol@google.com> wrote:
> > >
> > > On Thu, Feb 20, 2020 at 10:50 AM Daniel Colascione <dancol@google.com> wrote:
> > > >
> > > > On Thu, Feb 20, 2020 at 10:11 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >
> > > > > On 2/17/2020 4:14 PM, Paul Moore wrote:
> > > > > > On Thu, Feb 13, 2020 at 2:41 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > > > >> We are primarily posting this RFC patch now so that the two different
> > > > > >> approaches can be concretely compared. We anticipate a hybrid of the
> > > > > >> two approaches being the likely outcome in the end. In particular
> > > > > >> if support for allocating a separate inode for each of these files
> > > > > >> is acceptable, then we would favor storing the security information
> > > > > >> in the inode security blob and using it instead of the file security
> > > > > >> blob.
> > > > > > Bringing this back up in hopes of attracting some attention from the
> > > > > > fs-devel crowd and Al. As Stephen already mentioned, from a SELinux
> > > > > > perspective we would prefer to attach the security blob to the inode
> > > > > > as opposed to the file struct; does anyone have any objections to
> > > > > > that?
> > > > >
> > > > > Sorry for the delay - been sick the past few days.
> > > > >
> > > > > I agree that the inode is a better place than the file for information
> > > > > about the inode. This is especially true for Smack, which uses
> > > > > multiple extended attributes in some cases. I don't believe that any
> > > > > except the access label will be relevant to anonymous inodes, but
> > > > > I can imagine security modules with policies that would.
> > > > >
> > > > > I am always an advocate of full xattr support. It goes a long
> > > > > way in reducing the number and complexity of special case interfaces.
> > > >
> > > > It sounds like we have broad consensus on using the inode to hold
> > > > security information, implying that anon_inodes should create new
> > > > inodes. Do any of the VFS people want to object?
> > >
> > > Ping?
> >
> > I'd recommend refreshing your patch series to incorporate feedback on
> > the previous version and re-post,
> > including viro and linux-fsdevel on the cc, and see if they have any
> > comments on it.
>
> I don't think there's anything in the patch series that needs to
> change right now. AFAICT, we're still just waiting on comment from the
> VFS people, who should be on this thread. Did I miss something?
There was some discussion on the SELinux bits in patch 2/3. I would
take the silence on
the vfs bits as implicit acceptance until you hear otherwise and just
submit a v2 that addresses
the SELinux bits.
^ permalink raw reply
* Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read
From: Waiman Long @ 2020-03-11 15:33 UTC (permalink / raw)
To: David Howells
Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, Mimi Zohar,
keyrings, linux-kernel, linux-security-module, linux-integrity,
Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
Chris von Recklinghausen
In-Reply-To: <675400.1583860343@warthog.procyon.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]
On 3/10/20 1:12 PM, David Howells wrote:
> Waiman Long <longman@redhat.com> wrote:
>
>> That is not as simple as I thought. First of that, there is not an
>> equivalent kzvfree() helper to clear the buffer first before clearing.
>> Of course, I can do that manually.
> Yeah, the actual substance of vfree() may get deferred. It may be worth
> adding a kvzfree() that switches between kzfree() and memset(),vfree().
>
>> With patch 2, the allocated buffer length will be max(1024, keylen). The
>> security code uses kmalloc() for allocation. If we use kvalloc() here,
>> perhaps we should also use that for allocation that can be potentially
>> large like that in big_key. What do you think?
> Not for big_key: if it's larger than BIG_KEY_FILE_THRESHOLD (~1KiB) it gets
> written encrypted into shmem so that it can be swapped out to disk when not in
> use.
>
> However, other cases, sure - just be aware that on a 32-bit system,
> vmalloc/vmap space is a strictly limited resource.
Attached is an additional patch to make the transition from kmalloc() to
kvmalloc(). I put the __kvzfree() helper in internal.h for now. I plan
to send a patch later to add a kvzfree() API once there is a use case in
the kernel.
I am not going to touch other places for now to make thing simpler.
Cheers,
Longman
[-- Attachment #2: v2-0003-KEYS-Use-kvmalloc-to-better-handle-large-buffer-a.patch --]
[-- Type: text/x-patch, Size: 2955 bytes --]
>From e2e73e2bc0c5cd168de273b0fe9df1e5c48cd232 Mon Sep 17 00:00:00 2001
From: Waiman Long <longman@redhat.com>
Date: Wed, 11 Mar 2020 11:01:59 -0400
Subject: [PATCH v2 3/3] KEYS: Use kvmalloc() to better handle large buffer
allocation
For large multi-page temporary buffer allocation, the security/keys
subsystem don't need contiguous physical pages. It will work perfectly
fine with virtually mapped pages.
Replace the kmalloc() call by kvmalloc() and provide a __kvzfree()
helper function to clear and free the kvmalloc'ed buffer. This will
reduce the chance of memory allocation failure just because of highly
fragmented pages.
Suggested-by: David Howells <dhowells@redhat.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
security/keys/internal.h | 14 ++++++++++++++
security/keys/keyctl.c | 12 ++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/security/keys/internal.h b/security/keys/internal.h
index ba3e2da14cef..1b6e2d66e378 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -16,6 +16,8 @@
#include <linux/keyctl.h>
#include <linux/refcount.h>
#include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
struct iovec;
@@ -349,4 +351,16 @@ static inline void key_check(const struct key *key)
#endif
+/*
+ * Helper function to clear and free a kvmalloc'ed memory object.
+ */
+static inline void __kvzfree(const void *addr, size_t len)
+{
+ if (is_vmalloc_addr(addr)) {
+ memset((char *)addr, 0, len);
+ vfree(addr);
+ } else {
+ kzfree(addr);
+ }
+}
#endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 662a638a680d..ca05604bc9c0 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
payload = NULL;
if (plen) {
ret = -ENOMEM;
- payload = kmalloc(plen, GFP_KERNEL);
+ payload = kvmalloc(plen, GFP_KERNEL);
if (!payload)
goto error;
@@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
key_ref_put(key_ref);
error2:
- kzfree(payload);
+ __kvzfree(payload, plen);
error:
return ret;
}
@@ -870,7 +870,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
*/
if (buflen && buffer && (buflen <= 0x400)) {
allocbuf:
- tmpbuf = kmalloc(tbuflen, GFP_KERNEL);
+ tmpbuf = kvmalloc(tbuflen, GFP_KERNEL);
if (!tmpbuf) {
ret = -ENOMEM;
goto error2;
@@ -892,9 +892,9 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
* again.
*/
if (!tmpbuf || unlikely(ret > tbuflen)) {
- tbuflen = ret;
if (unlikely(tmpbuf))
- kzfree(tmpbuf);
+ __kvzfree(tmpbuf, tbuflen);
+ tbuflen = ret;
goto allocbuf;
}
@@ -903,7 +903,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
}
if (tmpbuf)
- kzfree(tmpbuf);
+ __kvzfree(tmpbuf, tbuflen);
}
error2:
--
2.18.1
^ permalink raw reply related
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: Linus Torvalds @ 2020-03-11 17:59 UTC (permalink / raw)
To: David Howells
Cc: Aleksa Sarai, Al Viro, Stefan Metzmacher, Ian Kent,
Miklos Szeredi, Christian Brauner, Jann Horn, Darrick J. Wong,
Karel Zak, jlayton, Linux API, linux-fsdevel, LSM List,
Linux Kernel Mailing List
In-Reply-To: <580352.1583825105@warthog.procyon.org.uk>
On Tue, Mar 10, 2020 at 12:25 AM David Howells <dhowells@redhat.com> wrote:
?
> Okay. So what's the equivalent of AT_SYMLINK_NOFOLLOW in RESOLVE_* flag
> terms?
Nothing.
openat2() takes two sets of flags. We'll never get rid of
AT_SYMLINK_NOFOLLOW / O_NOFOLLOW, and we've added RESOLVE_NO_SYMLINKS
to the new set of flags. It's just a separate namespace.
We will _not_ be adding a RESOLVE_XYZ flag for O_NOFOLLOW or
AT_SYMLINK_NOFOLLOW. At least not visible to user space - because as
people already figured out, that just causes problems with consistency
issues.
And yes, the fact that we then have three different user-visible
namespaces (O_xyz flags for open(), AT_xyz flags for linkat(), and now
RESOLVE_xyz flags for openat2()) is sad and messy. But it's an
inherent messiness from just how the world works. We can't get rid of
it.
If we need linkat2() and friends, so be it. Do we?
Could we have a _fourth_ set of flags that are simply for internal use
that is a superset of them all? Sure. But no, it's almost certainly
not worth it. Four is not better than three.
Now, some type-safety in the kernel to make sure that we can't mix
AT_xyz with O_xyz or RESOLVE_xyz - that might be worth it. Although
judging by past experience, not enough people run sparse for it to
really be worth it.
Linus
PS. Yeah, we also have that LOOKUP_xyz namespace, and the access mode
namespace, so we already have those internal format versions too.
^ permalink raw reply
* Re: [RFC PATCH v14 00/10] Landlock LSM
From: Mickaël Salaün @ 2020-03-11 23:38 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <CAG48ez21bEn0wL1bbmTiiu8j9jP5iEWtHOwz4tURUJ+ki0ydYw@mail.gmail.com>
On 10/03/2020 00:44, Jann Horn wrote:
> On Mon, Feb 24, 2020 at 5:03 PM Mickaël Salaün <mic@digikod.net> wrote:
>> This new version of Landlock is a major revamp of the previous series
>> [1], hence the RFC tag. The three main changes are the replacement of
>> eBPF with a dedicated safe management of access rules, the replacement
>> of the use of seccomp(2) with a dedicated syscall, and the management of
>> filesystem access-control (back from the v10).
>>
>> As discussed in [2], eBPF may be too powerful and dangerous to be put in
>> the hand of unprivileged and potentially malicious processes, especially
>> because of side-channel attacks against access-controls or other parts
>> of the kernel.
>>
>> Thanks to this new implementation (1540 SLOC), designed from the ground
>> to be used by unprivileged processes, this series enables a process to
>> sandbox itself without requiring CAP_SYS_ADMIN, but only the
>> no_new_privs constraint (like seccomp). Not relying on eBPF also
>> enables to improve performances, especially for stacked security
>> policies thanks to mergeable rulesets.
>>
>> The compiled documentation is available here:
>> https://landlock.io/linux-doc/landlock-v14/security/landlock/index.html
>>
>> This series can be applied on top of v5.6-rc3. This can be tested with
>> CONFIG_SECURITY_LANDLOCK and CONFIG_SAMPLE_LANDLOCK. This patch series
>> can be found in a Git repository here:
>> https://github.com/landlock-lsm/linux/commits/landlock-v14
>> I would really appreciate constructive comments on the design and the code.
>
> I've looked through the patchset, and I think that it would be
> possible to simplify it quite a bit. I have tried to do that (and
> compiled-tested it, but not actually tried running it); here's what I
> came up with:
>
> https://github.com/thejh/linux/commits/landlock-mod
>
> The three modified patches (patches 1, 2 and 5) are marked with
> "[MODIFIED]" in their title. Please take a look - what do you think?
> Feel free to integrate my changes into your patches if you think they
> make sense.
Regarding the landlock_release_inodes(), the final wait_var_event() is
indeed needed (as does fsnotify), but why do you use a READ_ONCE() for
landlock_initialized?
I was reluctant to use function pointers but landlock_object_operations
makes a cleaner and more generic interface to manage objects.
Your get_inode_object() is much simpler and easier to understand than
the get_object() and get_cleaner().
The other main change is about the object cross-reference: you entirely
removed it, which means that an object will only be free when there are
no rules using it. This does not free an object when its underlying
object is being terminated. We now only have to worry about the
termination of the parent of an underlying object (e.g. the super-block
of an inode).
However, I think you forgot to increment object->usage in
create_ruleset_elem(). There is also an unused checked_mask variable in
merge_ruleset().
All this removes optimizations that made the code more difficult to
understand. The performance difference is negligible, and I think that
the memory footprint is fine.
These optimizations (and others) could be discussed later. I'm
integrating most of your changes in the next patch series.
Thank you very much for this review and the code.
>
>
> Apart from simplifying the code, I also found the following issues,
> which I have fixed in the modified patches:
>
> put_hierarchy() has to drop a reference on its parent. (However, this
> must not recurse, so we have to do it with a loop.)
Right, fixed.
>
> put_ruleset() is not in an RCU read-side critical section, so as soon
> as it calls kfree_rcu(), "freeme" might disappear; but "orig" is in
> "freeme", so when the loop tries to find the next element with
> rb_next(orig), that can be a UAF.
> rbtree_postorder_for_each_entry_safe() exists for dealing with such
> issues.
Good catch.
>
> AFAIK the calls to rb_erase() in clean_ruleset() is not safe if
> someone is concurrently accessing the rbtree as an RCU reader, because
> concurrent rotations can prevent a lookup from succeeding. The
> simplest fix is probably to just make any rbtree that has been
> installed on a process immutable, and give up on the cleaning -
> arguably the memory wastage that can cause is pretty limited.
Yes, let's go for immutable domains.
> (By the
> way, as a future optimization, we might want to turn the rbtree into a
> hashtable when installing it?)
Definitely. This was a previous (private) implementation I did for
domains, but to simplify the code I reused the same type as a ruleset. A
future evolution of Landlock could add back this optimization.
>
> The iput() in landlock_release_inode() looks unsafe - you need to
> guarantee that even if the deletion of a ruleset races with
> generic_shutdown_super(), every iput() for that superblock finishes
> before landlock_release_inodes() returns, even if the iput() is
> happening in the context of ruleset deletion. This is why
> fsnotify_unmount_inodes() has that wait_var_event() at the end.
Right, much better with that.
>
>
> Aside from those things, there is also a major correctness issue where
> I'm not sure how to solve it properly:
>
> Let's say a process installs a filter on itself like this:
>
> struct landlock_attr_ruleset ruleset = { .handled_access_fs =
> ACCESS_FS_ROUGHLY_WRITE};
> int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
> LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
> struct landlock_attr_path_beneath path_beneath = {
> .ruleset_fd = ruleset_fd,
> .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
> .parent_fd = open("/tmp/foobar", O_PATH),
> };
> landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
> sizeof(path_beneath), &path_beneath);
> prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
> landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
> sizeof(attr_enforce), &attr_enforce);
>
> At this point, the process is not supposed to be able to write to
> anything outside /tmp/foobar, right? But what happens if the process
> does the following next?
>
> struct landlock_attr_ruleset ruleset = { .handled_access_fs =
> ACCESS_FS_ROUGHLY_WRITE};
> int ruleset_fd = landlock(LANDLOCK_CMD_CREATE_RULESET,
> LANDLOCK_OPT_CREATE_RULESET, sizeof(ruleset), &ruleset);
> struct landlock_attr_path_beneath path_beneath = {
> .ruleset_fd = ruleset_fd,
> .allowed_access = ACCESS_FS_ROUGHLY_WRITE,
> .parent_fd = open("/", O_PATH),
> };
> landlock(LANDLOCK_CMD_ADD_RULE, LANDLOCK_OPT_ADD_RULE_PATH_BENEATH,
> sizeof(path_beneath), &path_beneath);
> prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
> struct landlock_attr_enforce attr_enforce = { .ruleset_fd = ruleset_fd };
> landlock(LANDLOCK_CMD_ENFORCE_RULESET, LANDLOCK_OPT_ENFORCE_RULESET,
> sizeof(attr_enforce), &attr_enforce);
>
> As far as I can tell from looking at the source, after this, you will
> have write access to the entire filesystem again. I think the idea is
> that LANDLOCK_CMD_ENFORCE_RULESET should only let you drop privileges,
> not increase them, right?
There is an additionnal check in syscall.c:get_path_from_fd(): it is
forbidden to add a rule with a path which is not accessible (according
to LANDLOCK_ACCESS_FS_OPEN) thanks to a call to security_file_open(),
but this is definitely not perfect.
>
> I think the easy way to fix this would be to add a bitmask to each
> rule that says from which ruleset it originally comes, and then let
> check_access_path() collect these bitmasks from each rule with OR, and
> check at the end whether the resulting bitmask is full - if not, at
> least one of the rulesets did not permit the access, and it should be
> denied.
>
> But maybe it would make more sense to change how the API works
> instead, and get rid of the concept of "merging" two rulesets
> together? Instead, we could make the API work like this:
>
> - LANDLOCK_CMD_CREATE_RULESET gives you a file descriptor whose
> ->private_data contains a pointer to the old ruleset of the process,
> as well as a pointer to a new empty ruleset.
> - LANDLOCK_CMD_ADD_RULE fails if the specified rule would not be
> permitted by the old ruleset, then adds the rule to the new ruleset
> - LANDLOCK_CMD_ENFORCE_RULESET fails if the old ruleset pointer in
> ->private_data doesn't match the current ruleset of the process, then
> replaces the old ruleset with the new ruleset.
>
> With this, the new ruleset is guaranteed to be a subset of the old
> ruleset because each of the new ruleset's rules is permitted by the
> old ruleset. (Unless the directory hierarchy rotates, but in that case
> the inaccuracy isn't much worse than what would've been possible
> through RCU path walk anyway AFAIK.)
>
> What do you think?
>
I would prefer to add the same checks you described at first (with
check_access_path), but only when creating a new ruleset with
merge_ruleset() (which should probably be renamed). This enables not to
rely on a parent ruleset/domain until the enforcement, which is the case
anyway.
Unfortunately this doesn't work for some cases with bind mounts. Because
check_access_path() goes through one path, another (bind mounted) path
could be illegitimately allowed.
That makes the problem a bit more complicated. A solution may be to keep
track of the hierarchy of each rule (e.g. with a layer/depth number),
and only allow an access request if at least a rule of each layer allow
this access. In this case we also need to correctly handle the case when
rules from different layers are tied to the same object.
I would like Landlock to have "pure" syscalls, in the sense that a
process A (e.g. a daemon) could prepare a ruleset and sends its FD to a
process B which would then be able to use it to sandbox itself. I think
it makes the reasoning clearer not to have a given ruleset (FD) tied to
a domain (i.e. parent ruleset) at first.
Landlock should (as much as possible) return an error if a syscall
argument is invalid, not according to the current access control (which
is not the case currently because of the security_file_open() check).
This means that these additional merge_ruleset() checks should only
affect the new domain/ruleset, but it should not be visible to userspace.
In a future evolution, it may be useful to add a lock/seal command to
deny any additional rule enforcement. However that may be
counter-productive because that enable application developers (e.g. for
a shell) to deny the use of Landlock features to its child processes.
But it would be possible anyway with seccomp-bpf…
^ permalink raw reply
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: Stefan Metzmacher @ 2020-03-12 9:08 UTC (permalink / raw)
To: Linus Torvalds, David Howells
Cc: Aleksa Sarai, Al Viro, Ian Kent, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak, jlayton,
Linux API, linux-fsdevel, LSM List, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wiaL6zznNtCHKg6+MJuCqDxO=yVfms3qR9A0czjKuSSiA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3065 bytes --]
Hi Linus,
>> Okay. So what's the equivalent of AT_SYMLINK_NOFOLLOW in RESOLVE_* flag
>> terms?
>
> Nothing.
>
> openat2() takes two sets of flags. We'll never get rid of
> AT_SYMLINK_NOFOLLOW / O_NOFOLLOW, and we've added RESOLVE_NO_SYMLINKS
> to the new set of flags. It's just a separate namespace.
>
> We will _not_ be adding a RESOLVE_XYZ flag for O_NOFOLLOW or
> AT_SYMLINK_NOFOLLOW. At least not visible to user space - because as
> people already figured out, that just causes problems with consistency
> issues.
>
> And yes, the fact that we then have three different user-visible
> namespaces (O_xyz flags for open(), AT_xyz flags for linkat(), and now
> RESOLVE_xyz flags for openat2()) is sad and messy. But it's an
> inherent messiness from just how the world works. We can't get rid of
> it.
For openat2() and other existing syscalls I agree, that it's good to
have just a single bit to control a feature.
The whole discussion was triggered by the introduction of a completely
new fsinfo()
call:
>> The new system call looks like:
>>
>> int ret = fsinfo(int dfd,
>> const char *pathname,
>> const struct fsinfo_params *params,
>> size_t params_size,
>> void *result_buffer,
>> size_t result_buf_size);
>>
>> The params parameter optionally points to a block of parameters:
>>
>> struct fsinfo_params {
>> __u32 resolve_flags;
If I remember correctly with was named at_flags initially.
And I thought it would be great to also have the new RESOLVE_YXZ feature
available for that new path based syscall.
Would you propose to have 'at_flags' and 'resolve_flags' passed in here?
Or is there something even better you would propose for new syscalls?
>> __u32 flags;
>> __u32 request;
>> __u32 Nth;
>> __u32 Mth;
>> };
> If we need linkat2() and friends, so be it. Do we?
Yes, I'm going to propose something like this, as it would make the life
much easier for Samba to have the new features available on all path
based syscalls.
In addition I'll propose to have a way to specify the source of
removeat and unlinkat also by fd in addition to the the source parent fd
and relative path, the reason are also to detect races of path
recycling. pidfd_open() solved a similar problem for pid recycling.
> Could we have a _fourth_ set of flags that are simply for internal use
> that is a superset of them all? Sure. But no, it's almost certainly
> not worth it. Four is not better than three.
As you pointed our below the LOOKUP_yxz namespace is already in place...
And the discussion was more about an possible single namespace for
completely new syscalls.
> Now, some type-safety in the kernel to make sure that we can't mix
> AT_xyz with O_xyz or RESOLVE_xyz - that might be worth it. Although
> judging by past experience, not enough people run sparse for it to
> really be worth it.
I'm new to all this and maybe too naive, but would a build bot running
sparse on linux-next be able to catch this early enough?
metze
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: Linus Torvalds @ 2020-03-12 16:24 UTC (permalink / raw)
To: Stefan Metzmacher
Cc: David Howells, Aleksa Sarai, Al Viro, Ian Kent, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak, jlayton,
Linux API, linux-fsdevel, LSM List, Linux Kernel Mailing List
In-Reply-To: <3d209e29-e73d-23a6-5c6f-0267b1e669b6@samba.org>
On Thu, Mar 12, 2020 at 2:08 AM Stefan Metzmacher <metze@samba.org> wrote:
>
> The whole discussion was triggered by the introduction of a completely
> new fsinfo() call:
>
> Would you propose to have 'at_flags' and 'resolve_flags' passed in here?
Yes, I think that would be the way to go.
> > If we need linkat2() and friends, so be it. Do we?
>
> Yes, I'm going to propose something like this, as it would make the life
> much easier for Samba to have the new features available on all path
> based syscalls.
Will samba actually use them? I think we've had extensions before that
weren't worth the non-portability pain?
But yes, if we have a major package like samba use it, then by all
means let's add linkat2(). How many things are we talking about? We
have a number of system calls that do *not* take flags, but do do
pathname walking. I'm thinking things like "mkdirat()"?)
> In addition I'll propose to have a way to specify the source of
> removeat and unlinkat also by fd in addition to the the source parent fd
> and relative path, the reason are also to detect races of path
> recycling.
Would that be basically just an AT_EMPTY_PATH kind of thing? IOW,
you'd be able to remove a file by doing
fd = open(path.., O_PATH);
unlinkat(fd, "", AT_EMPTY_PATH);
Hmm. We have _not_ allowed filesystem changes without that last
component lookup. Of course, with our dentry model, we *can* do it,
but this smells fairly fundamental to me.
It might avoid some of the extra system calls (ie you could use
openat2() to do the path walking part, and then
unlinkat(AT_EMPTY_PATH) to remove it, and have a "fstat()" etc in
between the verify that it's the right type of file or whatever - and
you'd not need an unlinkat2() with resolve flags).
I think Al needs to ok this kind of change. Maybe you've already
discussed it with him and I just missed it.
Linus
^ permalink raw reply
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: David Howells @ 2020-03-12 16:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: dhowells, Stefan Metzmacher, Aleksa Sarai, Al Viro, Ian Kent,
Miklos Szeredi, Christian Brauner, Jann Horn, Darrick J. Wong,
Karel Zak, jlayton, Linux API, linux-fsdevel, LSM List,
Linux Kernel Mailing List
In-Reply-To: <CAHk-=wgu3Wo_xcjXnwski7JZTwQFaMmKD0hoTZ=hqQv3-YojSg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > The whole discussion was triggered by the introduction of a completely
> > new fsinfo() call:
> >
> > Would you propose to have 'at_flags' and 'resolve_flags' passed in here?
>
> Yes, I think that would be the way to go.
Okay, I can do that.
Any thoughts on which set of flags should override the other? If we're making
RESOLVE_* flags the new definitive interface, then I feel they should probably
override the AT_* flags where there's a conflict, ie. RESOLVE_NO_SYMLINKS
should override AT_SYMLINK_FOLLOW for example.
David
^ permalink raw reply
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: Stefan Metzmacher @ 2020-03-12 17:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, Aleksa Sarai, Al Viro, Ian Kent, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak, jlayton,
Linux API, linux-fsdevel, LSM List, Linux Kernel Mailing List,
Jeremy Allison, Ralph Böhme, Volker Lendecke
In-Reply-To: <CAHk-=wgu3Wo_xcjXnwski7JZTwQFaMmKD0hoTZ=hqQv3-YojSg@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3679 bytes --]
Am 12.03.20 um 17:24 schrieb Linus Torvalds:
> On Thu, Mar 12, 2020 at 2:08 AM Stefan Metzmacher <metze@samba.org> wrote:
>>
>> The whole discussion was triggered by the introduction of a completely
>> new fsinfo() call:
>>
>> Would you propose to have 'at_flags' and 'resolve_flags' passed in here?
>
> Yes, I think that would be the way to go.
Ok, that's also fine for me:-)
>>> If we need linkat2() and friends, so be it. Do we?
>>
>> Yes, I'm going to propose something like this, as it would make the life
>> much easier for Samba to have the new features available on all path
>> based syscalls.
>
> Will samba actually use them? I think we've had extensions before that
> weren't worth the non-portability pain?
Yes, we're currently moving to the portable *at() calls as a start.
And we already make use of Linux only feature for performance reasons
in other places. Having the new resolve flags will make it possible to
move some of the performance intensive work into non-linux specific
modules as fallback.
I hope that we'll use most of this through io_uring in the end,
that's the reason Jens added the IORING_REGISTER_PERSONALITY feature
used for IORING_OP_OPENAT2.
> But yes, if we have a major package like samba use it, then by all
> means let's add linkat2(). How many things are we talking about? We
> have a number of system calls that do *not* take flags, but do do
> pathname walking. I'm thinking things like "mkdirat()"?)
I haven't looked them up in detail yet.
Jeremy can you provide a list?
Do you think we could route some of them like mkdirat() and mknodat()
via openat2() instead of creating new syscalls?
>> In addition I'll propose to have a way to specify the source of
>> removeat and unlinkat also by fd in addition to the the source parent fd
>> and relative path, the reason are also to detect races of path
>> recycling.
>
> Would that be basically just an AT_EMPTY_PATH kind of thing? IOW,
> you'd be able to remove a file by doing
>
> fd = open(path.., O_PATH);
> unlinkat(fd, "", AT_EMPTY_PATH);
>
> Hmm. We have _not_ allowed filesystem changes without that last
> component lookup. Of course, with our dentry model, we *can* do it,
> but this smells fairly fundamental to me.
>
> It might avoid some of the extra system calls (ie you could use
> openat2() to do the path walking part, and then
> unlinkat(AT_EMPTY_PATH) to remove it, and have a "fstat()" etc in
> between the verify that it's the right type of file or whatever - and
> you'd not need an unlinkat2() with resolve flags).
If that works safely for hardlinks and having another process doing a
rename between openat2() and unlinkat(), we could try that.
My initial naive idea was to have one syscall instead of
linkat2/renameat3/unlinkat2.
int xlinkat(int src_dfd, const char *src_path,
int dst_dfd, const char *dst_path,
const struct xlinkat_how *how, size_t how_size);
struct xlinkat_how {
__u64 src_at_flags;
__u64 src_resolve_flags;
__u64 dst_at_flags;
__u64 dst_resolve_flags;
__u64 rename_flags;
__s32 src_fd;
};
With src_dfd=-1, src_path=NULL, how.src_fd = -1, this would be like
linkat().
With dst_dfd=-1, dst_path=NULL, it would be like unlinkat().
Otherwise a renameat2().
If how.src_fd is not -1, it would be checked to be the same path as
specified by src_dfd and src_path.
> I think Al needs to ok this kind of change. Maybe you've already
> discussed it with him and I just missed it.
This is the first time I'm discussing this.
Thanks for the useful feedback!
metze
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: Linus Torvalds @ 2020-03-12 18:09 UTC (permalink / raw)
To: David Howells
Cc: Stefan Metzmacher, Aleksa Sarai, Al Viro, Ian Kent,
Miklos Szeredi, Christian Brauner, Jann Horn, Darrick J. Wong,
Karel Zak, jlayton, Linux API, linux-fsdevel, LSM List,
Linux Kernel Mailing List
In-Reply-To: <1147628.1584032201@warthog.procyon.org.uk>
On Thu, Mar 12, 2020 at 9:56 AM David Howells <dhowells@redhat.com> wrote:
>
> Any thoughts on which set of flags should override the other?
Do we need to care? I don't think we actually have conflicts, because
the semantics aren't the same, and they are about independent issues.
> If we're making
> RESOLVE_* flags the new definitive interface, then I feel they should probably
> override the AT_* flags where there's a conflict, ie. RESOLVE_NO_SYMLINKS
> should override AT_SYMLINK_FOLLOW for example.
That's just for a linkat2() system call? I think the natural semantic
is the one that falls out directly: RESOLVE_NO_SYMLINKS will cause it
to fail with -ELOOP if it is a symlink.
NOTE! This isn't really a "conflict". It's actually two different and
independent things:
- without AT_SYMLINK_FOLLOW, a linkat() simply won't even try to
follow the symlink, and will link to the symlink itself instead.
- RESOLVE_NO_SYMLINKS says "never follow symlinks".
Note how one does *NOT* override the other, quite the reverse. They
are about different things. One is about the _behavior_ when the last
component is a symlink, and the other is about permission to follow
any symlinks.
So all combinations make sense:
- no AT_SYMLINK_FOLLOW, no RESOLVE_NO_SYMLINKS: just link to the
target, whether it's a symlink or not
This is obviously our historical link() behavior.
- no AT_SYMLINK_FOLLOW, yes RESOLVE_NO_SYMLINKS: just link to the
target, whether it's a symlink or not, but if there's a symlink in the
middle, return -ELOOP
Note how this case doesn't follow the last one, so
RESOLVE_NO_SYMLINKS isn't an issue for the last component, but _is_ an
issue for the components in the middle.
- AT_SYMLINK_FOLLOW, no RESOLVE_NO_SYMLINKS: just link to the target,
following the symlink if it exists
This is obviously the historical AT_SYMLINK_FOLLOW behavior
- AT_SYMLINK_FOLLOW | RESOLVE_NO_SYMLINKS: just link to the target,
return -ELOOP if it's a symlink (of if there's a symlink on the way).
This is the natural behavior for "refuse to follow any symlinks anywhere".
note how they are all completely sane versions, and in no case does
one flag really override the other.
If anything, we actually miss a third flag: the "don't allow linking
to a final symlink at all" (but allow intermediate symlinks). We've
never had that behavior, although I think POSIX makes that case
undefined (ie you're not guaranteed to be able to link to a symlink in
the first place in POSIX).
I guess that third case could be emulated with open(O_PATH) + fstat to
check it's not a symlink + linkat(fd,AT_EMPTY_PATH) if it turns
somebody would want something like that (and we decided that
AT_EMPTY_PATH is ok for linkat()).
I doubt anybody cares.
Linus
^ permalink raw reply
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: Al Viro @ 2020-03-12 19:25 UTC (permalink / raw)
To: Linus Torvalds
Cc: Stefan Metzmacher, David Howells, Aleksa Sarai, Ian Kent,
Miklos Szeredi, Christian Brauner, Jann Horn, Darrick J. Wong,
Karel Zak, jlayton, Linux API, linux-fsdevel, LSM List,
Linux Kernel Mailing List
In-Reply-To: <CAHk-=wgu3Wo_xcjXnwski7JZTwQFaMmKD0hoTZ=hqQv3-YojSg@mail.gmail.com>
On Thu, Mar 12, 2020 at 09:24:49AM -0700, Linus Torvalds wrote:
> Would that be basically just an AT_EMPTY_PATH kind of thing? IOW,
> you'd be able to remove a file by doing
>
> fd = open(path.., O_PATH);
> unlinkat(fd, "", AT_EMPTY_PATH);
>
> Hmm. We have _not_ allowed filesystem changes without that last
> component lookup. Of course, with our dentry model, we *can* do it,
> but this smells fairly fundamental to me.
That's a bloody bad idea. It breeds fuckloads of corner cases, it does not
match the locking model at all and I don't want to even think of e.g.
the interplay with open-by-fhandle ("Parent? What parent?"), etc.
Fundamentally, there are operations on objects and there are operations
on links to objects. Mixing those is the recipe for massive headache.
> It might avoid some of the extra system calls (ie you could use
> openat2() to do the path walking part, and then
> unlinkat(AT_EMPTY_PATH) to remove it, and have a "fstat()" etc in
> between the verify that it's the right type of file or whatever - and
> you'd not need an unlinkat2() with resolve flags).
>
> I think Al needs to ok this kind of change. Maybe you've already
> discussed it with him and I just missed it.
They have not. And IME samba folks tend to present the set of
primitives they want without bothering to explain what do they
want to factorize that way, let alone why it should be factorized
that way...
^ permalink raw reply
* Re: [PATCH 01/14] VFS: Add additional RESOLVE_* flags [ver #18]
From: Al Viro @ 2020-03-12 19:37 UTC (permalink / raw)
To: Stefan Metzmacher
Cc: Linus Torvalds, David Howells, Aleksa Sarai, Ian Kent,
Miklos Szeredi, Christian Brauner, Jann Horn, Darrick J. Wong,
Karel Zak, jlayton, Linux API, linux-fsdevel, LSM List,
Linux Kernel Mailing List, Jeremy Allison, Ralph Böhme,
Volker Lendecke
In-Reply-To: <8d24e9f6-8e90-96bb-6e98-035127af0327@samba.org>
On Thu, Mar 12, 2020 at 06:11:09PM +0100, Stefan Metzmacher wrote:
> If that works safely for hardlinks and having another process doing a
> rename between openat2() and unlinkat(), we could try that.
>
> My initial naive idea was to have one syscall instead of
> linkat2/renameat3/unlinkat2.
>
> int xlinkat(int src_dfd, const char *src_path,
> int dst_dfd, const char *dst_path,
> const struct xlinkat_how *how, size_t how_size);
>
> struct xlinkat_how {
> __u64 src_at_flags;
> __u64 src_resolve_flags;
> __u64 dst_at_flags;
> __u64 dst_resolve_flags;
> __u64 rename_flags;
> __s32 src_fd;
> };
>
> With src_dfd=-1, src_path=NULL, how.src_fd = -1, this would be like
> linkat().
> With dst_dfd=-1, dst_path=NULL, it would be like unlinkat().
> Otherwise a renameat2().
>
> If how.src_fd is not -1, it would be checked to be the same path as
> specified by src_dfd and src_path.
"Checked" as in...? And is that the same path or another link to the
same object, or...?
The idea of dumping all 3 into the same syscall looks wrong - compare
the effects of link() and rename() on the opened files, for starters,
and try to come up with documentation for all of that. Multiplexors
tend to be very bad, in large part because they have so bloody
convoluted semantics...
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox