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

Peter Zijlstra
2023-05-27T08:58:24Z

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;
}