Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-24 Thread Miklos Szeredi
On Wed, Mar 24, 2021 at 1:28 PM Al Viro  wrote:
>
> On Wed, Mar 24, 2021 at 09:45:02AM +0100, Miklos Szeredi wrote:

> > Isn't structure initialization supposed to zero everything not
> > explicitly initialized?
>
> All fields, but not the padding...

Ah...  while the structure is unlikely to change, I'll switch to
memset to be safe.

>
> > The one in io_uring() seems wrong also, as a beast needing
> > file_dentry() should never get out of overlayfs and into io_uring:
>
> That one would be wrong in overlayfs as well - we'd better had the
> same names in all layers...

Right.

Thanks,
Miklos


Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-24 Thread Al Viro
On Wed, Mar 24, 2021 at 09:45:02AM +0100, Miklos Szeredi wrote:
> On Wed, Mar 24, 2021 at 6:03 AM Al Viro  wrote:
> >
> > On Mon, Mar 22, 2021 at 03:48:59PM +0100, Miklos Szeredi wrote:
> >
> > minor nit: copy_fsxattr_{to,from}_user() might be better.
> >
> > > +int fsxattr_copy_to_user(const struct miscattr *ma, struct fsxattr 
> > > __user *ufa)
> > > +{
> > > + struct fsxattr fa = {
> > > + .fsx_xflags = ma->fsx_xflags,
> > > + .fsx_extsize= ma->fsx_extsize,
> > > + .fsx_nextents   = ma->fsx_nextents,
> > > + .fsx_projid = ma->fsx_projid,
> > > + .fsx_cowextsize = ma->fsx_cowextsize,
> > > + };
> >
> > That wants a comment along the lines of "guaranteed to be gap-free",
> > since otherwise you'd need memset() to avoid an infoleak.
> 
> Isn't structure initialization supposed to zero everything not
> explicitly initialized?

All fields, but not the padding...

> The one in io_uring() seems wrong also, as a beast needing
> file_dentry() should never get out of overlayfs and into io_uring:

That one would be wrong in overlayfs as well - we'd better had the
same names in all layers...

> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -9297,7 +9297,7 @@ static void __io_uring_show_fdinfo(struct
> io_ring_ctx *ctx, struct seq_file *m)
> struct file *f = *io_fixed_file_slot(ctx->file_data, i);
> 
> if (f)
> -   seq_printf(m, "%5u: %s\n", i, 
> file_dentry(f)->d_iname);
> +   seq_printf(m, "%5u: %pD\n", i, f);
> else
> seq_printf(m, "%5u: \n", i);
> }
> 
> 
> Thanks,
> Miklos


Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-24 Thread Miklos Szeredi
On Wed, Mar 24, 2021 at 6:03 AM Al Viro  wrote:
>
> On Mon, Mar 22, 2021 at 03:48:59PM +0100, Miklos Szeredi wrote:
>
> minor nit: copy_fsxattr_{to,from}_user() might be better.
>
> > +int fsxattr_copy_to_user(const struct miscattr *ma, struct fsxattr __user 
> > *ufa)
> > +{
> > + struct fsxattr fa = {
> > + .fsx_xflags = ma->fsx_xflags,
> > + .fsx_extsize= ma->fsx_extsize,
> > + .fsx_nextents   = ma->fsx_nextents,
> > + .fsx_projid = ma->fsx_projid,
> > + .fsx_cowextsize = ma->fsx_cowextsize,
> > + };
>
> That wants a comment along the lines of "guaranteed to be gap-free",
> since otherwise you'd need memset() to avoid an infoleak.

Isn't structure initialization supposed to zero everything not
explicitly initialized?

>
> > +static int ioctl_getflags(struct file *file, void __user *argp)
> > +{
> > + struct miscattr ma = { .flags_valid = true }; /* hint only */
> > + unsigned int flags;
> > + int err;
> > +
> > + err = vfs_miscattr_get(file_dentry(file), );
>
> Umm...  Just to clarify - do we plan to have that ever called via
> ovl_real_ioctl()?  IOW, is file_dentry() anything other than a way
> to spell ->f_path.dentry here?

Indeed, file_dentry() only makes sense when called from a layer inside
overlayfs.

The one in io_uring() seems wrong also, as a beast needing
file_dentry() should never get out of overlayfs and into io_uring:

--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -9297,7 +9297,7 @@ static void __io_uring_show_fdinfo(struct
io_ring_ctx *ctx, struct seq_file *m)
struct file *f = *io_fixed_file_slot(ctx->file_data, i);

if (f)
-   seq_printf(m, "%5u: %s\n", i, file_dentry(f)->d_iname);
+   seq_printf(m, "%5u: %pD\n", i, f);
else
seq_printf(m, "%5u: \n", i);
}


Thanks,
Miklos


Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-24 Thread Christian Brauner
On Mon, Mar 22, 2021 at 03:48:59PM +0100, Miklos Szeredi wrote:
> 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 "miscattr" (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 ->miscattr_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 miscattr 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 
> ---

Fwiw, I think this is a good cleanup. Changing something like the
miscattr_set() method to take a mnt_userns would've been less
churn then having to audit all ioctls individually.

(Only one small comment below.)

>  Documentation/filesystems/locking.rst |   5 +
>  Documentation/filesystems/vfs.rst |  15 ++
>  fs/ioctl.c| 329 ++
>  include/linux/fs.h|   4 +
>  include/linux/miscattr.h  |  53 +
>  5 files changed, 406 insertions(+)
>  create mode 100644 include/linux/miscattr.h
> 
> diff --git a/Documentation/filesystems/locking.rst 
> b/Documentation/filesystems/locking.rst
> index b7dcc86c92a4..a5aa2046d48f 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 (*miscattr_set)(struct user_namespace *mnt_userns,
> + struct dentry *dentry, struct miscattr *ma);
> + int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
>  
>  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
> +miscattr_get:no or exclusive
> +miscattr_set:exclusive
>   =
>  
>  
> diff --git a/Documentation/filesystems/vfs.rst 
> b/Documentation/filesystems/vfs.rst
> index 2049bbf5e388..f125ce6c3b47 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 (*miscattr_set)(struct user_namespace *mnt_userns,
> + struct dentry *dentry, struct miscattr *ma);
> + int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
>   };
>  
>  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.
>  
> +``miscattr_get``
> + called on ioctl(FS_IOC_GETFLAGS) and ioctl(FS_IOC_FSGETXATTR) to
> + retrieve miscellaneous filesystem 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().
> +
> +``miscattr_set``
> + called on ioctl(FS_IOC_SETFLAGS) and ioctl(FS_IOC_FSSETXATTR) to
> + change miscellaneous filesystem 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..e5f3820809a4 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,311 @@ static int ioctl_file_dedupe_range(struct file *file,
>   return ret;
>  }
>  
> +/**
> + * miscattr_fill_xflags - initialize miscattr with xflags
> + * @ma:  miscattr pointer
> + * @xflags:  FS_XFLAG_* flags
> + *
> + * Set ->fsx_xflags, ->xattr_valid and ->flags (translated xflags).  All
> + * other fields 

Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-24 Thread Christian Brauner
On Mon, Mar 22, 2021 at 03:33:38PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 22, 2021 at 03:48:59PM +0100, Miklos Szeredi wrote:
> > 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 "miscattr" (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 ->miscattr_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 miscattr 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| 329 ++
> >  include/linux/fs.h|   4 +
> >  include/linux/miscattr.h  |  53 +
> >  5 files changed, 406 insertions(+)
> >  create mode 100644 include/linux/miscattr.h
> > 
> > diff --git a/Documentation/filesystems/locking.rst 
> > b/Documentation/filesystems/locking.rst
> > index b7dcc86c92a4..a5aa2046d48f 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 (*miscattr_set)(struct user_namespace *mnt_userns,
> > +   struct dentry *dentry, struct miscattr *ma);
> > +   int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
> >  
> >  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
> > +miscattr_get:  no or exclusive
> > +miscattr_set:  exclusive
> >     =
> >  
> >  
> > diff --git a/Documentation/filesystems/vfs.rst 
> > b/Documentation/filesystems/vfs.rst
> > index 2049bbf5e388..f125ce6c3b47 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 (*miscattr_set)(struct user_namespace *mnt_userns,
> > +   struct dentry *dentry, struct miscattr *ma);
> > +   int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
> > };
> >  
> >  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.
> >  
> > +``miscattr_get``
> 
> I wish this wasn't named "misc" because miscellaneous is vague.
> 
> fileattr_get, perhaps?
> 
> (FWIW I'm not /that/ passionate about starting a naming bikeshed, feel
> free to ignore.)
> 
> > +   called on ioctl(FS_IOC_GETFLAGS) and ioctl(FS_IOC_FSGETXATTR) to
> > +   retrieve miscellaneous filesystem flags and attributes.  Also
> 
> "...miscellaneous *file* flags and attributes."
> 
> > +   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().
> > +
> > +``miscattr_set``
> > +   called on ioctl(FS_IOC_SETFLAGS) and ioctl(FS_IOC_FSSETXATTR) to
> > +   change miscellaneous filesystem flags and attributes.  Callers hold
> 
> Same here.
> 
> > +   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..e5f3820809a4 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,311 @@ static int 

Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-23 Thread Al Viro
On Mon, Mar 22, 2021 at 03:48:59PM +0100, Miklos Szeredi wrote:

minor nit: copy_fsxattr_{to,from}_user() might be better.

> +int fsxattr_copy_to_user(const struct miscattr *ma, struct fsxattr __user 
> *ufa)
> +{
> + struct fsxattr fa = {
> + .fsx_xflags = ma->fsx_xflags,
> + .fsx_extsize= ma->fsx_extsize,
> + .fsx_nextents   = ma->fsx_nextents,
> + .fsx_projid = ma->fsx_projid,
> + .fsx_cowextsize = ma->fsx_cowextsize,
> + };

That wants a comment along the lines of "guaranteed to be gap-free",
since otherwise you'd need memset() to avoid an infoleak.

> +static int ioctl_getflags(struct file *file, void __user *argp)
> +{
> + struct miscattr ma = { .flags_valid = true }; /* hint only */
> + unsigned int flags;
> + int err;
> +
> + err = vfs_miscattr_get(file_dentry(file), );

Umm...  Just to clarify - do we plan to have that ever called via
ovl_real_ioctl()?  IOW, is file_dentry() anything other than a way
to spell ->f_path.dentry here?

> +struct miscattr {
> + u32 flags;  /* flags (FS_IOC_GETFLAGS/FS_IOC_SETFLAGS) */
> + /* struct fsxattr: */
> + u32 fsx_xflags; /* xflags field value (get/set) */
> + u32 fsx_extsize;/* extsize field value (get/set)*/
> + u32 fsx_nextents;   /* nextents field value (get)   */
> + u32 fsx_projid; /* project identifier (get/set) */
> + u32 fsx_cowextsize; /* CoW extsize field value (get/set)*/
> + /* selectors: */
> + boolflags_valid:1;
> + boolxattr_valid:1;
> +};

OK as long as it stays kernel-only, but if we ever expose that to userland, we'd
better remember to turn the last two into an u32 with explicit bitmasks.


Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-23 Thread David Sterba
On Mon, Mar 22, 2021 at 03:33:38PM -0700, Darrick J. Wong wrote:
> On Mon, Mar 22, 2021 at 03:48:59PM +0100, Miklos Szeredi wrote:
> > --- 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 (*miscattr_set)(struct user_namespace *mnt_userns,
> > +   struct dentry *dentry, struct miscattr *ma);
> > +   int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
> > };
> >  
> >  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.
> >  
> > +``miscattr_get``
> 
> I wish this wasn't named "misc" because miscellaneous is vague.

