linux-kernel - Re: [PATCH v3 10/11] xfs: make the calculation generic in xfs_sb_validate_fsb

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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 26 Mar 2024 10:53:27 +0100
From: "Pankaj Raghav (Samsung)" <kernel@...kajraghav.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org, 
	gost.dev@...sung.com, chandan.babu@...cle.com, hare@...e.de, mcgrof@...nel.org, 
	djwong@...nel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	david@...morbit.com, akpm@...ux-foundation.org, Pankaj Raghav <p.raghav@...sung.com>
Subject: Re: [PATCH v3 10/11] xfs: make the calculation generic in
 xfs_sb_validate_fsb_count()

On Mon, Mar 25, 2024 at 07:15:59PM +0000, Matthew Wilcox wrote:
> On Wed, Mar 13, 2024 at 06:02:52PM +0100, Pankaj Raghav (Samsung) wrote:
> > From: Pankaj Raghav <p.raghav@...sung.com>
> > 
> > Instead of assuming that PAGE_SHIFT is always higher than the blocklog,
> > make the calculation generic so that page cache count can be calculated
> > correctly for LBS.
> > 
> > Signed-off-by: Pankaj Raghav <p.raghav@...sung.com>
> > ---
> >  fs/xfs/xfs_mount.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index aabb25dc3efa..9cf800586da7 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -133,9 +133,16 @@ xfs_sb_validate_fsb_count(
> >  {
> >  	ASSERT(PAGE_SHIFT >= sbp->sb_blocklog);
> 
> but ... you're still asserting that PAGE_SHIFT is larger than blocklog.
> Shouldn't you delete that assertion?

I do that in the next patch when we enable LBS support in XFS by setting
min order. So we still need the check here that will be removed in the
next patch.
> 
> >  	ASSERT(sbp->sb_blocklog >= BBSHIFT);
> > +	uint64_t max_index;
> > +	uint64_t max_bytes;
> > +
> > +	if (check_shl_overflow(nblocks, sbp->sb_blocklog, &max_bytes))
> > +		return -EFBIG;
> >  
> >  	/* Limited by ULONG_MAX of page cache index */
> > -	if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX)
> > +	max_index = max_bytes >> PAGE_SHIFT;
> > +
> > +	if (max_index > ULONG_MAX)
> >  		return -EFBIG;
> 
> This kind of depends on the implementation details of the page cache.
> We have MAX_LFS_FILESIZE to abstract that; maybe that should be used
> here?

Makes sense. I will use it instead of using ULONG_MAX.