Re: [PATCH v2 09/10] mm/shmem_luo: Allow retrieving tmpfs files with O_RDONLY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/shmem_luo: unnormalized flags passed to dentry_open() on O_RDONLY reopen
--

commit 18d00c8fbd0dd510d3e12bc0fbf7c761642eaf15
Author: David Matlack <dmatlack@google.com>

mm/shmem_luo: Allow retrieving tmpfs files with O_RDONLY

This commit allows userspace to retrieve preserved tmpfs files read-only by
passing O_RDONLY in struct liveupdate_shmem_retrieve_arg. It creates the
temporary file as writable via file_open_root() and then reopens it read-only
with dentry_open().

> diff --git a/mm/shmem_luo.c b/mm/shmem_luo.c
> index aba2bc1f18c52..0b4a9d6e2968c 100644
> --- a/mm/shmem_luo.c
> +++ b/mm/shmem_luo.c
[ ... ]
> @@ -649,7 +650,9 @@ static struct file *shmem_luo_create_memfd(const struct liveupdate_shmem_retriev
>  static struct file *shmem_luo_create_tmpfs_file(const struct liveupdate_shmem_retrieve_arg *arg,
>  						unsigned int seals)
>  {
> -	struct file *file;
> +	struct file *file, *ro_file;
> +	bool rdonly;
> +	int flags;
>  	int err;
>  
>  	if (!arg)
> @@ -673,10 +676,16 @@ static struct file *shmem_luo_create_tmpfs_file(const struct liveupdate_shmem_re
>  
>  	/*
>  	 * Create the file like open(O_TMPFILE) does, so that it gets the same
> -	 * flag checks, permission checks, owner and security labels.
> +	 * flag checks, permission checks, owner and security labels. open()
> +	 * requires write access for O_TMPFILE, so create the file writable and
> +	 * reopen it read-only below if that is what userspace asked for. LUO
> +	 * keeps the file until the session is finished, and a file opened for
> +	 * writing couldn't be executed until then.
>  	 */
> +	flags = O_TMPFILE | O_LARGEFILE | arg->flags;
> +	rdonly = (flags & O_ACCMODE) == O_RDONLY;
>  	file = file_open_root(&fd_file(dir)->f_path, ".",
> -			      O_TMPFILE | O_LARGEFILE | arg->flags, arg->mode);
> +			      flags | (rdonly ? O_RDWR : 0), arg->mode);
>  	if (IS_ERR(file))
>  		return file;
>  
> @@ -694,7 +703,12 @@ static struct file *shmem_luo_create_tmpfs_file(const struct liveupdate_shmem_re
>  	}
>  	SHMEM_I(file_inode(file))->seals = seals;
>  
> -	return file;
> +	if (!rdonly)
> +		return file;
> +
> +	ro_file = dentry_open(&file->f_path, flags, current_cred());
> +	fput(file);
> +	return ro_file;
>  }
>  
>  static int shmem_luo_retrieve(struct liveupdate_file_op_args *args)

[Severity: Medium]
When userspace retrieves a tmpfs file via LIVEUPDATE_SESSION_RETRIEVE_FD with
O_RDONLY in arg->flags, does passing the raw flags variable to dentry_open()
in shmem_luo_create_tmpfs_file() in mm/shmem_luo.c bypass the flag
normalization performed by file_open_root() -> build_open_flags()?

In fs/open.c, build_open_flags() strips O_CLOEXEC and ensures O_DSYNC is set
when __O_SYNC is present:

    u64 flags = how->flags;
    u64 strip = O_CLOEXEC;
    ...
    flags &= ~strip;
    ...
    if (flags & __O_SYNC)
        flags |= O_DSYNC;

Because dentry_open() -> alloc_empty_file() -> init_file() initializes
ro_file->f_flags directly from the passed flags and do_dentry_open() only
clears:

    f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | __O_REGULAR);
    f->f_iocb_flags = iocb_flags(f);

passing O_RDONLY | O_CLOEXEC in arg->flags leaves O_CLOEXEC in
ro_file->f_flags (visible via fcntl(fd, F_GETFL) and /proc/<pid>/fdinfo/<fd>),
and passing O_RDONLY | __O_SYNC without O_DSYNC leaves O_DSYNC and IOCB_DSYNC
unset on ro_file.

Should dentry_open() be passed (file->f_flags & ~O_ACCMODE) | O_RDONLY instead
of flags so that ro_file uses the normalized flags from file?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022225035.377445-1-dmatlack@google.com?part=9