On Fri, Oct 04, 2024 at 12:48:08AM GMT, Al Viro wrote:
> There are four places where we end up adding an extra scope
> covering just the range from constructor to destructor;
I don't think this is something to worry about. While there are cases
where the width of the scope might make a meaningful difference I doubt
it does here.
> not sure if that's the best way to handle that.
>
> The functions in question are ovl_write_iter(), ovl_splice_write(),
> ovl_fadvise() and ovl_copyfile().
>
> I still don't like the way we have to deal with the scopes, but...
> use of guard() for inode_lock()/inode_unlock() is a gutter too deep,
> as far as I'm concerned.
>
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> fs/overlayfs/file.c | 72 ++++++++++++++++++---------------------------
> 1 file changed, 29 insertions(+), 43 deletions(-)
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index c711fa5d802f..a0ab981b13d9 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -131,6 +131,8 @@ static struct fderr ovl_real_fdget(const struct file *file)
> return ovl_real_fdget_meta(file, false);
> }
>
> +DEFINE_CLASS(fd_real, struct fderr, fdput(_T), ovl_real_fdget(file), struct file *file)
> +
> static int ovl_open(struct inode *inode, struct file *file)
> {
> struct dentry *dentry = file_dentry(file);
> @@ -173,7 +175,6 @@ 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 fderr real;
> const struct cred *old_cred;
> loff_t ret;
>
> @@ -189,7 +190,7 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
> return vfs_setpos(file, 0, 0);
> }
>
> - real = ovl_real_fdget(file);
> + CLASS(fd_real, real)(file);
> if (fd_empty(real))
> return fd_err(real);
>
> @@ -210,8 +211,6 @@ static loff_t ovl_llseek(struct file *file, loff_t offset, int whence)
> file->f_pos = fd_file(real)->f_pos;
> ovl_inode_unlock(inode);
>
> - fdput(real);
> -
> return ret;
> }
>
> @@ -252,8 +251,6 @@ 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 fderr real;
> - ssize_t ret;
> struct backing_file_ctx ctx = {
> .cred = ovl_creds(file_inode(file)->i_sb),
> .user_file = file,
> @@ -263,22 +260,18 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> if (!iov_iter_count(iter))
> return 0;
>
> - real = ovl_real_fdget(file);
> + CLASS(fd_real, real)(file);
> if (fd_empty(real))
> return fd_err(real);
>
> - ret = backing_file_read_iter(fd_file(real), iter, iocb, iocb->ki_flags,
> - &ctx);
> - fdput(real);
> -
> - return ret;
> + return backing_file_read_iter(fd_file(real), iter, iocb, iocb->ki_flags,
> + &ctx);
> }
>
> 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 fderr real;
> ssize_t ret;
> int ifl = iocb->ki_flags;
> struct backing_file_ctx ctx = {
> @@ -294,7 +287,9 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
> /* Update mode */
> ovl_copyattr(inode);
>
> - real = ovl_real_fdget(file);
> + {
> +
> + CLASS(fd_real, real)(file);
> if (fd_empty(real)) {
> ret = fd_err(real);
> goto out_unlock;
> @@ -309,7 +304,8 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
> */
> ifl &= ~IOCB_DIO_CALLER_COMP;
> ret = backing_file_write_iter(fd_file(real), iter, iocb, ifl, &ctx);
> - fdput(real);
> +
> + }
>
> out_unlock:
> inode_unlock(inode);
> @@ -321,22 +317,18 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
> struct pipe_inode_info *pipe, size_t len,
> unsigned int flags)
> {
> - struct fderr real;
> - ssize_t ret;
> + CLASS(fd_real, real)(in);
> struct backing_file_ctx ctx = {
> .cred = ovl_creds(file_inode(in)->i_sb),
> .user_file = in,
> .accessed = ovl_file_accessed,
> };
>
> - 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);
> -
> - return ret;
> + return backing_file_splice_read(fd_file(real), ppos, pipe, len, flags,
> + &ctx);
> }
>
> /*
> @@ -350,7 +342,6 @@ 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 fderr real;
> struct inode *inode = file_inode(out);
> ssize_t ret;
> struct backing_file_ctx ctx = {
> @@ -363,15 +354,17 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
> /* Update mode */
> ovl_copyattr(inode);
>
> - real = ovl_real_fdget(out);
> + {
> +
> + CLASS(fd_real, real)(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);
>
> + }
> out_unlock:
> inode_unlock(inode);
>
> @@ -419,7 +412,6 @@ 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 fderr real;
> const struct cred *old_cred;
> int ret;
>
> @@ -429,7 +421,9 @@ 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;
> - real = ovl_real_fdget(file);
> + {
> +
> + CLASS(fd_real, real)(file);
> if (fd_empty(real)) {
> ret = fd_err(real);
> goto out_unlock;
> @@ -442,8 +436,7 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
> /* Update size */
> ovl_file_modified(file);
>
> - fdput(real);
> -
> + }
> out_unlock:
> inode_unlock(inode);
>
> @@ -452,11 +445,10 @@ 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 fderr real;
> + CLASS(fd_real, real)(file);
> const struct cred *old_cred;
> int ret;
>
> - real = ovl_real_fdget(file);
> if (fd_empty(real))
> return fd_err(real);
>
> @@ -464,8 +456,6 @@ static int ovl_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
> ret = vfs_fadvise(fd_file(real), offset, len, advice);
> revert_creds(old_cred);
>
> - fdput(real);
> -
> return ret;
> }
>
> @@ -480,7 +470,6 @@ 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 fderr real_in, real_out;
> const struct cred *old_cred;
> loff_t ret;
>
> @@ -493,15 +482,16 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> goto out_unlock;
> }
>
> - real_out = ovl_real_fdget(file_out);
> + {
> +
> + CLASS(fd_real, real_out)(file_out);
> if (fd_empty(real_out)) {
> ret = fd_err(real_out);
> goto out_unlock;
> }
>
> - real_in = ovl_real_fdget(file_in);
> + CLASS(fd_real, real_in)(file_in);
> if (fd_empty(real_in)) {
> - fdput(real_out);
> ret = fd_err(real_in);
> goto out_unlock;
> }
> @@ -529,8 +519,7 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
> /* Update size */
> ovl_file_modified(file_out);
>
> - fdput(real_in);
> - fdput(real_out);
> + }
>
> out_unlock:
> inode_unlock(inode_out);
> @@ -575,11 +564,10 @@ 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 fderr real;
> + CLASS(fd_real, real)(file);
> const struct cred *old_cred;
> int err = 0;
>
> - real = ovl_real_fdget(file);
> if (fd_empty(real))
> return fd_err(real);
>
> @@ -588,8 +576,6 @@ static int ovl_flush(struct file *file, fl_owner_t id)
> err = fd_file(real)->f_op->flush(fd_file(real), id);
> revert_creds(old_cred);
> }
> - fdput(real);
> -
> return err;
> }
>
> --
> 2.39.5
>