It also adds yet another way to name all the attributes (the "N + 1st
standard" problem). So I'd rather reuse a term that's already known and
understood by users. And this is 'file attributes', eg. as noted in
chattr manual page "change file attributes on a Linux file system".
For clarity avoid any 'x' in the name so we easily distinguish that from
the extended attributes aka xattrs.

We can perhaps live with miscattrs in code as anybody who has ever
touched the flags/attrs interfaces knows what it is referring to.

> fileattr_get, perhaps?

That sounds about right to me.


Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-22 Thread Amir Goldstein
> > +``miscattr_get``
>
> I wish this wasn't named "misc" because miscellaneous is vague.
>
> fileattr_get, perhaps?
>
> (FWIW I'm not /that/ passionate about starting a naming bikeshed, feel
> free to ignore.)
>

Eventual bikeshedding is hard to avoid in this case...

I don't feel strongly against "misc", but I do think the flags and
ioctl are already
known as "fsx" so it would be more friendly to go with that.

If you don't like "fsxflags" because it's not only flags and you think
"fsxattr" is too
close to "xattr" (FWIW I don't think it is going to be a source of
confusion), we
can simply go with get_fsx(), similar to get_acl(). It doesn't matter
what name we
use as long as everyone is clear on what it is.

"struct fsx" is not any more or any less clear than "struct statx" and
while "fsx"
it is a pretty arbitrary name, it is not much less arbitrary than "miscattr".

Thanks,
Amir.


Re: [PATCH v2 01/18] vfs: add miscattr ops

2021-03-22 Thread Darrick J. Wong
On Mon, Mar 22, 2021 at 03:48:59PM +0100, Miklos Szeredi wrote:
> 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 "miscattr" (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 ->miscattr_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 miscattr 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| 329 ++
>  include/linux/fs.h|   4 +
>  include/linux/miscattr.h  |  53 +
>  5 files changed, 406 insertions(+)
>  create mode 100644 include/linux/miscattr.h
> 
> diff --git a/Documentation/filesystems/locking.rst 
> b/Documentation/filesystems/locking.rst
> index b7dcc86c92a4..a5aa2046d48f 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 (*miscattr_set)(struct user_namespace *mnt_userns,
> + struct dentry *dentry, struct miscattr *ma);
> + int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
>  
>  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
> +miscattr_get:no or exclusive
> +miscattr_set:exclusive
>   =
>  
>  
> diff --git a/Documentation/filesystems/vfs.rst 
> b/Documentation/filesystems/vfs.rst
> index 2049bbf5e388..f125ce6c3b47 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 (*miscattr_set)(struct user_namespace *mnt_userns,
> + struct dentry *dentry, struct miscattr *ma);
> + int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
>   };
>  
>  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.
>  
> +``miscattr_get``

I wish this wasn't named "misc" because miscellaneous is vague.

fileattr_get, perhaps?

(FWIW I'm not /that/ passionate about starting a naming bikeshed, feel
free to ignore.)

> + called on ioctl(FS_IOC_GETFLAGS) and ioctl(FS_IOC_FSGETXATTR) to
> + retrieve miscellaneous filesystem flags and attributes.  Also

"...miscellaneous *file* flags and attributes."

> + 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().
> +
> +``miscattr_set``
> + called on ioctl(FS_IOC_SETFLAGS) and ioctl(FS_IOC_FSSETXATTR) to
> + change miscellaneous filesystem flags and attributes.  Callers hold

Same here.

> + 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..e5f3820809a4 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,311 @@ static int ioctl_file_dedupe_range(struct file *file,
>   return ret;
>  }
>  
> +/**
> + * miscattr_fill_xflags - initialize miscattr with xflags
> + * @ma:  miscattr pointer
> + * @xflags:  FS_XFLAG_* flags
> + *
> + * Set ->fsx_xflags, ->xattr_valid and ->flags (translated 

[PATCH v2 01/18] vfs: add miscattr ops

2021-03-22 Thread 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 "miscattr" (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 ->miscattr_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 miscattr 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| 329 ++
 include/linux/fs.h|   4 +
 include/linux/miscattr.h  |  53 +
 5 files changed, 406 insertions(+)
 create mode 100644 include/linux/miscattr.h

diff --git a/Documentation/filesystems/locking.rst 
b/Documentation/filesystems/locking.rst
index b7dcc86c92a4..a5aa2046d48f 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 (*miscattr_set)(struct user_namespace *mnt_userns,
+   struct dentry *dentry, struct miscattr *ma);
+   int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
 
 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
+miscattr_get:  no or exclusive
+miscattr_set:  exclusive
    =
 
 
diff --git a/Documentation/filesystems/vfs.rst 
b/Documentation/filesystems/vfs.rst
index 2049bbf5e388..f125ce6c3b47 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 (*miscattr_set)(struct user_namespace *mnt_userns,
+   struct dentry *dentry, struct miscattr *ma);
+   int (*miscattr_get)(struct dentry *dentry, struct miscattr *ma);
};
 
 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.
 
+``miscattr_get``
+   called on ioctl(FS_IOC_GETFLAGS) and ioctl(FS_IOC_FSGETXATTR) to
+   retrieve miscellaneous filesystem 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().
+
+``miscattr_set``
+   called on ioctl(FS_IOC_SETFLAGS) and ioctl(FS_IOC_FSSETXATTR) to
+   change miscellaneous filesystem 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..e5f3820809a4 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,311 @@ static int ioctl_file_dedupe_range(struct file *file,
return ret;
 }
 
+/**
+ * miscattr_fill_xflags - initialize miscattr with xflags
+ * @ma:miscattr pointer
+ * @xflags:FS_XFLAG_* flags
+ *
+ * Set ->fsx_xflags, ->xattr_valid and ->flags (translated xflags).  All
+ * other fields are zeroed.
+ */
+void miscattr_fill_xflags(struct miscattr *ma, u32 xflags)
+{
+   memset(ma, 0, sizeof(*ma));
+   ma->xattr_valid = true;
+   ma->fsx_xflags = xflags;
+   if (ma->fsx_xflags & FS_XFLAG_IMMUTABLE)
+   ma->flags |= FS_IMMUTABLE_FL;
+   if (ma->fsx_xflags & FS_XFLAG_APPEND)
+   ma->flags |= FS_APPEND_FL;
+   if (ma->fsx_xflags & FS_XFLAG_SYNC)
+   ma->flags |= FS_SYNC_FL;
+   if