Re: [PATCH v4 1/4] mm/truncate: align truncation boundaries to mapping minimum folio order

[Jan Kara]

On Thu 24-09-26 17:31:40, Zhang Yi wrote:
> > @@ -395,6 +397,10 @@ void truncate_inode_pages_range(struct address_space *mapping,
> >  	else
> >  		end = (lend + 1) >> PAGE_SHIFT;
> >  
> > +	start = round_up(start, min_nrpages);
> > +	if (end != (pgoff_t)-1)
> > +		end = round_down(end, min_nrpages);
> > +
> Hi, all
> 
> The sashiko pointed out an issue on this patch:
> 
> > [Severity: Critical]
> > Can this integer overflow on 32-bit architectures?
> >
> > In truncate_inode_pages_range(), pgoff_t is a 32-bit integer on 32-bit
> > systems. If a hole punch or truncate operation occurs at an offset near
> > MAX_LFS_FILESIZE, the start page index will be close to 0xFFFFFFFF.
> >
> > When min_nrpages > 1 (e.g., on filesystems supporting min_order > 0 like
> > Ext4 with bigalloc), rounding up this index wraps it to 0.
> >
> > Would this wrap-around cause the entire page cache for the file to be
> > truncated instead of just the intended tail, leading to data loss?
> 
> This is a pre-existing issue, and it is not specific to the truncate
> path.
> 
> On 32-bit systems, for a mapping with a non-zero minimum folio order,
> the last min_order-aligned block of a file of MAX_LFS_FILESIZE is also
> the last block of the page index space, so the folio covering it ends at
> or past the last representable page index.  All places that do offset
> calculations on such end folios carry an overflow risk, e.g.,
> folio_next_index() wraps to 0 for that folio, and the values derived
> from it are wrong.
> 
> For example, on a 32-bit environment I create an ext4 filesystem with a
> 16KB blocksize, where the maximum file size is 0xFFFFFFFF000
> (MAX_LFS_FILESIZE). If we run:
> 
>  xfs_io -f -c "pwrite -b 0x10000 0xFFFFFFFC000 0x3000" /mnt/foo
> 
> The last folio starts at 0xFFFFFFFC000, but its length is 0x4000,
> so calling folio_next_index() on it will overflow, leading to
> unpredictable errors.
> 
> 
> I think the root cause is that sb->s_maxbytes is set to
> MAX_LFS_FILESIZE, which on 32-bit is (loff_t)ULONG_MAX << PAGE_SHIFT.  A
> folio is at least min_order pages and min_order aligned, so the file
> must have at most ULONG_MAX + 1 - (1 << min_order) pages for the last
> folio's next index to stay representable.
> 
> I suppose filesystems that use a non-zero minimum folio order should cap
> sb->s_maxbytes instead of using MAX_LFS_FILESIZE directly, something
> like this:
> 
> static inline loff_t max_lfs_filesize(unsigned int min_order)
> {
> #if BITS_PER_LONG == 32
> 	return ((loff_t)ULONG_MAX + 1 - (1UL << min_order)) << PAGE_SHIFT;
> #else
>     return MAX_LFS_FILESIZE;
> #endif
> }
> 
> Any thoughts?

Yeah, I guess it makes sense.

> Besides, I understand that since this is a pre-existing issue, it
> shouldn't block the merging of this series?

Right, I don't think this belongs to this patchset.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR