Re: [PATCH v2 1/2] locking: Introduce __cleanup__ based guards

[Peter Zijlstra]

On Fri, May 26, 2023 at 02:54:40PM -0700, Linus Torvalds wrote:
> What's wrong with just writing it out:
> 
>     typedef struct fd guard_fdget_type_t;
>     static inline struct fd guard_fdget_init(int fd)
>     { return fdget(fd); }
>     static inline void guard_fdget_exit(struct fd fd)
>     { fdput(fd); }
> 

(wrong guard type, ptr_guard vs lock_guard) but yeah, I had this same
realization during breakfast. Clearly the brain had already left last
night.

Specifically, I think we want ptr_guard() here (and possibly fdnull for
__to_fd(0)) for things like do_sendfile() where a struct fd is
initialized late.

The below seems to compile...

--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -10,6 +10,7 @@
 #include <linux/types.h>
 #include <linux/posix_types.h>
 #include <linux/errno.h>
+#include <linux/guards.h>
 
 struct file;
 
@@ -45,6 +46,13 @@ static inline void fdput(struct fd fd)
 		fput(fd.file);
 }
 
+typedef struct fd ptr_guard_fdput_t;
+
+static inline void ptr_guard_fdput_cleanup(struct fd *fdp)
+{
+	fdput(*fdp);
+}
+
 extern struct file *fget(unsigned int fd);
 extern struct file *fget_raw(unsigned int fd);
 extern struct file *fget_task(struct task_struct *task, unsigned int fd);
@@ -58,6 +66,8 @@ static inline struct fd __to_fd(unsigned
 	return (struct fd){(struct file *)(v & ~3),v & 3};
 }
 
+#define fdnull	__to_fd(0)
+
 static inline struct fd fdget(unsigned int fd)
 {
 	return __to_fd(__fdget(fd));
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1180,7 +1180,8 @@ COMPAT_SYSCALL_DEFINE6(pwritev2, compat_ulong_t, fd,
 static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 		  	   size_t count, loff_t max)
 {
-	struct fd in, out;
+	ptr_guard(fdput, in) = fdnull;
+	ptr_guard(fdput, out) = fdnull;
 	struct inode *in_inode, *out_inode;
 	struct pipe_inode_info *opipe;
 	loff_t pos;
@@ -1191,35 +1192,35 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 	/*
 	 * Get input file, and verify that it is ok..
 	 */
-	retval = -EBADF;
 	in = fdget(in_fd);
 	if (!in.file)
-		goto out;
+		return -EBADF;
 	if (!(in.file->f_mode & FMODE_READ))
-		goto fput_in;
-	retval = -ESPIPE;
+		return -EBADF;
+
 	if (!ppos) {
 		pos = in.file->f_pos;
 	} else {
 		pos = *ppos;
 		if (!(in.file->f_mode & FMODE_PREAD))
-			goto fput_in;
+			return -ESPIPE;
 	}
+
 	retval = rw_verify_area(READ, in.file, &pos, count);
 	if (retval < 0)
-		goto fput_in;
+		return retval;
 	if (count > MAX_RW_COUNT)
 		count =  MAX_RW_COUNT;
 
 	/*
 	 * Get output file, and verify that it is ok..
 	 */
-	retval = -EBADF;
 	out = fdget(out_fd);
 	if (!out.file)
-		goto fput_in;
+		return -EBADF;
 	if (!(out.file->f_mode & FMODE_WRITE))
-		goto fput_out;
+		return -EBADF;
+
 	in_inode = file_inode(in.file);
 	out_inode = file_inode(out.file);
 	out_pos = out.file->f_pos;
@@ -1228,9 +1229,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 		max = min(in_inode->i_sb->s_maxbytes, out_inode->i_sb->s_maxbytes);
 
 	if (unlikely(pos + count > max)) {
-		retval = -EOVERFLOW;
 		if (pos >= max)
-			goto fput_out;
+			return -EOVERFLOW;
 		count = max - pos;
 	}
 
@@ -1249,7 +1249,7 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 	if (!opipe) {
 		retval = rw_verify_area(WRITE, out.file, &out_pos, count);
 		if (retval < 0)
-			goto fput_out;
+			return retval;
 		file_start_write(out.file);
 		retval = do_splice_direct(in.file, &pos, out.file, &out_pos,
 					  count, fl);
@@ -1278,11 +1278,6 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 	if (pos > max)
 		retval = -EOVERFLOW;
 
-fput_out:
-	fdput(out);
-fput_in:
-	fdput(in);
-out:
 	return retval;
 }