diff mbox series

[2/2] xfs: verify icount in superblock write

Message ID 153262652554.8934.816888163232208002.stgit@magnolia (mailing list archive)
State Superseded, archived
Headers show
Series [1/2] libxfs: add more bounds checking to sb sanity checks | expand

Commit Message

Darrick J. Wong July 26, 2018, 5:35 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Add a helper predicate to check the inode count for sanity, then use it
in the superblock write verifier to inspect sb_icount.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_sb.c    |    1 +
 fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_types.h |    1 +
 3 files changed, 36 insertions(+)



--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bill O'Donnell July 26, 2018, 5:48 p.m. UTC | #1 
On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a helper predicate to check the inode count for sanity, then use it
> in the superblock write verifier to inspect sb_icount.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Looks good.
Thanks.
Reviewed-by: Bill O'Donnell <billodo@redhat.com>

>  fs/xfs/libxfs/xfs_sb.c    |    1 +
>  fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_types.h |    1 +
>  3 files changed, 36 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index b2c683588519..1659016875f9 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -714,6 +714,7 @@ xfs_sb_write_verify(
>  	 * cases.
>  	 */
>  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> +	    !xfs_verify_icount(mp, sb.sb_icount) ||
>  	    sb.sb_ifree > sb.sb_icount) {
>  		xfs_notice(mp, "SB summary counter sanity check failed");
>  		error = -EFSCORRUPTED;
> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> index 2e2a243cef2e..2e9c0c25ccb6 100644
> --- a/fs/xfs/libxfs/xfs_types.c
> +++ b/fs/xfs/libxfs/xfs_types.c
> @@ -171,3 +171,37 @@ xfs_verify_rtbno(
>  {
>  	return rtbno < mp->m_sb.sb_rblocks;
>  }
> +
> +/* Calculate the range of valid icount values. */
> +static void
> +xfs_icount_range(
> +	struct xfs_mount	*mp,
> +	unsigned long long	*min,
> +	unsigned long long	*max)
> +{
> +	unsigned long long	nr_inos = 0;
> +	xfs_agnumber_t		agno;
> +
> +	/* root, rtbitmap, rtsum all live in the first chunk */
> +	*min = XFS_INODES_PER_CHUNK;
> +
> +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> +		xfs_agino_t	first, last;
> +
> +		xfs_agino_range(mp, agno, &first, &last);
> +		nr_inos += first - last + 1;
> +	}
> +	*max = nr_inos;
> +}
> +
> +/* Sanity-checking of inode counts. */
> +bool
> +xfs_verify_icount(
> +	struct xfs_mount	*mp,
> +	unsigned long long	icount)
> +{
> +	unsigned long long	min, max;
> +
> +	xfs_icount_range(mp, &min, &max);
> +	return icount >= min && icount < max;
> +}
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index 4055d62f690c..b9e6c89284c3 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
> @@ -165,5 +165,6 @@ bool xfs_verify_ino(struct xfs_mount *mp, xfs_ino_t ino);
>  bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino);
>  bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino);
>  bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
> +bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount);
>  
>  #endif	/* __XFS_TYPES_H__ */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner July 26, 2018, 11:20 p.m. UTC | #2 
On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a helper predicate to check the inode count for sanity, then use it
> in the superblock write verifier to inspect sb_icount.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c    |    1 +
>  fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_types.h |    1 +
>  3 files changed, 36 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index b2c683588519..1659016875f9 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -714,6 +714,7 @@ xfs_sb_write_verify(
>  	 * cases.
>  	 */
>  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> +	    !xfs_verify_icount(mp, sb.sb_icount) ||
>  	    sb.sb_ifree > sb.sb_icount) {
>  		xfs_notice(mp, "SB summary counter sanity check failed");
>  		error = -EFSCORRUPTED;
> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> index 2e2a243cef2e..2e9c0c25ccb6 100644
> --- a/fs/xfs/libxfs/xfs_types.c
> +++ b/fs/xfs/libxfs/xfs_types.c
> @@ -171,3 +171,37 @@ xfs_verify_rtbno(
>  {
>  	return rtbno < mp->m_sb.sb_rblocks;
>  }
> +
> +/* Calculate the range of valid icount values. */
> +static void
> +xfs_icount_range(
> +	struct xfs_mount	*mp,
> +	unsigned long long	*min,
> +	unsigned long long	*max)
> +{
> +	unsigned long long	nr_inos = 0;
> +	xfs_agnumber_t		agno;
> +
> +	/* root, rtbitmap, rtsum all live in the first chunk */
> +	*min = XFS_INODES_PER_CHUNK;
> +
> +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> +		xfs_agino_t	first, last;
> +
> +		xfs_agino_range(mp, agno, &first, &last);
> +		nr_inos += first - last + 1;
> +	}
> +	*max = nr_inos;
> +}

And the effect of the inode32 mount option on the valid icount range?

Cheers,

Dave.
Darrick J. Wong July 27, 2018, 12:07 a.m. UTC | #3 
On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote:
> On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a helper predicate to check the inode count for sanity, then use it
> > in the superblock write verifier to inspect sb_icount.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_sb.c    |    1 +
> >  fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_types.h |    1 +
> >  3 files changed, 36 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index b2c683588519..1659016875f9 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -714,6 +714,7 @@ xfs_sb_write_verify(
> >  	 * cases.
> >  	 */
> >  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> > +	    !xfs_verify_icount(mp, sb.sb_icount) ||
> >  	    sb.sb_ifree > sb.sb_icount) {
> >  		xfs_notice(mp, "SB summary counter sanity check failed");
> >  		error = -EFSCORRUPTED;
> > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> > index 2e2a243cef2e..2e9c0c25ccb6 100644
> > --- a/fs/xfs/libxfs/xfs_types.c
> > +++ b/fs/xfs/libxfs/xfs_types.c
> > @@ -171,3 +171,37 @@ xfs_verify_rtbno(
> >  {
> >  	return rtbno < mp->m_sb.sb_rblocks;
> >  }
> > +
> > +/* Calculate the range of valid icount values. */
> > +static void
> > +xfs_icount_range(
> > +	struct xfs_mount	*mp,
> > +	unsigned long long	*min,
> > +	unsigned long long	*max)
> > +{
> > +	unsigned long long	nr_inos = 0;
> > +	xfs_agnumber_t		agno;
> > +
> > +	/* root, rtbitmap, rtsum all live in the first chunk */
> > +	*min = XFS_INODES_PER_CHUNK;
> > +
> > +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> > +		xfs_agino_t	first, last;
> > +
> > +		xfs_agino_range(mp, agno, &first, &last);
> > +		nr_inos += first - last + 1;
> > +	}
> > +	*max = nr_inos;
> > +}
> 
> And the effect of the inode32 mount option on the valid icount range?

Heh, I wondered about that.  The premise of inode32 is that we will
never allocate an inode with a number exceeding 2^32, correct?  Do we
ever write anything to that fs to say "this fs must never have inode
numbers > 2^32"?  i.e. something that permanently restricts it to
32-bit inode numbers and counts?  I don't think I see any such device.

What's supposed to happen if I create a > 1TB fs, put a bunch of files
on it such that some of them end up with inode numbers exceeding 2^32,
unmount it, and then mount it again with inode32?  Do we detect this and
refuse the mount because we can't honor the inode32 constraints?

Similarly, what if I create a filesystem with more than 4 billion files
on it, then unmount and remount with inode32?  Do we actually detect
this situation and refuse to mount because we know the counter is
already larger than 2^32?  If we allow the mount today, should we start
failing superblock writes because sb_icount is greater than 2^32?

In other words, I'm not sure inode32 can have any effect on the icount
*max if we don't refuse the mount if the fs already has 64-bit inodes.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Foster July 27, 2018, 10:44 a.m. UTC | #4 
On Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote:
> On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote:
> > On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Add a helper predicate to check the inode count for sanity, then use it
> > > in the superblock write verifier to inspect sb_icount.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/libxfs/xfs_sb.c    |    1 +
> > >  fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
> > >  fs/xfs/libxfs/xfs_types.h |    1 +
> > >  3 files changed, 36 insertions(+)
> > > 
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > index b2c683588519..1659016875f9 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -714,6 +714,7 @@ xfs_sb_write_verify(
> > >  	 * cases.
> > >  	 */
> > >  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> > > +	    !xfs_verify_icount(mp, sb.sb_icount) ||
> > >  	    sb.sb_ifree > sb.sb_icount) {
> > >  		xfs_notice(mp, "SB summary counter sanity check failed");
> > >  		error = -EFSCORRUPTED;
> > > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> > > index 2e2a243cef2e..2e9c0c25ccb6 100644
> > > --- a/fs/xfs/libxfs/xfs_types.c
> > > +++ b/fs/xfs/libxfs/xfs_types.c
> > > @@ -171,3 +171,37 @@ xfs_verify_rtbno(
> > >  {
> > >  	return rtbno < mp->m_sb.sb_rblocks;
> > >  }
> > > +
> > > +/* Calculate the range of valid icount values. */
> > > +static void
> > > +xfs_icount_range(
> > > +	struct xfs_mount	*mp,
> > > +	unsigned long long	*min,
> > > +	unsigned long long	*max)
> > > +{
> > > +	unsigned long long	nr_inos = 0;
> > > +	xfs_agnumber_t		agno;
> > > +
> > > +	/* root, rtbitmap, rtsum all live in the first chunk */
> > > +	*min = XFS_INODES_PER_CHUNK;
> > > +
> > > +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> > > +		xfs_agino_t	first, last;
> > > +
> > > +		xfs_agino_range(mp, agno, &first, &last);
> > > +		nr_inos += first - last + 1;

Shouldn't this be last - first?

> > > +	}
> > > +	*max = nr_inos;
> > > +}
> > 
> > And the effect of the inode32 mount option on the valid icount range?
> 
> Heh, I wondered about that.  The premise of inode32 is that we will
> never allocate an inode with a number exceeding 2^32, correct?  Do we
> ever write anything to that fs to say "this fs must never have inode
> numbers > 2^32"?  i.e. something that permanently restricts it to
> 32-bit inode numbers and counts?  I don't think I see any such device.
> 
> What's supposed to happen if I create a > 1TB fs, put a bunch of files
> on it such that some of them end up with inode numbers exceeding 2^32,
> unmount it, and then mount it again with inode32?  Do we detect this and
> refuse the mount because we can't honor the inode32 constraints?
> 
> Similarly, what if I create a filesystem with more than 4 billion files
> on it, then unmount and remount with inode32?  Do we actually detect
> this situation and refuse to mount because we know the counter is
> already larger than 2^32?  If we allow the mount today, should we start
> failing superblock writes because sb_icount is greater than 2^32?
> 

I thought an inode32 mount should allow reading existing inode64 inodes
without an issue. As noted above, it just prevents the allocation of
further inodes beyond 1TB.

> In other words, I'm not sure inode32 can have any effect on the icount
> *max if we don't refuse the mount if the fs already has 64-bit inodes.
> 

This patch looks like it doesn't consider inode32. It just ensures that
the icount falls into a valid range based on the ag geometry, which
seems broad enough to cover all cases... hm?

That aside.. since these values shouldn't change often I'm wondering if
it's worth calculating the global min/max once at mount time (we'd have
to recalc on growfs) rather than in the sb verifier path... It looks
like we already have a bunch of such misc min/max counters in xfs_mount.

Brian

> --D
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen July 27, 2018, 2:30 p.m. UTC | #5 
On 7/27/18 3:44 AM, Brian Foster wrote:
> On Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote:
>> On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote:
>>> On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote:
>>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>>
>>>> Add a helper predicate to check the inode count for sanity, then use it
>>>> in the superblock write verifier to inspect sb_icount.
>>>>
>>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>>> ---
>>>>  fs/xfs/libxfs/xfs_sb.c    |    1 +
>>>>  fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
>>>>  fs/xfs/libxfs/xfs_types.h |    1 +
>>>>  3 files changed, 36 insertions(+)
>>>>
>>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
>>>> index b2c683588519..1659016875f9 100644
>>>> --- a/fs/xfs/libxfs/xfs_sb.c
>>>> +++ b/fs/xfs/libxfs/xfs_sb.c
>>>> @@ -714,6 +714,7 @@ xfs_sb_write_verify(
>>>>  	 * cases.
>>>>  	 */
>>>>  	if (sb.sb_fdblocks > sb.sb_dblocks ||
>>>> +	    !xfs_verify_icount(mp, sb.sb_icount) ||
>>>>  	    sb.sb_ifree > sb.sb_icount) {
>>>>  		xfs_notice(mp, "SB summary counter sanity check failed");
>>>>  		error = -EFSCORRUPTED;
>>>> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
>>>> index 2e2a243cef2e..2e9c0c25ccb6 100644
>>>> --- a/fs/xfs/libxfs/xfs_types.c
>>>> +++ b/fs/xfs/libxfs/xfs_types.c
>>>> @@ -171,3 +171,37 @@ xfs_verify_rtbno(
>>>>  {
>>>>  	return rtbno < mp->m_sb.sb_rblocks;
>>>>  }
>>>> +
>>>> +/* Calculate the range of valid icount values. */
>>>> +static void
>>>> +xfs_icount_range(
>>>> +	struct xfs_mount	*mp,
>>>> +	unsigned long long	*min,
>>>> +	unsigned long long	*max)
>>>> +{
>>>> +	unsigned long long	nr_inos = 0;
>>>> +	xfs_agnumber_t		agno;
>>>> +
>>>> +	/* root, rtbitmap, rtsum all live in the first chunk */
>>>> +	*min = XFS_INODES_PER_CHUNK;
>>>> +
>>>> +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
>>>> +		xfs_agino_t	first, last;
>>>> +
>>>> +		xfs_agino_range(mp, agno, &first, &last);
>>>> +		nr_inos += first - last + 1;
> 
> Shouldn't this be last - first?
> 
>>>> +	}
>>>> +	*max = nr_inos;
>>>> +}
>>>
>>> And the effect of the inode32 mount option on the valid icount range?
>>
>> Heh, I wondered about that.  The premise of inode32 is that we will
>> never allocate an inode with a number exceeding 2^32, correct?  Do we
>> ever write anything to that fs to say "this fs must never have inode
>> numbers > 2^32"?  i.e. something that permanently restricts it to
>> 32-bit inode numbers and counts?  I don't think I see any such device.
>>
>> What's supposed to happen if I create a > 1TB fs, put a bunch of files
>> on it such that some of them end up with inode numbers exceeding 2^32,
>> unmount it, and then mount it again with inode32?  Do we detect this and
>> refuse the mount because we can't honor the inode32 constraints?
>>
>> Similarly, what if I create a filesystem with more than 4 billion files
>> on it, then unmount and remount with inode32?  Do we actually detect
>> this situation and refuse to mount because we know the counter is
>> already larger than 2^32?  If we allow the mount today, should we start
>> failing superblock writes because sb_icount is greater than 2^32?
>>
> 
> I thought an inode32 mount should allow reading existing inode64 inodes
> without an issue. As noted above, it just prevents the allocation of
> further inodes beyond 1TB.
> 
>> In other words, I'm not sure inode32 can have any effect on the icount
>> *max if we don't refuse the mount if the fs already has 64-bit inodes.
>>
> 
> This patch looks like it doesn't consider inode32. It just ensures that
> the icount falls into a valid range based on the ag geometry, which
> seems broad enough to cover all cases... hm?
> 
> That aside.. since these values shouldn't change often I'm wondering if
> it's worth calculating the global min/max once at mount time (we'd have
> to recalc on growfs) rather than in the sb verifier path... It looks
> like we already have a bunch of such misc min/max counters in xfs_mount.

It does seem like a fair bit of work to calculate unchanging values.  Since
this is really only an order-ofmagnitude sanity check anyway, I wonder if
this part of the verifier isn't working too hard to arrive at the
best-possible upper bound. My 64-bit divide was dumb & broken, but wouldn't
~(dblocks/inopb) get us close enough with a lot less work?

-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong July 27, 2018, 10:13 p.m. UTC | #6 
On Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote:
> On Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote:
> > On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote:
> > > On Thu, Jul 26, 2018 at 10:35:25AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Add a helper predicate to check the inode count for sanity, then use it
> > > > in the superblock write verifier to inspect sb_icount.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_sb.c    |    1 +
> > > >  fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
> > > >  fs/xfs/libxfs/xfs_types.h |    1 +
> > > >  3 files changed, 36 insertions(+)
> > > > 
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > > index b2c683588519..1659016875f9 100644
> > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > @@ -714,6 +714,7 @@ xfs_sb_write_verify(
> > > >  	 * cases.
> > > >  	 */
> > > >  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> > > > +	    !xfs_verify_icount(mp, sb.sb_icount) ||
> > > >  	    sb.sb_ifree > sb.sb_icount) {
> > > >  		xfs_notice(mp, "SB summary counter sanity check failed");
> > > >  		error = -EFSCORRUPTED;
> > > > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> > > > index 2e2a243cef2e..2e9c0c25ccb6 100644
> > > > --- a/fs/xfs/libxfs/xfs_types.c
> > > > +++ b/fs/xfs/libxfs/xfs_types.c
> > > > @@ -171,3 +171,37 @@ xfs_verify_rtbno(
> > > >  {
> > > >  	return rtbno < mp->m_sb.sb_rblocks;
> > > >  }
> > > > +
> > > > +/* Calculate the range of valid icount values. */
> > > > +static void
> > > > +xfs_icount_range(
> > > > +	struct xfs_mount	*mp,
> > > > +	unsigned long long	*min,
> > > > +	unsigned long long	*max)
> > > > +{
> > > > +	unsigned long long	nr_inos = 0;
> > > > +	xfs_agnumber_t		agno;
> > > > +
> > > > +	/* root, rtbitmap, rtsum all live in the first chunk */
> > > > +	*min = XFS_INODES_PER_CHUNK;
> > > > +
> > > > +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> > > > +		xfs_agino_t	first, last;
> > > > +
> > > > +		xfs_agino_range(mp, agno, &first, &last);
> > > > +		nr_inos += first - last + 1;
> 
> Shouldn't this be last - first?

Oops, yes, will fix that.

> > > > +	}
> > > > +	*max = nr_inos;
> > > > +}
> > > 
> > > And the effect of the inode32 mount option on the valid icount range?
> > 
> > Heh, I wondered about that.  The premise of inode32 is that we will
> > never allocate an inode with a number exceeding 2^32, correct?  Do we
> > ever write anything to that fs to say "this fs must never have inode
> > numbers > 2^32"?  i.e. something that permanently restricts it to
> > 32-bit inode numbers and counts?  I don't think I see any such device.
> > 
> > What's supposed to happen if I create a > 1TB fs, put a bunch of files
> > on it such that some of them end up with inode numbers exceeding 2^32,
> > unmount it, and then mount it again with inode32?  Do we detect this and
> > refuse the mount because we can't honor the inode32 constraints?
> > 
> > Similarly, what if I create a filesystem with more than 4 billion files
> > on it, then unmount and remount with inode32?  Do we actually detect
> > this situation and refuse to mount because we know the counter is
> > already larger than 2^32?  If we allow the mount today, should we start
> > failing superblock writes because sb_icount is greater than 2^32?
> > 
> 
> I thought an inode32 mount should allow reading existing inode64 inodes
> without an issue. As noted above, it just prevents the allocation of
> further inodes beyond 1TB.
> 
> > In other words, I'm not sure inode32 can have any effect on the icount
> > *max if we don't refuse the mount if the fs already has 64-bit inodes.
> > 
> 
> This patch looks like it doesn't consider inode32. It just ensures that
> the icount falls into a valid range based on the ag geometry, which
> seems broad enough to cover all cases... hm?

Correct.

> That aside.. since these values shouldn't change often I'm wondering if
> it's worth calculating the global min/max once at mount time (we'd have
> to recalc on growfs) rather than in the sb verifier path... It looks
> like we already have a bunch of such misc min/max counters in xfs_mount.

<shrug> I suppose so, but otoh I doubt this function has a lot of
overhead.  I'll look into it for the next version.

--D

> Brian
> 
> > --D
> > 
> > > Cheers,
> > > 
> > > Dave.
> > > -- 
> > > Dave Chinner
> > > david@fromorbit.com
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner July 29, 2018, 4:39 a.m. UTC | #7 
On Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote:
> That aside.. since these values shouldn't change often I'm wondering if
> it's worth calculating the global min/max once at mount time (we'd have

The values are needed during the initial read of the superblock
before any "for the life of the mount" calculations can be made,
and they can change across log recovery, too, if recovery replays a
growfs transaction. Hence they really need to be point in time
calculations based on the current superblock values during
superblock reads....

Cheers,

Dave.
Brian Foster July 30, 2018, 12:36 p.m. UTC | #8 
On Sun, Jul 29, 2018 at 02:39:00PM +1000, Dave Chinner wrote:
> On Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote:
> > That aside.. since these values shouldn't change often I'm wondering if
> > it's worth calculating the global min/max once at mount time (we'd have
> 
> The values are needed during the initial read of the superblock
> before any "for the life of the mount" calculations can be made,
> and they can change across log recovery, too, if recovery replays a
> growfs transaction. Hence they really need to be point in time
> calculations based on the current superblock values during
> superblock reads....
> 

I'm not so worried about storing the calculated total as much as
simplifying the implementation. All but the last iteration in the
current loop calculate a fixed value, so this can trivially be
simplified to something like:

	xfs_agino_range(mp, --agcount, first, last);
	max = last - first + 1;
	if (agcount) {
		xfs_agino_range(mp, agcount - 1, first, last);
		max += (last - first + 1) * agcount;
	}

... which (if it mattered) could probably simplify even further if we
factored out the additional portion of the calculation that is fixed:

	xfs_agino_range(mp, --agcount, first, last);
	max = last - first + 1;
	if (agcount)
		max += agcount * mp->m_aginodes;

I.e., with an ->m_aginodes field in xfs_mount that is analogous to
sb_agblocks.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index b2c683588519..1659016875f9 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -714,6 +714,7 @@  xfs_sb_write_verify(
 	 * cases.
 	 */
 	if (sb.sb_fdblocks > sb.sb_dblocks ||
+	    !xfs_verify_icount(mp, sb.sb_icount) ||
 	    sb.sb_ifree > sb.sb_icount) {
 		xfs_notice(mp, "SB summary counter sanity check failed");
 		error = -EFSCORRUPTED;
diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
index 2e2a243cef2e..2e9c0c25ccb6 100644
--- a/fs/xfs/libxfs/xfs_types.c
+++ b/fs/xfs/libxfs/xfs_types.c
@@ -171,3 +171,37 @@  xfs_verify_rtbno(
 {
 	return rtbno < mp->m_sb.sb_rblocks;
 }
+
+/* Calculate the range of valid icount values. */
+static void
+xfs_icount_range(
+	struct xfs_mount	*mp,
+	unsigned long long	*min,
+	unsigned long long	*max)
+{
+	unsigned long long	nr_inos = 0;
+	xfs_agnumber_t		agno;
+
+	/* root, rtbitmap, rtsum all live in the first chunk */
+	*min = XFS_INODES_PER_CHUNK;
+
+	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
+		xfs_agino_t	first, last;
+
+		xfs_agino_range(mp, agno, &first, &last);
+		nr_inos += first - last + 1;
+	}
+	*max = nr_inos;
+}
+
+/* Sanity-checking of inode counts. */
+bool
+xfs_verify_icount(
+	struct xfs_mount	*mp,
+	unsigned long long	icount)
+{
+	unsigned long long	min, max;
+
+	xfs_icount_range(mp, &min, &max);
+	return icount >= min && icount < max;
+}
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index 4055d62f690c..b9e6c89284c3 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -165,5 +165,6 @@  bool xfs_verify_ino(struct xfs_mount *mp, xfs_ino_t ino);
 bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino);
 bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino);
 bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
+bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount);
 
 #endif	/* __XFS_TYPES_H__ */