diff mbox series

[08/39] experimental: convert fs/overlayfs/file.c to CLASS(...)

Message ID 20240730051625.14349-8-viro@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series [01/39] memcg_write_event_control(): fix a user-triggerable oops | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply, async

Commit Message

Al Viro July 30, 2024, 5:15 a.m. UTC 
From: Al Viro <viro@zeniv.linux.org.uk>

There are four places where we end up adding an extra scope
covering just the range from constructor to destructor;
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().

This is very likely *NOT* the final form of that thing - it
needs to be discussed.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/overlayfs/file.c | 72 ++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 43 deletions(-)

Comments

Josef Bacik July 30, 2024, 7:10 p.m. UTC | #1 
On Tue, Jul 30, 2024 at 01:15:54AM -0400, viro@kernel.org wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> There are four places where we end up adding an extra scope
> covering just the range from constructor to destructor;
> 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().
> 
> This is very likely *NOT* the final form of that thing - it
> needs to be discussed.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  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 4b9e145bc7b8..a2911c632137 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -132,6 +132,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);
> @@ -174,7 +176,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;
>  
> @@ -190,7 +191,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_error(real);
>  
> @@ -211,8 +212,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;
>  }
>  
> @@ -253,8 +252,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,
> @@ -264,22 +261,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_error(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 = {
> @@ -295,7 +288,9 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
>  	/* Update mode */
>  	ovl_copyattr(inode);
>  
> -	real = ovl_real_fdget(file);
> +	{

Is this what we want to do from a code cleanliness standpoint?  This feels
pretty ugly to me, I feal like it would be better to have something like

scoped_class(fd_real, real) {
	// code
}

rather than the {} at the same indent level as the underlying block.

I don't feel super strongly about this, but I do feel like we need to either
explicitly say "this is the way/an acceptable way to do this" from a code
formatting standpoint, or we need to come up with a cleaner way of representing
the scoped area.  Thanks,

Josef
Al Viro July 30, 2024, 9:12 p.m. UTC | #2 
On Tue, Jul 30, 2024 at 03:10:25PM -0400, Josef Bacik wrote:
> On Tue, Jul 30, 2024 at 01:15:54AM -0400, viro@kernel.org wrote:
> > From: Al Viro <viro@zeniv.linux.org.uk>
> > 
> > There are four places where we end up adding an extra scope
> > covering just the range from constructor to destructor;
> > 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().
> > 
> > This is very likely *NOT* the final form of that thing - it
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > needs to be discussed.

> Is this what we want to do from a code cleanliness standpoint?  This feels
> pretty ugly to me, I feal like it would be better to have something like
> 
> scoped_class(fd_real, real) {
> 	// code
> }
> 
> rather than the {} at the same indent level as the underlying block.
> 
> I don't feel super strongly about this, but I do feel like we need to either
> explicitly say "this is the way/an acceptable way to do this" from a code
> formatting standpoint, or we need to come up with a cleaner way of representing
> the scoped area.

That's a bit painful in these cases - sure, we can do something like
	scoped_class(fd_real, real)(file) {
		if (fd_empty(fd_real)) {
			ret = fd_error(real);
			break;
		}
		old_cred = ovl_override_creds(file_inode(file)->i_sb);
		ret = vfs_fallocate(fd_file(real), mode, offset, len);
		revert_creds(old_cred);

		/* Update size */
		ovl_file_modified(file);  
	}
but that use of break would need to be documented.  And IMO anything like
        scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
			   &task->signal->cred_guard_mutex) {
is just distasteful ;-/  Control flow should _not_ be hidden that way;
it's hard on casual reader.

The variant I'd put in there is obviously not suitable for merge - we need
something else, the question is what that something should be...
Josef Bacik July 31, 2024, 9:11 p.m. UTC | #3 
On Tue, Jul 30, 2024 at 10:12:25PM +0100, Al Viro wrote:
> On Tue, Jul 30, 2024 at 03:10:25PM -0400, Josef Bacik wrote:
> > On Tue, Jul 30, 2024 at 01:15:54AM -0400, viro@kernel.org wrote:
> > > From: Al Viro <viro@zeniv.linux.org.uk>
> > > 
> > > There are four places where we end up adding an extra scope
> > > covering just the range from constructor to destructor;
> > > 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().
> > > 
> > > This is very likely *NOT* the final form of that thing - it
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > needs to be discussed.
> 

Fair, I think I misunderstood what you were unhappy with in that code.

> > Is this what we want to do from a code cleanliness standpoint?  This feels
> > pretty ugly to me, I feal like it would be better to have something like
> > 
> > scoped_class(fd_real, real) {
> > 	// code
> > }
> > 
> > rather than the {} at the same indent level as the underlying block.
> > 
> > I don't feel super strongly about this, but I do feel like we need to either
> > explicitly say "this is the way/an acceptable way to do this" from a code
> > formatting standpoint, or we need to come up with a cleaner way of representing
> > the scoped area.
> 
> That's a bit painful in these cases - sure, we can do something like
> 	scoped_class(fd_real, real)(file) {
> 		if (fd_empty(fd_real)) {
> 			ret = fd_error(real);
> 			break;
> 		}
> 		old_cred = ovl_override_creds(file_inode(file)->i_sb);
> 		ret = vfs_fallocate(fd_file(real), mode, offset, len);
> 		revert_creds(old_cred);
> 
> 		/* Update size */
> 		ovl_file_modified(file);  
> 	}
> but that use of break would need to be documented.  And IMO anything like
>         scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
> 			   &task->signal->cred_guard_mutex) {
> is just distasteful ;-/  Control flow should _not_ be hidden that way;
> it's hard on casual reader.
> 
> The variant I'd put in there is obviously not suitable for merge - we need
> something else, the question is what that something should be...

I went and looked at our c++ codebase to see what they do here, and it appears
that this is the accepted norm for this style of scoped variables

{
	CLASS(fd_real, real_out)(file_out);
	// blah blah
}

Looking at our code guidelines this appears to be the widely accepted norm, and
I don't hate it.  I feel like this is more readable than the scoped_class()
idea, and is honestly the cleanest solution.  Thanks,

Josef
Christian Brauner Aug. 7, 2024, 10:23 a.m. UTC | #4 
On Tue, Jul 30, 2024 at 01:15:54AM GMT, viro@kernel.org wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> There are four places where we end up adding an extra scope
> covering just the range from constructor to destructor;
> not sure if that's the best way to handle that.

I think it's fine and not worth obsessing about it.
Reviewed-by: Christian Brauner <brauner@kernel.org>
diff mbox series

Patch

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 4b9e145bc7b8..a2911c632137 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -132,6 +132,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);
@@ -174,7 +176,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;
 
@@ -190,7 +191,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_error(real);
 
@@ -211,8 +212,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;
 }
 
@@ -253,8 +252,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,
@@ -264,22 +261,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_error(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 = {
@@ -295,7 +288,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_error(real);
 		goto out_unlock;
@@ -310,7 +305,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);
@@ -322,22 +318,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_error(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);
 }
 
 /*
@@ -351,7 +343,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 = {
@@ -364,15 +355,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_error(real);
 		goto out_unlock;
 	}
 
 	ret = backing_file_splice_write(pipe, fd_file(real), ppos, len, flags, &ctx);
-	fdput(real);
 
+	}
 out_unlock:
 	inode_unlock(inode);
 
@@ -420,7 +413,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;
 
@@ -430,7 +422,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_error(real);
 		goto out_unlock;
@@ -443,8 +437,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);
 
@@ -453,11 +446,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_error(real);
 
@@ -465,8 +457,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;
 }
 
@@ -481,7 +471,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;
 
@@ -494,15 +483,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_error(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_error(real_in);
 		goto out_unlock;
 	}
@@ -530,8 +520,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);
@@ -576,11 +565,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_error(real);
 
@@ -589,8 +577,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;
 }