On Fri, Oct 04, 2024 at 12:47:32AM GMT, Al Viro wrote:
> Similar to struct fd; unlike struct fd, it can represent
> error values.
>
> Accessors:
>
> * fd_empty(f): true if f represents an error
> * fd_file(f): just as for struct fd it yields a pointer to
> struct file if fd_empty(f) is false. If
> fd_empty(f) is true, fd_file(f) is guaranteed
> _not_ to be an address of any object (IS_ERR()
> will be true in that case)
> * fd_err(f): if f represents an error, returns that error,
> otherwise the return value is junk.
>
> Constructors:
>
> * ERR_FDERR(-E...): an instance encoding given error [ERR_FDERR, perhaps?]
> * BORROWED_FDERR(file): if file points to a struct file instance,
> return a struct fderr representing that file
> reference with no flags set.
> if file is an ERR_PTR(-E...), return a struct
> fderr representing that error.
> file MUST NOT be NULL.
> * CLONED_FDERR(file): similar, but in case when file points to
> a struct file instance, set FDPUT_FPUT in flags.
>
> Same destructor as for struct fd; I'm not entirely convinced that
> playing with _Generic is a good idea here, but for now let's go
Why? It's exactly what it's very useful for and I've used it myself this
way. I think that's good use of _Generic.
> that way...
>
> See fs/overlayfs/file.c for example of use.
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> fs/overlayfs/file.c | 128 +++++++++++++++++++++----------------------
> include/linux/file.h | 37 +++++++++++--
> 2 files changed, 95 insertions(+), 70 deletions(-)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 4504493b20be..c711fa5d802f 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -89,56 +89,46 @@ static int ovl_change_flags(struct file *file, unsigned int flags)
> return 0;
> }
>
> -static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
> - bool allow_meta)
> +static struct fderr ovl_real_fdget_meta(const struct file *file, bool allow_meta)
> {
> struct dentry *dentry = file_dentry(file);
> struct file *realfile = file->private_data;
> struct path realpath;
> int err;
>
> - real->word = (unsigned long)realfile;
> -
> if (allow_meta) {
> ovl_path_real(dentry, &realpath);
> } else {
> /* lazy lookup and verify of lowerdata */
> err = ovl_verify_lowerdata(dentry);
> if (err)
> - return err;
> + return ERR_FDERR(err);
>
> ovl_path_realdata(dentry, &realpath);
> }
> if (!realpath.dentry)
> - return -EIO;
> + return ERR_FDERR(-EIO);
>
> /* Has it been copied up since we'd opened it? */
> - if (unlikely(file_inode(realfile) != d_inode(realpath.dentry))) {
> - struct file *f = ovl_open_realfile(file, &realpath);
> - if (IS_ERR(f))
> - return PTR_ERR(f);
> - real->word = (unsigned long)f | FDPUT_FPUT;
> - return 0;
> - }
> + if (unlikely(file_inode(realfile) != d_inode(realpath.dentry)))
> + return CLONED_FDERR(ovl_open_realfile(file, &realpath));
>
> /* Did the flags change since open? */
> - if (unlikely((file->f_flags ^ realfile->f_flags) & ~OVL_OPEN_FLAGS))
> - return ovl_change_flags(realfile, file->f_flags);
> + if (unlikely((file->f_flags ^ realfile->f_flags) & ~OVL_OPEN_FLAGS)) {
> + err = ovl_change_flags(realfile, file->f_flags);
> + if (err)
> + return ERR_FDERR(err);
> + }
>
> - return 0;
> + return BORROWED_FDERR(realfile);
> }
>
> -static int ovl_real_fdget(const struct file *file, struct fd *real)
> +static struct fderr ovl_real_fdget(const struct file *file)
> {
> - if (d_is_dir(file_dentry(file))) {
> - struct file *f = ovl_dir_real_file(file, false);
> - if (IS_ERR(f))
> - return PTR_ERR(f);
> - real->word = (unsigned long)f;
> - return 0;
> - }
> + if (d_is_dir(file_dentry(file)))
> + return BORROWED_FDERR(ovl_dir_real_file(file, false));
>
> - return ovl_real_fdget_meta(file, real, false);
> + return ovl_real_fdget_meta(file, false);
> }
>
> static int ovl_open(struct inode *inode, struct file *file)
> @@ -183,7 +173,7 @@ static int ovl_release(struct inode *inode, struct file *file)
> static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
> {
> struct inode *inode = file_inode(file);
> - struct fd real;
> + struct fderr real;
> const struct cred *old_cred;
> loff_t ret;
>
> @@ -199,9 +189,9 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
> return vfs_setpos(file, 0, 0);
> }
>
> - ret = ovl_real_fdget(file, &real);
> - if (ret)
> - return ret;
> + real = ovl_real_fdget(file);
> + if (fd_empty(real))
> + return fd_err(real);
>
> /*
> * Overlay file f_pos is the master copy that is preserved
> @@ -262,7 +252,7 @@ static void ovl_file_accessed(struct file *file)
> static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> {
> struct file *file = iocb->ki_filp;
> - struct fd real;
> + struct fderr real;
> ssize_t ret;
> struct backing_file_ctx ctx = {
> .cred = ovl_creds(file_inode(file)->i_sb),
> @@ -273,9 +263,9 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> if (!iov_iter_count(iter))
> return 0;
>
> - ret = ovl_real_fdget(file, &real);
> - if (ret)
> - return ret;
> + real = ovl_real_fdget(file);
> + if (fd_empty(real))
> + return fd_err(real);
>
> ret = backing_file_read_iter(fd_file(real), iter, iocb, iocb->ki_flags,
> &ctx);
> @@ -288,7 +278,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
> {
> struct file *file = iocb->ki_filp;
> struct inode *inode = file_inode(file);
> - struct fd real;
> + struct fderr real;
> ssize_t ret;
> int ifl = iocb->ki_flags;
> struct backing_file_ctx ctx = {
> @@ -304,9 +294,11 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
> /* Update mode */
> ovl_copyattr(inode);
>
> - ret = ovl_real_fdget(file, &real);
> - if (ret)
> + real = ovl_real_fdget(file);
> + if (fd_empty(real)) {
> + ret = fd_err(real);
> goto out_unlock;
> + }
>
> if (!ovl_should_sync(OVL_FS(inode->i_sb)))
> ifl &= ~(IOCB_DSYNC | IOCB_SYNC);
> @@ -329,7 +321,7 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
> struct pipe_inode_info *pipe, size_t len,
> unsigned int flags)
> {
> - struct fd real;
> + struct fderr real;
> ssize_t ret;
> struct backing_file_ctx ctx = {
> .cred = ovl_creds(file_inode(in)->i_sb),
> @@ -337,9 +329,9 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
> .accessed = ovl_file_accessed,
> };
>
> - ret = ovl_real_fdget(in, &real);
> - if (ret)
> - return ret;
> + real = ovl_real_fdget(in);
> + if (fd_empty(real))
> + return fd_err(real);
>
> ret = backing_file_splice_read(fd_file(real), ppos, pipe, len, flags, &ctx);
> fdput(real);
> @@ -358,7 +350,7 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
> static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
> loff_t *ppos, size_t len, unsigned int flags)
> {
> - struct fd real;
> + struct fderr real;
> struct inode *inode = file_inode(out);
> ssize_t ret;
> struct backing_file_ctx ctx = {
> @@ -371,9 +363,11 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
> /* Update mode */
> ovl_copyattr(inode);
>
> - ret = ovl_real_fdget(out, &real);
> - if (ret)
> + real = ovl_real_fdget(out);
> + if (fd_empty(real)) {
> + ret = fd_err(real);
> goto out_unlock;
> + }
>
> ret = backing_file_splice_write(pipe, fd_file(real), ppos, len, flags, &ctx);
> fdput(real);
> @@ -386,7 +380,7 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
>
> static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> {
> - struct fd real;
> + struct fderr real;
> const struct cred *old_cred;
> int ret;
>
> @@ -394,9 +388,9 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> if (ret <= 0)
> return ret;
>
> - ret = ovl_real_fdget_meta(file, &real, !datasync);
> - if (ret)
> - return ret;
> + real = ovl_real_fdget_meta(file, !datasync);
> + if (fd_empty(real))
> + return fd_err(real);
>
> /* Don't sync lower file for fear of receiving EROFS error */
> if (file_inode(fd_file(real)) == ovl_inode_upper(file_inode(file))) {
> @@ -425,7 +419,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
> static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> {
> struct inode *inode = file_inode(file);
> - struct fd real;
> + struct fderr real;
> const struct cred *old_cred;
> int ret;
>
> @@ -435,10 +429,11 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
> ret = file_remove_privs(file);
> if (ret)
> goto out_unlock;
> -
> - ret = ovl_real_fdget(file, &real);
> - if (ret)
> + real = ovl_real_fdget(file);
> + if (fd_empty(real)) {
> + ret = fd_err(real);
> goto out_unlock;
> + }
>
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> ret = vfs_fallocate(fd_file(real), mode, offset, len);
> @@ -457,13 +452,13 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
>
> static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
> {
> - struct fd real;
> + struct fderr real;
> const struct cred *old_cred;
> int ret;
>
> - ret = ovl_real_fdget(file, &real);
> - if (ret)
> - return ret;
> + real = ovl_real_fdget(file);
> + if (fd_empty(real))
> + return fd_err(real);
>
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> ret = vfs_fadvise(fd_file(real), offset, len, advice);
> @@ -485,7 +480,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> loff_t len, unsigned int flags, enum ovl_copyop op)
> {
> struct inode *inode_out = file_inode(file_out);
> - struct fd real_in, real_out;
> + struct fderr real_in, real_out;
> const struct cred *old_cred;
> loff_t ret;
>
> @@ -498,13 +493,16 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> goto out_unlock;
> }
>
> - ret = ovl_real_fdget(file_out, &real_out);
> - if (ret)
> + real_out = ovl_real_fdget(file_out);
> + if (fd_empty(real_out)) {
> + ret = fd_err(real_out);
> goto out_unlock;
> + }
>
> - ret = ovl_real_fdget(file_in, &real_in);
> - if (ret) {
> + real_in = ovl_real_fdget(file_in);
> + if (fd_empty(real_in)) {
> fdput(real_out);
> + ret = fd_err(real_in);
> goto out_unlock;
> }
>
> @@ -577,13 +575,13 @@ static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,
>
> static int ovl_flush(struct file *file, fl_owner_t id)
> {
> - struct fd real;
> + struct fderr real;
> const struct cred *old_cred;
> - int err;
> + int err = 0;
>
> - err = ovl_real_fdget(file, &real);
> - if (err)
> - return err;
> + real = ovl_real_fdget(file);
> + if (fd_empty(real))
> + return fd_err(real);
>
> if (fd_file(real)->f_op->flush) {
> old_cred = ovl_override_creds(file_inode(file)->i_sb);
> diff --git a/include/linux/file.h b/include/linux/file.h
> index f98de143245a..d85352523368 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -44,13 +44,26 @@ static inline void fput_light(struct file *file, int fput_needed)
> struct fd {
> unsigned long word;
> };
> +
> +/* either a reference to struct file + flags
> + * (cloned vs. borrowed, pos locked), with
> + * flags stored in lower bits of value,
> + * or an error (represented by small negative value).
> + */
> +struct fderr {
> + unsigned long word;
> +};
> +
> #define FDPUT_FPUT 1
> #define FDPUT_POS_UNLOCK 2
>
> +#define fd_empty(f) _Generic((f), \
> + struct fd: unlikely(!(f).word), \
> + struct fderr: IS_ERR_VALUE((f).word))
> #define fd_file(f) ((struct file *)((f).word & ~(FDPUT_FPUT|FDPUT_POS_UNLOCK)))
> -static inline bool fd_empty(struct fd f)
> +static inline long fd_err(struct fderr f)
> {
> - return unlikely(!f.word);
> + return (long)f.word;
> }
>
> #define EMPTY_FD (struct fd){0}
> @@ -63,11 +76,25 @@ static inline struct fd CLONED_FD(struct file *f)
> return (struct fd){(unsigned long)f | FDPUT_FPUT};
> }
>
> -static inline void fdput(struct fd fd)
> +static inline struct fderr ERR_FDERR(long n)
> +{
> + return (struct fderr){(unsigned long)n};
> +}
> +static inline struct fderr BORROWED_FDERR(struct file *f)
> {
> - if (fd.word & FDPUT_FPUT)
> - fput(fd_file(fd));
> + return (struct fderr){(unsigned long)f};
> }
> +static inline struct fderr CLONED_FDERR(struct file *f)
> +{
> + if (IS_ERR(f))
> + return BORROWED_FDERR(f);
> + return (struct fderr){(unsigned long)f | FDPUT_FPUT};
> +}
> +
> +#define fdput(f) (void) (_Generic((f), \
> + struct fderr: IS_ERR_VALUE((f).word), \
> + struct fd: true) && \
> + ((f).word & FDPUT_FPUT) && (fput(fd_file(f)),0))
>
> extern struct file *fget(unsigned int fd);
> extern struct file *fget_raw(unsigned int fd);
> --
> 2.39.5
>
