linux-kernel - Re: [PATCH 1/1] loop: sync filesystem cache before getting file size in get

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aJi3fFwlqb-SfHGg@infradead.org>
Date: Sun, 10 Aug 2025 08:15:08 -0700
From: Christoph Hellwig <hch@...radead.org>
To: Rajeev Mishra <rajeevm@....com>
Cc: axboe@...nel.dk, linux-block@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] loop: sync filesystem cache before getting file size
 in get_size()

On Thu, Aug 07, 2025 at 11:25:22PM +0000, Rajeev Mishra wrote:
> The get_size() function now uses vfs_getattr_nosec() with AT_STATX_SYNC_AS_STAT
> to ensure filesystem cache is synchronized before retrieving file size. This
> provides more accurate size information, especially when:
> 
> - The backing file size has been changed by another process
> - The file is on a network filesystem (NFS, CIFS, etc.)
> - The file is being modified concurrently
> - The most accurate size is needed for loop device setup
> 

Please wrap your commit messages at 73 (or apparently 75) lines.

Also 'syncing the cache' (what cache?) is at best an implementation
detail.  The VFS semantics simply are that you need a getattr to
retrieve the inode size, and the loop code fails to do this correctly.

> The implementation gracefully falls back to i_size_read() if vfs_getattr_nosec()
> fails, maintaining backward compatibility.

No need to fall back.  If vfs_getattr faills the file systems is
completely toast.

>  static int part_shift;
>  
> +/**
> + * get_size - calculate the effective size of a loop device
> + * @offset: offset into the backing file
> + * @sizelimit: user-specified size limit
> + * @file: the backing file
> + *
> + * Calculate the effective size of the loop device
> + *
> + * Returns: size in 512-byte sectors, or 0 if invalid
> + */

We don't really need a verbose kerneldoc for a static helper with 2
callers.

>  static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
>  {
> +	struct kstat stat;
>  	loff_t loopsize;
> +	int ret;
> +
> +	/*
> +	 * Get file attributes for validation. We use vfs_getattr() to ensure
> +	 * we have up-to-date file size information.
> +	 */

The comment seems a bit misleading or at least not to the point.

I'd say:

	/*
	 * File systems don't have to keep i_size in sync.  While local file
	 * systems typically keep it in sync, remote file system often do not.
	 * Go through ->getattr to retrieve the current value.
	 */

> +	ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 
> +			        AT_STATX_SYNC_AS_STAT);

Most kernel callers just pass 0 instead of AT_STATX_SYNC_AS_STAT here,
which honestly is less confusing as I had to look up
AT_STATX_SYNC_AS_STAT first.

> +	if (ret) {
> +		/*
> +		 * If we can't get attributes, fall back to i_size_read()
> +		 * which should work for most cases.
> +		 */

As said above, there is no need to do this.  Adding a proper error
return chain here is better.