]> Git Repo - linux.git/commitdiff
struct fd: representation change
authorAl Viro <[email protected]>
Fri, 31 May 2024 19:45:12 +0000 (15:45 -0400)
committerAl Viro <[email protected]>
Tue, 13 Aug 2024 02:01:05 +0000 (22:01 -0400)
We want the compiler to see that fdput() on empty instance
is a no-op.  The emptiness check is that file reference is NULL,
while fdput() is "fput() if FDPUT_FPUT is present in flags".
The reason why fdput() on empty instance is a no-op is something
compiler can't see - it's that we never generate instances with
NULL file reference combined with non-zero flags.

It's not that hard to deal with - the real primitives behind
fdget() et.al. are returning an unsigned long value, unpacked by (inlined)
__to_fd() into the current struct file * + int.  The lower bits are
used to store flags, while the rest encodes the pointer.  Linus suggested
that keeping this unsigned long around with the extractions done by inlined
accessors should generate a sane code and that turns out to be the case.
Namely, turning struct fd into a struct-wrapped unsinged long, with
        fd_empty(f) => unlikely(f.word == 0)
fd_file(f) => (struct file *)(f.word & ~3)
fdput(f) => if (f.word & 1) fput(fd_file(f))
ends up with compiler doing the right thing.  The cost is the patch
footprint, of course - we need to switch f.file to fd_file(f) all over
the tree, and it's not doable with simple search and replace; there are
false positives, etc.

Note that the sole member of that structure is an opaque
unsigned long - all accesses should be done via wrappers and I don't
want to use a name that would invite manual casts to file pointers,
etc.  The value of that member is equal either to (unsigned long)p | flags,
p being an address of some struct file instance, or to 0 for an empty fd.

For now the new predicate (fd_empty(f)) has no users; all the
existing checks have form (!fd_file(f)).  We will convert to fd_empty()
use later; here we only define it (and tell the compiler that it's
unlikely to return true).

This commit only deals with representation change; there will
be followups.

Reviewed-by: Christian Brauner <[email protected]>
Signed-off-by: Al Viro <[email protected]>
drivers/infiniband/core/uverbs_cmd.c
fs/overlayfs/file.c
fs/xfs/xfs_handle.c
include/linux/file.h
kernel/events/core.c
net/socket.c

index 3f85575cf9715bf0197e8748c1aade029b2778a8..a4cce360df21789096042f4593540c946e977a55 100644 (file)
@@ -572,7 +572,7 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs)
        struct inode                   *inode = NULL;
        int                             new_xrcd = 0;
        struct ib_device *ib_dev;
-       struct fd f = {};
+       struct fd f = EMPTY_FD;
        int ret;
 
        ret = uverbs_request(attrs, &cmd, sizeof(cmd));
index c4963d0c55492868dc06beeb6bbec6a621d02ea1..2b7a5a3a7a2fed1bedea6173a101045b04e83a2a 100644 (file)
@@ -93,11 +93,11 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
                               bool allow_meta)
 {
        struct dentry *dentry = file_dentry(file);
+       struct file *realfile = file->private_data;
        struct path realpath;
        int err;
 
-       real->flags = 0;
-       real->file = file->private_data;
+       real->word = (unsigned long)realfile;
 
        if (allow_meta) {
                ovl_path_real(dentry, &realpath);
@@ -113,16 +113,17 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
                return -EIO;
 
        /* Has it been copied up since we'd opened it? */
-       if (unlikely(file_inode(real->file) != d_inode(realpath.dentry))) {
-               real->flags = FDPUT_FPUT;
-               real->file = ovl_open_realfile(file, &realpath);
-
-               return PTR_ERR_OR_ZERO(real->file);
+       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)ovl_open_realfile(file, &realpath) | FDPUT_FPUT;
+               return 0;
        }
 
        /* Did the flags change since open? */
-       if (unlikely((file->f_flags ^ real->file->f_flags) & ~OVL_OPEN_FLAGS))
-               return ovl_change_flags(real->file, file->f_flags);
+       if (unlikely((file->f_flags ^ realfile->f_flags) & ~OVL_OPEN_FLAGS))
+               return ovl_change_flags(realfile, file->f_flags);
 
        return 0;
 }
