Re: [PATCH v3 01/18] vfs: add fileattr ops

[Miklos Szeredi]

On Tue, Apr 13, 2021 at 4:46 PM Matthew Wilcox  wrote:
>
> On Thu, Mar 25, 2021 at 08:37:38PM +0100, Miklos Szeredi wrote:
> > @@ -107,6 +110,8 @@ fiemap:   no
> >  update_time: no
> >  atomic_open: shared (exclusive if O_CREAT is set in open flags)
> >  tmpfile: no
> > +fileattr_get:no or exclusive
> > +fileattr_set:exclusive
> >   =
>
> This introduces a warning to `make htmldocs`:
>
> /home/willy/kernel/folio/Documentation/filesystems/locking.rst:113: WARNING: 
> Malformed table.
> Text in column margin in table line 24.
>
> You need to add an extra '=' to the first batch of '=' (on all three lines of
> the table).  Like this:

Yep, already fixed in #fileattr_v6, which I asked Al to pull.

Thanks,
Miklos

Re: [PATCH v3 01/18] vfs: add fileattr ops

[Matthew Wilcox]

On Thu, Mar 25, 2021 at 08:37:38PM +0100, Miklos Szeredi wrote:
> @@ -107,6 +110,8 @@ fiemap:   no
>  update_time: no
>  atomic_open: shared (exclusive if O_CREAT is set in open flags)
>  tmpfile: no
> +fileattr_get:no or exclusive
> +fileattr_set:exclusive
>   =

This introduces a warning to `make htmldocs`:

/home/willy/kernel/folio/Documentation/filesystems/locking.rst:113: WARNING: 
Malformed table.
Text in column margin in table line 24.

You need to add an extra '=' to the first batch of '=' (on all three lines of
the table).  Like this:

@@ -87,9 +87,9 @@ prototypes::
 locking rules:
all may block
 
-   =
+=  =
 opsi_rwsem(inode)
-   =
+=  =
 lookup:shared
 create:exclusive
 link:  exclusive (both)
@@ -112,7 +112,7 @@ atomic_open:shared (exclusive if O_CREAT is set in 
open flags)
 tmpfile:   no
 fileattr_get:  no or exclusive
 fileattr_set:  exclusive
-   =
+=  =

(whitespace damaged)

Re: [PATCH v3 01/18] vfs: add fileattr ops

[Miklos Szeredi]

On Sun, Mar 28, 2021 at 8:08 PM Al Viro  wrote:
>
> On Thu, Mar 25, 2021 at 08:37:38PM +0100, Miklos Szeredi wrote:
>
> > +int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> > +{
> > + struct inode *inode = d_inode(dentry);
> > +
> > + if (d_is_special(dentry))
> > + return -ENOTTY;
>
> FWIW - why?  For uses via ioctl() you simply won't get there with
> device nodes et.al. - they have file_operations of their own.

Yes it will: this is called by the vfs, not the filesystem.

> If we add syscall(s) for getting/setting those, there's no reason
> for e.g. a device node not to have those attributes...

Fair enough, but I guess filesystems will need to explicitly enable
support for these attributes on special files.

We can move that check inside filesystems now, or we can move it
later, when actually needed.  Which do you prefer?  (only a couple of
filesystems are affected, IIRC, which don't have separate i_ops for
regular and special files).

Thanks,
Miklos

Re: [PATCH v3 01/18] vfs: add fileattr ops

[Al Viro]

On Thu, Mar 25, 2021 at 08:37:38PM +0100, Miklos Szeredi wrote:

> +int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> +{
> + struct inode *inode = d_inode(dentry);
> +
> + if (d_is_special(dentry))
> + return -ENOTTY;

FWIW - why?  For uses via ioctl() you simply won't get there with
device nodes et.al. - they have file_operations of their own.
If we add syscall(s) for getting/setting those, there's no reason
for e.g. a device node not to have those attributes...

> +static int ioctl_getflags(struct file *file, void __user *argp)

unsigned int __user *argp, surely?

> +{
> + struct fileattr fa = { .flags_valid = true }; /* hint only */
> + unsigned int flags;
> + int err;
> +
> + err = vfs_fileattr_get(file->f_path.dentry, );
> + if (!err) {
> + flags = fa.flags;
> + if (copy_to_user(argp, , sizeof(flags)))
> + err = -EFAULT;

... and put_user() here.

> + }
> + return err;
> +}
> +
> +static int ioctl_setflags(struct file *file, void __user *argp)
> +{
> + struct fileattr fa;
> + unsigned int flags;
> + int err;
> +
> + if (copy_from_user(, argp, sizeof(flags)))
> + return -EFAULT;
> +
> + err = mnt_want_write_file(file);
> + if (!err) {
> + fileattr_fill_flags(, flags);
> + err = vfs_fileattr_set(file_mnt_user_ns(file), 
> file_dentry(file), );
> + mnt_drop_write_file(file);
> + }
> + return err;
> +}

Similar here.

[PATCH v3 01/18] vfs: add fileattr ops

[Miklos Szeredi]

There's a substantial amount of boilerplate in filesystems handling
FS_IOC_[GS]ETFLAGS/ FS_IOC_FS[GS]ETXATTR ioctls.

Also due to userspace buffers being involved in the ioctl API this is
difficult to stack, as shown by overlayfs issues related to these ioctls.

Introduce a new internal API named "fileattr" (fsxattr can be confused with
xattr, xflags is inappropriate, since this is more than just flags).

There's significant overlap between flags and xflags and this API handles
the conversions automatically, so filesystems may choose which one to use.

In ->fileattr_get() a hint is provided to the filesystem whether flags or
xattr are being requested by userspace, but in this series this hint is
ignored by all filesystems, since generating all the attributes is cheap.

If a filesystem doesn't implemement the fileattr API, just fall back to
f_op->ioctl().  When all filesystems are converted, the fallback can be
removed.

32bit compat ioctls are now handled by the generic code as well.

Signed-off-by: Miklos Szeredi 
---
 Documentation/filesystems/locking.rst |   5 +
 Documentation/filesystems/vfs.rst |  15 ++
 fs/ioctl.c| 331 ++
 include/linux/fileattr.h  |  59 +
 include/linux/fs.h|   4 +
 5 files changed, 414 insertions(+)
 create mode 100644 include/linux/fileattr.h

diff --git a/Documentation/filesystems/locking.rst 
b/Documentation/filesystems/locking.rst
index b7dcc86c92a4..7b9b307bedfc 100644
--- a/Documentation/filesystems/locking.rst
+++ b/Documentation/filesystems/locking.rst
@@ -80,6 +80,9 @@ prototypes::
struct file *, unsigned open_flag,
umode_t create_mode);
int (*tmpfile) (struct inode *, struct dentry *, umode_t);
+   int (*fileattr_set)(struct user_namespace *mnt_userns,
+   struct dentry *dentry, struct fileattr *fa);
+   int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
 
 locking rules:
all may block
@@ -107,6 +110,8 @@ fiemap: no
 update_time:   no
 atomic_open:   shared (exclusive if O_CREAT is set in open flags)
 tmpfile:   no
+fileattr_get:  no or exclusive
+fileattr_set:  exclusive
    =
 
 
diff --git a/Documentation/filesystems/vfs.rst 
b/Documentation/filesystems/vfs.rst
index 2049bbf5e388..14c31eced416 100644
--- a/Documentation/filesystems/vfs.rst
+++ b/Documentation/filesystems/vfs.rst
@@ -441,6 +441,9 @@ As of kernel 2.6.22, the following members are defined:
   unsigned open_flag, umode_t create_mode);
