On Mon, 29 Jan 2024, Christoph Hellwig wrote:
> shmem_file_setup is explicitly intended for a file that can be
> fully read and written by kernel users without restrictions. Don't
> poke into internals to change random flags in the file or inode.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/scrub/xfile.c | 17 +----------------
> 1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c
> index 7785afacf21809..7e915385ef0011 100644
> --- a/fs/xfs/scrub/xfile.c
> +++ b/fs/xfs/scrub/xfile.c
> @@ -68,28 +68,13 @@ xfile_create(
> if (!xf)
> return -ENOMEM;
>
> - xf->file = shmem_file_setup(description, isize, 0);
> + xf->file = shmem_kernel_file_setup(description, isize, 0);
I think you probably want to say VM_NORESERVE there, instead of 0.
I see from the (deleted) comment below that "We want a large sparse file",
and the VM_NORESERVE does become important on large sparse files. It is
how normal flles and memfds are set up (whereas SysV SHM says 0 to reserve
size at setup). It affects the Committed_AS seen in /proc/meminfo, and how
__vm_enough_memory() behaves (depending on /proc/sys/vm/overcommit_memory).
Hmm, I think mm/shmem.c is not prepared for the case of flags 0 there,
and then the file growing bigger than isize later on - I suspect that
Committed_AS will end up underflowing. shmem.c ought to be defensive
against that, sure, but I don't think such a case has come up before.
I see you have two calls to xfile_create(), one with what looks like
a known fixed non-0 isize, but one with isize 0 which will grow later.
> if (IS_ERR(xf->file)) {
> error = PTR_ERR(xf->file);
> goto out_xfile;
> }
>
> - /*
> - * We want a large sparse file that we can pread, pwrite, and seek.
> - * xfile users are responsible for keeping the xfile hidden away from
> - * all other callers, so we skip timestamp updates and security checks.
> - * Make the inode only accessible by root, just in case the xfile ever
> - * escapes.
> - */
> - xf->file->f_mode |= FMODE_PREAD | FMODE_PWRITE | FMODE_NOCMTIME |
> - FMODE_LSEEK;
> - xf->file->f_flags |= O_RDWR | O_LARGEFILE | O_NOATIME;
I've forgotten offhand how O_LARGEFILE propagates down to where a file
gets grown, and I realize that 32-bit is not your primary concern: but
please double-check that it works correctly without O_LARGEFILE now
(maybe the new folio ops avoid everywhere that largefile would be checked).
Hugh
> inode = file_inode(xf->file);
> - inode->i_flags |= S_PRIVATE | S_NOCMTIME | S_NOATIME;
> - inode->i_mode &= ~0177;
> - inode->i_uid = GLOBAL_ROOT_UID;
> - inode->i_gid = GLOBAL_ROOT_GID;
> -
> lockdep_set_class(&inode->i_rwsem, &xfile_i_mutex_key);
>
> trace_xfile_create(xf);
> --
> 2.39.2