@@ -130,10 +131,11 @@ static int ovl_real_fdget_meta(const struct file *file, struct fd *real,
 static int ovl_real_fdget(const struct file *file, struct fd *real)
 {
        if (d_is_dir(file_dentry(file))) {
-               real->flags = 0;
-               real->file = ovl_dir_real_file(file, false);
-
-               return PTR_ERR_OR_ZERO(real->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;
        }
 
        return ovl_real_fdget_meta(file, real, false);
index 7bcc4f519cb838ec8a906481421212bf2eb073c2..49e5e5f04e60339100a67e61a0e21d22f5d48204 100644 (file)
@@ -85,7 +85,7 @@ xfs_find_handle(
        int                     hsize;
        xfs_handle_t            handle;
        struct inode            *inode;
-       struct fd               f = {NULL};
+       struct fd               f = EMPTY_FD;
        struct path             path;
        int                     error;
        struct xfs_inode        *ip;
index 0f3f369f24501bef3c40120a9756015cf0837233..eb28469b1c161204b4eccf8568fc967fc8e940e2 100644 (file)
@@ -35,18 +35,28 @@ static inline void fput_light(struct file *file, int fput_needed)
                fput(file);
 }
 
+/* either a reference to struct file + flags
+ * (cloned vs. borrowed, pos locked), with
+ * flags stored in lower bits of value,
+ * or empty (represented by 0).
+ */
 struct fd {
-       struct file *file;
-       unsigned int flags;
+       unsigned long word;
 };
 #define FDPUT_FPUT       1
 #define FDPUT_POS_UNLOCK 2
 
-#define fd_file(f) ((f).file)
+#define fd_file(f) ((struct file *)((f).word & ~(FDPUT_FPUT|FDPUT_POS_UNLOCK)))
+static inline bool fd_empty(struct fd f)
+{
+       return unlikely(!f.word);
+}
+
+#define EMPTY_FD (struct fd){0}
 
 static inline void fdput(struct fd fd)
 {
-       if (fd.flags & FDPUT_FPUT)
+       if (fd.word & FDPUT_FPUT)
                fput(fd_file(fd));
 }
 
@@ -60,7 +70,7 @@ extern void __f_unlock_pos(struct file *);
 
 static inline struct fd __to_fd(unsigned long v)
 {
-       return (struct fd){(struct file *)(v & ~3),v & 3};
+       return (struct fd){v};
 }
 
 static inline struct fd fdget(unsigned int fd)
@@ -80,7 +90,7 @@ static inline struct fd fdget_pos(int fd)
 
 static inline void fdput_pos(struct fd f)
 {
-       if (f.flags & FDPUT_POS_UNLOCK)
+       if (f.word & FDPUT_POS_UNLOCK)
                __f_unlock_pos(fd_file(f));
        fdput(f);
 }
index 17b19d3e74ba4bf6da68303ba14efd493c074783..fd2ac9c7fd7733c88c51a603fdc1d2b62fe411a6 100644 (file)
@@ -12474,7 +12474,7 @@ SYSCALL_DEFINE5(perf_event_open,
        struct perf_event_attr attr;
        struct perf_event_context *ctx;
        struct file *event_file = NULL;
-       struct fd group = {NULL, 0};
+       struct fd group = EMPTY_FD;
        struct task_struct *task = NULL;
        struct pmu *pmu;
        int event_fd;
index f77a42a7451048a46f0bf95042f674f1ab9444f3..c0d4f5032374983cf05c826ca0cb58bffa25d9aa 100644 (file)
@@ -559,7 +559,7 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
        if (fd_file(f)) {
                sock = sock_from_file(fd_file(f));
                if (likely(sock)) {
-                       *fput_needed = f.flags & FDPUT_FPUT;
+                       *fput_needed = f.word & FDPUT_FPUT;
                        return sock;
                }
                *err = -ENOTSOCK;
This page took 0.082311 seconds and 4 git commands to generate.