Re: [PATCH 12/20] xfs: don't modify file and inode flags for shmem files

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux