[RFC][PATCHES] struct fderr

[RFC][PATCHES] struct fderr

[Al Viro]

	overlayfs uses struct fd for the things where it does not quite
fit.  There we want not "file reference or nothing" - it's "file reference
or an error".  For pointers we can do that by use of ERR_PTR(); strictly
speaking, that's an abuse of C pointers, but the kernel does make sure
that the uppermost page worth of virtual addresses never gets mapped
and no architecture we care about has those as trap representations.
It might be possible to use the same trick for struct fd; however, for
most of the regular struct fd users that would be a pointless headache -
it would pessimize the code generation for no real benefit.  I considered
a possibility of using represenation of ERR_PTR(-EBADF) for empty struct
fd, but that's far from being consistent among the struct fd users and
that ends up with bad code generation even for the users that want to
treat empty as "fail with -EBADF".

	It's better to introduce a separate type, say, struct fderr.
Representable values:
	* borrowed file reference (address of struct file instance)
	* cloned file reference (address of struct file instance)
	* error (a small negative integer).

	With sane constructors we get rid of the field-by-field mess in
ovl_real_fdget{,_meta}() and have them serve as initializers, always
returning a valid struct fderr value.

	That results in mostly sane code; however, there are several
places where we run into an unpleasant problem - the intended scope
is nested inside inode_lock()/inode_unlock(), with problematic gotos.

	Possible solutions:
1) turn inode_lock() into guard-based thing.  No, with the side of
"fuck, no".  guard() has its uses, but
	* the area covered by it should be _very_ easy to see
	* it should not be mixed with gotos, now *OR* later, so
any subsequent changes in the area should remember not to use those.
	* it should never fall into hands of Markus-level entities.
inode_lock fails all those; guard-based variant _will_ be abstracted
away by well-meaning folks, and it will start spreading.  And existing
users are often long, have non-trivial amounts of goto-based cleanups
in them and lifetimes of the objects involved are not particularly
easy to convert to __cleanup-based variants (to put it very mildly).

We might eventually need to reconsider that, but for now the only sane
policy is "no guard-based variants of VFS locks".

2) turn the scopes into explicit compound statements.

3) take them into helper functions.

The series (in viro/vfs.git #work.fderr) tries both (2) and (3) (the
latter as incremental to the former); I would like to hear opinions
on the relative attractiveness of those variants, first and foremost
from overlayfs folks.  If (3) is preferred, the last two commits of
that branch will be collapsed together; if not - the last commit
simply gets dropped.

Please, review.  Individual patches are in followups.

Changes since the last time it had been posted:
	* s/fd_error/fd_err/, to avoid a conflict
	* s/FD_ERR/FDERR_ERR/, to have constructor names consistent
	* add a followup converting the problematic scopes into
helper calls.

Al Viro (3):
      introduce struct fderr, convert overlayfs uses to that
      experimental: convert fs/overlayfs/file.c to CLASS(...)
      [experimental] another way to deal with scopes for overlayfs real_fd-under-inode_lock

 fs/overlayfs/file.c  | 211 +++++++++++++++++++++++----------------------------
 include/linux/file.h |  37 +++++++--
 2 files changed, 128 insertions(+), 120 deletions(-)

1/3) introduce struct fderr, convert overlayfs uses to that

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
that way...

2/3) experimental: convert fs/overlayfs/file.c to CLASS(...)

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().

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.

3/3) [experimental] another way to deal with scopes for overlayfs real_fd-under-inode_lock

Takes the 4 scopes mentioned in the previous commit into helper
functions.  To be folded into the previous if both go in.

Re: [RFC][PATCHES] struct fderr

[Al Viro]

On Fri, Oct 04, 2024 at 12:45:34AM +0100, Al Viro wrote:
> 	* s/FD_ERR/FDERR_ERR/, to have constructor names consistent

Grrr...  That would be s/ERR_FD/ERR_FDERR/, sorry

introduce struct fderr, convert overlayfs uses to that

[Al Viro]

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
that way...

See fs/overlayfs/file.c for example of use.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 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

[PATCH 2/3] experimental: convert fs/overlayfs/file.c to CLASS(...)

[Al Viro]

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().

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@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 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

[PATCH 3/3] [experimental] another way to deal with scopes for overlayfs real_fd-under-inode_lock

[Al Viro]

[incremental to the previous]

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

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index a0ab981b13d9..e10a009d32e7 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -268,6 +268,15 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 				      &ctx);
 }
 
+static ssize_t ovl_write_locked(struct kiocb *iocb, struct iov_iter *iter, int ifl,
+				 struct backing_file_ctx *ctx)
+{
+	CLASS(fd_real, real)(ctx->user_file);
+	if (fd_empty(real))
+		return fd_err(real);
+	return backing_file_write_iter(fd_file(real), iter, iocb, ifl, ctx);
+}
+
 static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 {
 	struct file *file = iocb->ki_filp;
@@ -287,14 +296,6 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	/* Update mode */
 	ovl_copyattr(inode);
 
-	{
-
-	CLASS(fd_real, real)(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);
 
@@ -303,11 +304,8 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	 * this property in case it is set by the issuer.
 	 */
 	ifl &= ~IOCB_DIO_CALLER_COMP;
-	ret = backing_file_write_iter(fd_file(real), iter, iocb, ifl, &ctx);
-
-	}
+	ret = ovl_write_locked(iocb, iter, ifl, &ctx);
 
-out_unlock:
 	inode_unlock(inode);
 
 	return ret;
@@ -331,6 +329,16 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
 					&ctx);
 }
 
+static ssize_t ovl_splice_locked(struct pipe_inode_info *pipe, 
+				 loff_t *ppos, size_t len, unsigned int flags,
+				 struct backing_file_ctx *ctx)
+{
+	CLASS(fd_real, real)(ctx->user_file);
+	if (fd_empty(real))
+		return fd_err(real);
+	return backing_file_splice_write(pipe, fd_file(real), ppos, len, flags, ctx);
+}
+
 /*
  * Calling iter_file_splice_write() directly from overlay's f_op may deadlock
  * due to lock order inversion between pipe->mutex in iter_file_splice_write()
@@ -353,19 +361,7 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
 	inode_lock(inode);
 	/* Update mode */
 	ovl_copyattr(inode);
-
-	{
-
-	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);
-
-	}
-out_unlock:
+	ret = ovl_splice_locked(pipe, ppos, len, flags, &ctx);
 	inode_unlock(inode);
 
 	return ret;
@@ -409,25 +405,14 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
 	return backing_file_mmap(realfile, vma, &ctx);
 }
 
-static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
+static long ovl_fallocate_locked(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct inode *inode = file_inode(file);
 	const struct cred *old_cred;
 	int ret;
 
-	inode_lock(inode);
-	/* Update mode */
-	ovl_copyattr(inode);
-	ret = file_remove_privs(file);
-	if (ret)
-		goto out_unlock;
-	{
-
 	CLASS(fd_real, real)(file);
-	if (fd_empty(real)) {
-		ret = fd_err(real);
-		goto out_unlock;
-	}
+	if (fd_empty(real))
+		return fd_err(real);
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	ret = vfs_fallocate(fd_file(real), mode, offset, len);
@@ -435,9 +420,20 @@ static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len
 
 	/* Update size */
 	ovl_file_modified(file);
+	return ret;
+}
 
-	}
-out_unlock:
+static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
+{
+	struct inode *inode = file_inode(file);
+	int ret;
+
+	inode_lock(inode);
+	/* Update mode */
+	ovl_copyattr(inode);
+	ret = file_remove_privs(file);
+	if (!ret)
+		ret = ovl_fallocate_locked(file, mode, offset, len);
 	inode_unlock(inode);
 
 	return ret;
@@ -465,36 +461,28 @@ enum ovl_copyop {
 	OVL_DEDUPE,
 };
 
-static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
+static loff_t ovl_copyfile_locked(struct file *file_in, loff_t pos_in,
 			    struct file *file_out, loff_t pos_out,
 			    loff_t len, unsigned int flags, enum ovl_copyop op)
 {
-	struct inode *inode_out = file_inode(file_out);
 	const struct cred *old_cred;
 	loff_t ret;
 
-	inode_lock(inode_out);
 	if (op != OVL_DEDUPE) {
 		/* Update mode */
-		ovl_copyattr(inode_out);
+		ovl_copyattr(file_inode(file_out));
 		ret = file_remove_privs(file_out);
 		if (ret)
-			goto out_unlock;
+			return ret;
 	}
 
-	{
-
 	CLASS(fd_real, real_out)(file_out);
-	if (fd_empty(real_out)) {
-		ret = fd_err(real_out);
-		goto out_unlock;
-	}
+	if (fd_empty(real_out))
+		return fd_err(real_out);
 
 	CLASS(fd_real, real_in)(file_in);
-	if (fd_empty(real_in)) {
-		ret = fd_err(real_in);
-		goto out_unlock;
-	}
+	if (fd_empty(real_in))
+		return fd_err(real_in);
 
 	old_cred = ovl_override_creds(file_inode(file_out)->i_sb);
 	switch (op) {
@@ -518,10 +506,19 @@ static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
 
 	/* Update size */
 	ovl_file_modified(file_out);
+	return ret;
+}
 
-	}
+static loff_t ovl_copyfile(struct file *file_in, loff_t pos_in,
+			    struct file *file_out, loff_t pos_out,
+			    loff_t len, unsigned int flags, enum ovl_copyop op)
+{
+	struct inode *inode_out = file_inode(file_out);
+	loff_t ret;
 
-out_unlock:
+	inode_lock(inode_out);
+	ret = ovl_copyfile_locked(file_in, pos_in, file_out, pos_out,
+				  len, flags, op);
 	inode_unlock(inode_out);
 
 	return ret;
-- 
2.39.5

Re: [PATCH 2/3] experimental: convert fs/overlayfs/file.c to CLASS(...)

[Christian Brauner]

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@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 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
> 

Re: introduce struct fderr, convert overlayfs uses to that

[Christian Brauner]

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@zeniv.linux.org.uk>
> ---
>  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
> 

Re: [RFC][PATCHES] struct fderr

[Amir Goldstein]

On Fri, Oct 4, 2024 at 1:45 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
>         overlayfs uses struct fd for the things where it does not quite
> fit.  There we want not "file reference or nothing" - it's "file reference
> or an error".  For pointers we can do that by use of ERR_PTR(); strictly
> speaking, that's an abuse of C pointers, but the kernel does make sure
> that the uppermost page worth of virtual addresses never gets mapped
> and no architecture we care about has those as trap representations.
> It might be possible to use the same trick for struct fd; however, for
> most of the regular struct fd users that would be a pointless headache -
> it would pessimize the code generation for no real benefit.  I considered
> a possibility of using represenation of ERR_PTR(-EBADF) for empty struct
> fd, but that's far from being consistent among the struct fd users and
> that ends up with bad code generation even for the users that want to
> treat empty as "fail with -EBADF".
>
>         It's better to introduce a separate type, say, struct fderr.
> Representable values:
>         * borrowed file reference (address of struct file instance)
>         * cloned file reference (address of struct file instance)
>         * error (a small negative integer).
>
>         With sane constructors we get rid of the field-by-field mess in
> ovl_real_fdget{,_meta}() and have them serve as initializers, always
> returning a valid struct fderr value.
>
>         That results in mostly sane code; however, there are several
> places where we run into an unpleasant problem - the intended scope
> is nested inside inode_lock()/inode_unlock(), with problematic gotos.
>
>         Possible solutions:
> 1) turn inode_lock() into guard-based thing.  No, with the side of
> "fuck, no".  guard() has its uses, but
>         * the area covered by it should be _very_ easy to see
>         * it should not be mixed with gotos, now *OR* later, so
> any subsequent changes in the area should remember not to use those.
>         * it should never fall into hands of Markus-level entities.
> inode_lock fails all those; guard-based variant _will_ be abstracted
> away by well-meaning folks, and it will start spreading.  And existing
> users are often long, have non-trivial amounts of goto-based cleanups
> in them and lifetimes of the objects involved are not particularly
> easy to convert to __cleanup-based variants (to put it very mildly).
>
> We might eventually need to reconsider that, but for now the only sane
> policy is "no guard-based variants of VFS locks".
>
> 2) turn the scopes into explicit compound statements.
>
> 3) take them into helper functions.