int (*tmpfile) (struct user_namespace *, struct inode *, struct 
dentry *, umode_t);
int (*set_acl)(struct user_namespace *, struct inode *, struct 
posix_acl *, int);
+   int (*fileattr_set)(struct user_namespace *mnt_userns,
+   struct dentry *dentry, struct fileattr *fa);
+   int (*fileattr_get)(struct dentry *dentry, struct fileattr *fa);
};
 
 Again, all methods are called without any locks being held, unless
@@ -588,6 +591,18 @@ otherwise noted.
atomically creating, opening and unlinking a file in given
directory.
 
+``fileattr_get``
+   called on ioctl(FS_IOC_GETFLAGS) and ioctl(FS_IOC_FSGETXATTR) to
+   retrieve miscellaneous file flags and attributes.  Also called
+   before the relevant SET operation to check what is being changed
+   (in this case with i_rwsem locked exclusive).  If unset, then
+   fall back to f_op->ioctl().
+
+``fileattr_set``
+   called on ioctl(FS_IOC_SETFLAGS) and ioctl(FS_IOC_FSSETXATTR) to
+   change miscellaneous file flags and attributes.  Callers hold
+   i_rwsem exclusive.  If unset, then fall back to f_op->ioctl().
+
 
 The Address Space Object
 
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 4e6cc0a7d69c..b27b5316b30c 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -19,6 +19,9 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 #include "internal.h"
 
@@ -657,6 +660,313 @@ static int ioctl_file_dedupe_range(struct file *file,
return ret;
 }
 
+/**
+ * fileattr_fill_xflags - initialize fileattr with xflags
+ * @fa:fileattr pointer
+ * @xflags:FS_XFLAG_* flags
+ *
+ * Set ->fsx_xflags, ->fsx_valid and ->flags (translated xflags).  All
+ * other fields are zeroed.
+ */
+void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
+{
+   memset(fa, 0, sizeof(*fa));
+   fa->fsx_valid = true;
+   fa->fsx_xflags = xflags;
+   if (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)
+   fa->flags |= FS_IMMUTABLE_FL;
+   if (fa->fsx_xflags & FS_XFLAG_APPEND)
+   fa->flags |= FS_APPEND_FL;
+   if (fa->fsx_xflags & FS_XFLAG_SYNC)
+   fa->flags |= FS_SYNC_FL;
+   if (fa->fsx_xflags