From 1da91ea87aefe2c25b68c9f96947a9271ba6325d Mon Sep 17 00:00:00 2001 From: Al Viro Date: Fri, 31 May 2024 14:12:01 -0400 Subject: introduce fd_file(), convert all accessors to it. For any changes of struct fd representation we need to turn existing accesses to fields into calls of wrappers. Accesses to struct fd::flags are very few (3 in linux/file.h, 1 in net/socket.c, 3 in fs/overlayfs/file.c and 3 more in explicit initializers). Those can be dealt with in the commit converting to new layout; accesses to struct fd::file are too many for that. This commit converts (almost) all of f.file to fd_file(f). It's not entirely mechanical ('file' is used as a member name more than just in struct fd) and it does not even attempt to distinguish the uses in pointer context from those in boolean context; the latter will be eventually turned into a separate helper (fd_empty()). NOTE: mass conversion to fd_empty(), tempting as it might be, is a bad idea; better do that piecewise in commit that convert from fdget...() to CLASS(...). [conflicts in fs/fhandle.c, kernel/bpf/syscall.c, mm/memcontrol.c caught by git; fs/stat.c one got caught by git grep] [fs/xattr.c conflict] Reviewed-by: Christian Brauner Signed-off-by: Al Viro --- drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 8 ++++---- drivers/gpu/drm/drm_syncobj.c | 6 +++--- drivers/infiniband/core/ucma.c | 6 +++--- drivers/infiniband/core/uverbs_cmd.c | 8 ++++---- drivers/media/mc/mc-request.c | 6 +++--- drivers/media/rc/lirc_dev.c | 8 ++++---- drivers/vfio/group.c | 6 +++--- drivers/vfio/virqfd.c | 6 +++--- drivers/virt/acrn/irqfd.c | 6 +++--- drivers/xen/privcmd.c | 10 +++++----- 10 files changed, 35 insertions(+), 35 deletions(-) (limited to 'drivers') diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c index 863b2a34b2d6..a9298cb8d19a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c @@ -43,10 +43,10 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, uint32_t id; int r; - if (!f.file) + if (!fd_file(f)) return -EINVAL; - r = amdgpu_file_to_fpriv(f.file, &fpriv); + r = amdgpu_file_to_fpriv(fd_file(f), &fpriv); if (r) { fdput(f); return r; @@ -72,10 +72,10 @@ static int amdgpu_sched_context_priority_override(struct amdgpu_device *adev, struct amdgpu_ctx *ctx; int r; - if (!f.file) + if (!fd_file(f)) return -EINVAL; - r = amdgpu_file_to_fpriv(f.file, &fpriv); + r = amdgpu_file_to_fpriv(fd_file(f), &fpriv); if (r) { fdput(f); return r; diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index a0e94217b511..7fb31ca3b5fc 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -715,16 +715,16 @@ static int drm_syncobj_fd_to_handle(struct drm_file *file_private, struct fd f = fdget(fd); int ret; - if (!f.file) + if (!fd_file(f)) return -EINVAL; - if (f.file->f_op != &drm_syncobj_file_fops) { + if (fd_file(f)->f_op != &drm_syncobj_file_fops) { fdput(f); return -EINVAL; } /* take a reference to put in the idr */ - syncobj = f.file->private_data; + syncobj = fd_file(f)->private_data; drm_syncobj_get(syncobj); idr_preload(GFP_KERNEL); diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index 5f5ad8faf86e..dc57d07a1f45 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -1624,13 +1624,13 @@ static ssize_t ucma_migrate_id(struct ucma_file *new_file, /* Get current fd to protect against it being closed */ f = fdget(cmd.fd); - if (!f.file) + if (!fd_file(f)) return -ENOENT; - if (f.file->f_op != &ucma_fops) { + if (fd_file(f)->f_op != &ucma_fops) { ret = -EINVAL; goto file_put; } - cur_file = f.file->private_data; + cur_file = fd_file(f)->private_data; /* Validate current fd and prevent destruction of id. */ ctx = ucma_get_ctx(cur_file, cmd.id); diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index 1b3ea71f2c33..3f85575cf971 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -584,12 +584,12 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs) if (cmd.fd != -1) { /* search for file descriptor */ f = fdget(cmd.fd); - if (!f.file) { + if (!fd_file(f)) { ret = -EBADF; goto err_tree_mutex_unlock; } - inode = file_inode(f.file); + inode = file_inode(fd_file(f)); xrcd = find_xrcd(ibudev, inode); if (!xrcd && !(cmd.oflags & O_CREAT)) { /* no file descriptor. Need CREATE flag */ @@ -632,7 +632,7 @@ static int ib_uverbs_open_xrcd(struct uverbs_attr_bundle *attrs) atomic_inc(&xrcd->usecnt); } - if (f.file) + if (fd_file(f)) fdput(f); mutex_unlock(&ibudev->xrcd_tree_mutex); @@ -648,7 +648,7 @@ err: uobj_alloc_abort(&obj->uobject, attrs); err_tree_mutex_unlock: - if (f.file) + if (fd_file(f)) fdput(f); mutex_unlock(&ibudev->xrcd_tree_mutex); diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c index addb8f2d8939..e064914c476e 100644 --- a/drivers/media/mc/mc-request.c +++ b/drivers/media/mc/mc-request.c @@ -254,12 +254,12 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd) return ERR_PTR(-EBADR); f = fdget(request_fd); - if (!f.file) + if (!fd_file(f)) goto err_no_req_fd; - if (f.file->f_op != &request_fops) + if (fd_file(f)->f_op != &request_fops) goto err_fput; - req = f.file->private_data; + req = fd_file(f)->private_data; if (req->mdev != mdev) goto err_fput; diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c index 717c441b4a86..b8dfd530fab7 100644 --- a/drivers/media/rc/lirc_dev.c +++ b/drivers/media/rc/lirc_dev.c @@ -820,20 +820,20 @@ struct rc_dev *rc_dev_get_from_fd(int fd, bool write) struct lirc_fh *fh; struct rc_dev *dev; - if (!f.file) + if (!fd_file(f)) return ERR_PTR(-EBADF); - if (f.file->f_op != &lirc_fops) { + if (fd_file(f)->f_op != &lirc_fops) { fdput(f); return ERR_PTR(-EINVAL); } - if (write && !(f.file->f_mode & FMODE_WRITE)) { + if (write && !(fd_file(f)->f_mode & FMODE_WRITE)) { fdput(f); return ERR_PTR(-EPERM); } - fh = f.file->private_data; + fh = fd_file(f)->private_data; dev = fh->rc; get_device(&dev->dev); diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c index ded364588d29..95b336de8a17 100644 --- a/drivers/vfio/group.c +++ b/drivers/vfio/group.c @@ -112,7 +112,7 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group, return -EFAULT; f = fdget(fd); - if (!f.file) + if (!fd_file(f)) return -EBADF; mutex_lock(&group->group_lock); @@ -125,13 +125,13 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group, goto out_unlock; } - container = vfio_container_from_file(f.file); + container = vfio_container_from_file(fd_file(f)); if (container) { ret = vfio_container_attach_group(container, group); goto out_unlock; } - iommufd = iommufd_ctx_from_file(f.file); + iommufd = iommufd_ctx_from_file(fd_file(f)); if (!IS_ERR(iommufd)) { if (IS_ENABLED(CONFIG_VFIO_NOIOMMU) && group->type == VFIO_NO_IOMMU) diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c index 532269133801..d22881245e89 100644 --- a/drivers/vfio/virqfd.c +++ b/drivers/vfio/virqfd.c @@ -134,12 +134,12 @@ int vfio_virqfd_enable(void *opaque, INIT_WORK(&virqfd->flush_inject, virqfd_flush_inject); irqfd = fdget(fd); - if (!irqfd.file) { + if (!fd_file(irqfd)) { ret = -EBADF; goto err_fd; } - ctx = eventfd_ctx_fileget(irqfd.file); + ctx = eventfd_ctx_fileget(fd_file(irqfd)); if (IS_ERR(ctx)) { ret = PTR_ERR(ctx); goto err_ctx; @@ -171,7 +171,7 @@ int vfio_virqfd_enable(void *opaque, init_waitqueue_func_entry(&virqfd->wait, virqfd_wakeup); init_poll_funcptr(&virqfd->pt, virqfd_ptable_queue_proc); - events = vfs_poll(irqfd.file, &virqfd->pt); + events = vfs_poll(fd_file(irqfd), &virqfd->pt); /* * Check if there was an event already pending on the eventfd diff --git a/drivers/virt/acrn/irqfd.c b/drivers/virt/acrn/irqfd.c index d4ad211dce7a..9994d818bb7e 100644 --- a/drivers/virt/acrn/irqfd.c +++ b/drivers/virt/acrn/irqfd.c @@ -125,12 +125,12 @@ static int acrn_irqfd_assign(struct acrn_vm *vm, struct acrn_irqfd *args) INIT_WORK(&irqfd->shutdown, hsm_irqfd_shutdown_work); f = fdget(args->fd); - if (!f.file) { + if (!fd_file(f)) { ret = -EBADF; goto out; } - eventfd = eventfd_ctx_fileget(f.file); + eventfd = eventfd_ctx_fileget(fd_file(f)); if (IS_ERR(eventfd)) { ret = PTR_ERR(eventfd); goto fail; @@ -157,7 +157,7 @@ static int acrn_irqfd_assign(struct acrn_vm *vm, struct acrn_irqfd *args) mutex_unlock(&vm->irqfds_lock); /* Check the pending event in this stage */ - events = vfs_poll(f.file, &irqfd->pt); + events = vfs_poll(fd_file(f), &irqfd->pt); if (events & EPOLLIN) acrn_irqfd_inject(irqfd); diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c index 9563650dfbaf..54e4f285c0f4 100644 --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -959,12 +959,12 @@ static int privcmd_irqfd_assign(struct privcmd_irqfd *irqfd) INIT_WORK(&kirqfd->shutdown, irqfd_shutdown); f = fdget(irqfd->fd); - if (!f.file) { + if (!fd_file(f)) { ret = -EBADF; goto error_kfree; } - kirqfd->eventfd = eventfd_ctx_fileget(f.file); + kirqfd->eventfd = eventfd_ctx_fileget(fd_file(f)); if (IS_ERR(kirqfd->eventfd)) { ret = PTR_ERR(kirqfd->eventfd); goto error_fd_put; @@ -995,7 +995,7 @@ static int privcmd_irqfd_assign(struct privcmd_irqfd *irqfd) * Check if there was an event already pending on the eventfd before we * registered, and trigger it as if we didn't miss it. */ - events = vfs_poll(f.file, &kirqfd->pt); + events = vfs_poll(fd_file(f), &kirqfd->pt); if (events & EPOLLIN) irqfd_inject(kirqfd); @@ -1345,12 +1345,12 @@ static int privcmd_ioeventfd_assign(struct privcmd_ioeventfd *ioeventfd) return -ENOMEM; f = fdget(ioeventfd->event_fd); - if (!f.file) { + if (!fd_file(f)) { ret = -EBADF; goto error_kfree; } - kioeventfd->eventfd = eventfd_ctx_fileget(f.file); + kioeventfd->eventfd = eventfd_ctx_fileget(fd_file(f)); fdput(f); if (IS_ERR(kioeventfd->eventfd)) { -- cgit v1.2.3 From 88a2f6468d013ca1163490dbddfc95135d1c27a1 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Fri, 31 May 2024 15:45:12 -0400 Subject: struct fd: representation change 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 Signed-off-by: Al Viro --- drivers/infiniband/core/uverbs_cmd.c | 2 +- fs/overlayfs/file.c | 28 +++++++++++++++------------- fs/xfs/xfs_handle.c | 2 +- include/linux/file.h | 22 ++++++++++++++++------ kernel/events/core.c | 2 +- net/socket.c | 2 +- 6 files changed, 35 insertions(+), 23 deletions(-) (limited to 'drivers') diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c index 3f85575cf971..a4cce360df21 100644 --- a/drivers/infiniband/core/uverbs_cmd.c +++ b/drivers/infiniband/core/uverbs_cmd.c @@ -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)); diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c index c4963d0c5549..2b7a5a3a7a2f 100644 --- a/fs/overlayfs/file.c +++ b/fs/overlayfs/file.c @@ -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); diff --git a/fs/xfs/xfs_handle.c b/fs/xfs/xfs_handle.c index 7bcc4f519cb8..49e5e5f04e60 100644 --- a/fs/xfs/xfs_handle.c +++ b/fs/xfs/xfs_handle.c @@ -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; diff --git a/include/linux/file.h b/include/linux/file.h index 0f3f369f2450..eb28469b1c16 100644 --- a/include/linux/file.h +++ b/include/linux/file.h @@ -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); } diff --git a/kernel/events/core.c b/kernel/events/core.c index 17b19d3e74ba..fd2ac9c7fd77 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -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; diff --git a/net/socket.c b/net/socket.c index f77a42a74510..c0d4f5032374 100644 --- a/net/socket.c +++ b/net/socket.c @@ -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; -- cgit v1.2.3