4) Get rid of the cloned real file references

I was going to explain what I mean, but it was easier to write patches:

https://lore.kernel.org/linux-fsdevel/20241004102342.179434-1-amir73il@gmail.com/

(also at https://github.com/amir73il/linux/commits/ovl_real_file/)

After my patches, there is no longer any use for CLONED_FDERR()
in overlayfs code and there is no need for guarded real fd in
overlayfs file methods, so the value of this work for overlayfs is
reduced.

I have no objection to the fderr abstraction, but I don't think
that it will improve overlayfs code after my change. WDYT?

Thanks,
Amir.

Re: introduce struct fderr, convert overlayfs uses to that

[Amir Goldstein]

On Fri, Oct 4, 2024 at 1:47 AM Al Viro <viro@zeniv.linux.org.uk> 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
> that way...
>
> See fs/overlayfs/file.c for example of use.

I had already posted an alternative code for overlayfs, but in case this
is going to be used anyway in overlayfs or in another code, see some
comments below...

>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  fs/overlayfs/file.c  | 128 +++++++++++++++++++++----------------------
>  include/linux/file.h |  37 +++++++++++--
>  2 files changed, 95 insertions(+), 70 deletions(-)
>

[...]

> 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))


I suggest adding a fd_is_err(f) helper to rhyme with IS_ERR().

>  #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),   \

Should that be !IS_ERR_VALUE((f).word)?

> +                               struct fd: true) && \
> +                           ((f).word & FDPUT_FPUT) && (fput(fd_file(f)),0))
>

or better yet

#define fd_is_err(f) _Generic((f), \
                                struct fd: false, \
                                struct fderr: IS_ERR_VALUE((f).word))
#define fdput(f) (!fd_is_err(f) && ((f).word & FDPUT_FPUT) &&
(fput(fd_file(f)),0))

Thanks,
Amir.

Re: introduce struct fderr, convert overlayfs uses to that

[Al Viro]

On Fri, Oct 04, 2024 at 12:47:09PM +0200, Amir Goldstein wrote:

> I had already posted an alternative code for overlayfs, but in case this
> is going to be used anyway in overlayfs or in another code, see some
> comments below...

As far as I can see, the current #overlayfs-next kills the case for
struct fderr; we might eventually get a valid use for it, but for the
time being I'm going to strip the overlayfs-related parts of that branch
(obviously), fix the braino you've spotted in fdput() and archive the
branch in case it's ever needed.

> > +#define fd_empty(f)    _Generic((f), \
> > +                               struct fd: unlikely(!(f).word), \
> > +                               struct fderr: IS_ERR_VALUE((f).word))
> 
> 
> I suggest adding a fd_is_err(f) helper to rhyme with IS_ERR().

Umm...  Dropping fd_empty() for that one, you mean?

> > +#define fdput(f)       (void) (_Generic((f), \
> > +                               struct fderr: IS_ERR_VALUE((f).word),   \
> 
> Should that be !IS_ERR_VALUE((f).word)?

It should, thanks for spotting that braino.
 
> or better yet
> 
> #define fd_is_err(f) _Generic((f), \
>                                 struct fd: false, \
>                                 struct fderr: IS_ERR_VALUE((f).word))

I think that's a bad idea; too likely to spill into struct fd users,
with "it's never false here" being a nasty surprise.