| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240807-gecheckt-groll-94355086496f@brauner>
Date: Wed, 7 Aug 2024 12:03:08 +0200
From: Christian Brauner <brauner@...nel.org>
To: viro@...nel.org
Cc: linux-fsdevel@...r.kernel.org, amir73il@...il.com, bpf@...r.kernel.org,
cgroups@...r.kernel.org, kvm@...r.kernel.org, netdev@...r.kernel.org,
torvalds@...ux-foundation.org
Subject: Re: [PATCH 03/39] struct fd: representation change
On Tue, Jul 30, 2024 at 01:15:49AM GMT, viro@...nel.org wrote:
> From: Al Viro <viro@...iv.linux.org.uk>
>
> The absolute majority of instances comes from fdget() and its
> relatives; the underlying primitives actually return a struct file
> reference and a couple of flags encoded into an unsigned long - the lower
> two bits of file address are always zero, so we can stash the flags
> into those. On the way out we use __to_fd() to unpack that unsigned
> long into struct fd.
>
> Let's use that representation for struct fd itself - make it
> a structure with a single unsigned long member (.word), with the value
> equal either to (unsigned long)p | flags, p being an address of some
> struct file instance, or to 0 for an empty fd.
>
> Note that we never used a struct fd instance with NULL ->file
> and non-zero ->flags; the emptiness had been checked as (!f.file) and
> we expected e.g. fdput(empty) to be a no-op. With new representation
> we can use (!f.word) for emptiness check; that is enough for compiler
> to figure out that (f.word & FDPUT_FPUT) will be false and that fdput(f)
> will be a no-op in such case.
>
> 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.
>
> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> ---
> 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(-)
>
> 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..bdd6e1766839 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 & ~3))
Minor thing but it would be nice if you'd use the macros for this
instead of the raw number:
#define fd_file(f) ((struct file *)((f).word & ~(FDPUT_FPUT | FDPUT_POS_UNLOCK))
Reviewed-by: Christian Brauner <brauner@...nel.org>
Powered by blists - more mailing lists