diff mbox

[v3,4/5] xfs_db: sanitize geometry on load

Message ID 20170113003201.GC14038@birch.djwong.org
State Superseded, archived
Headers show

Commit Message

Darrick J. Wong Jan. 13, 2017, 12:32 a.m. UTC
xfs_db doesn't check the filesystem geometry when it's mounting, which
means that garbage agcount values can cause OOMs when we try to allocate
all the per-AG incore metadata.  If we see geometry that looks
suspicious, try to derive the actual AG geometry to avoid crashing the
system.  This should help with xfs/1301 fuzzing.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
variables to avoid screwing up the rest of the metadata.
v3: Suggest a possible agcount value, but always restrict to 1 AG.
---
 db/init.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 85 insertions(+), 10 deletions(-)

--
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

Brian Foster Jan. 13, 2017, 1:35 p.m. UTC | #1
On Thu, Jan 12, 2017 at 04:32:01PM -0800, Darrick J. Wong wrote:
> xfs_db doesn't check the filesystem geometry when it's mounting, which
> means that garbage agcount values can cause OOMs when we try to allocate
> all the per-AG incore metadata.  If we see geometry that looks
> suspicious, try to derive the actual AG geometry to avoid crashing the
> system.  This should help with xfs/1301 fuzzing.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: Only modify sb_ag{blocks,count} if they seem insane -- use local
> variables to avoid screwing up the rest of the metadata.
> v3: Suggest a possible agcount value, but always restrict to 1 AG.
> ---
>  db/init.c |   95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/db/init.c b/db/init.c
> index ec1e274..43a9409 100644
> --- a/db/init.c
> +++ b/db/init.c
> @@ -51,13 +51,94 @@ usage(void)
>  	exit(1);
>  }
>  
> +/* Try to load a superblock for the given agno, no verifiers. */
> +static bool
> +load_sb(
> +	struct xfs_mount	*mp,
> +	xfs_agnumber_t		agno,
> +	struct xfs_sb		*sbp)
> +{
> +	struct xfs_buf		*bp;
> +
> +	bp = libxfs_readbuf(mp->m_ddev_targp,
> +			    XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
> +			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> +
> +	if (!bp || bp->b_error)
> +		return false;
> +
> +	/* copy SB from buffer to in-core, converting architecture as we go */
> +	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
> +	libxfs_putbuf(bp);
> +	libxfs_purgebuf(bp);
> +
> +	return true;
> +}
> +
> +/*
> + * If the agcount doesn't look sane, suggest a real agcount to the user,
> + * and pretend agcount = 1 to avoid OOMing libxfs_initialize_perag.
> + */
> +static void
> +sanitize_geometry(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	struct xfs_sb		sb;
> +	unsigned int		blocklog;
> +	unsigned int		blocksize;
> +	unsigned int		agblocks;
> +	unsigned long long	dblocks;
> +
> +	/* If the geometry looks ok, we're done. */
> +	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
> +	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
> +	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
> +	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
> +	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
> +		return;
> +
> +	/* Check blocklog and blocksize */
> +	blocklog = sbp->sb_blocklog;
> +	blocksize = sbp->sb_blocksize;
> +	if (blocklog < XFS_MIN_BLOCKSIZE_LOG ||
> +	    blocklog > XFS_MAX_BLOCKSIZE_LOG)
> +		blocklog = libxfs_log2_roundup(blocksize);
> +	if (blocksize != (1 << blocklog))
> +		blocksize = (1 << blocksize);
> +

Same questions as before with regard to the above hunk.

> +	/* Clamp dblocks to the size of the device. */
> +	dblocks = sbp->sb_dblocks;
> +	if (dblocks > x.dsize * x.dbsize / blocksize)
> +		dblocks = x.dsize * x.dbsize / blocksize;
> +
> +	/*
> +	 * See if agblocks helps us find a superblock.
> +	 * blkbb_log is later (re)set by libxfs_mount.
> +	 */
> +	mp->m_blkbb_log = blocklog - BBSHIFT;
> +	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
> +	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {

If agblocks is bogus, this can effectively point anywhere in the fs,
correct? If so, I wonder how robust this magic number check is... for
example, what prevents us from pointing at a file data block with a
valid XFS superblock? IOW, perhaps we'd also need to validate sb_uuid.

> +		fprintf(stderr,
> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
> +	} else {
> +		fprintf(stderr,
> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> +			progname, fsdevice);
> +	}

For reasons like the above, I think xfs_db shouldn't be in the business
of repair like validation (xfs_check notwithstanding). That said,
dropping into a fixed single AG mode seems less risky than trying to
surmise a valid geometry. I'd get rid of the "this might be your
agcount" messaging entirely though and just replace it with something
that explicitly states the filesystem is corrupted, the runtime geometry
is invalid and that the user should probably run xfs_repair before doing
anything.

I still like the idea of the single AG mode thing as a command line flag
rather than default behavior because it requires user acknowledgement,
but this is a debug tool after all, so I'll defer to Eric on that. I do
think that if we create this kind of invalid runtime mode, this should
be split into two patches. First, a bugfix patch for the core OOM
problem (i.e., detect a wacky superblock and exit). Second, replace the
exit with the single AG runtime mode thing.

Brian

> +
> +	/* Assume 1 AG to avoid OOM. */
> +	sbp->sb_agcount = 1;
> +}
> +
>  void
>  init(
>  	int		argc,
>  	char		**argv)
>  {
>  	struct xfs_sb	*sbp;
> -	struct xfs_buf	*bp;
>  	int		c;
>  
>  	setlocale(LC_ALL, "");
> @@ -124,20 +205,12 @@ init(
>  	 */
>  	memset(&xmount, 0, sizeof(struct xfs_mount));
>  	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
> -	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
> -			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
> -
> -	if (!bp || bp->b_error) {
> +	if (!load_sb(&xmount, 0, &xmount.m_sb)) {
>  		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
>  			"bytes)\n"), progname, fsdevice);
>  		exit(1);
>  	}
>  
> -	/* copy SB from buffer to in-core, converting architecture as we go */
> -	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
> -	libxfs_putbuf(bp);
> -	libxfs_purgebuf(bp);
> -
>  	sbp = &xmount.m_sb;
>  	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
>  		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
> @@ -148,6 +221,8 @@ init(
>  		}
>  	}
>  
> +	sanitize_geometry(&xmount, sbp);
> +
>  	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
>  			  LIBXFS_MOUNT_DEBUGGER);
>  	if (!mp) {
> --
> 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 Jan. 14, 2017, 2:25 a.m. UTC | #2
On 1/13/17 7:35 AM, Brian Foster wrote:
>> +		fprintf(stderr,
>> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
>> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
>> +	} else {
>> +		fprintf(stderr,
>> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
>> +			progname, fsdevice);
>> +	}
> For reasons like the above, I think xfs_db shouldn't be in the business
> of repair like validation (xfs_check notwithstanding). That said,
> dropping into a fixed single AG mode seems less risky than trying to
> surmise a valid geometry. I'd get rid of the "this might be your
> agcount" messaging entirely though and just replace it with something
> that explicitly states the filesystem is corrupted, the runtime geometry
> is invalid and that the user should probably run xfs_repair before doing
> anything.

So keep in mind that xfs_db is for people with super xfs powers. (*)

I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state 
that clearly, and punt the problem to the admin with no other specific
suggestions.

> I still like the idea of the single AG mode thing as a command line flag
> rather than default behavior because it requires user acknowledgement,
> but this is a debug tool after all, so I'll defer to Eric on that. I do
> think that if we create this kind of invalid runtime mode, this should
> be split into two patches. First, a bugfix patch for the core OOM
> problem (i.e., detect a wacky superblock and exit). Second, replace the
> exit with the single AG runtime mode thing.

Well, the problem with a flag, I think, is that you might have already
unwittingly OOMed your box to find out that you need it.
Rebooting to try again with a flag sucks.

(*) unless you are invoking it via xfs_admin.sh, dammit.  We sure wouldn't
want xfs_admin to exit happily, having updated only one AG.  Dammit!

Perhaps it should set exitcode, and then xfs_admin could do something
like:

	xfs_db -c quit $DEV

first, and check that db is able to initialize sanely before using it again
to perform normal admin functions.

-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
Brian Foster Jan. 14, 2017, 3:44 a.m. UTC | #3
On Fri, Jan 13, 2017 at 08:25:47PM -0600, Eric Sandeen wrote:
> On 1/13/17 7:35 AM, Brian Foster wrote:
> >> +		fprintf(stderr,
> >> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
> >> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
> >> +	} else {
> >> +		fprintf(stderr,
> >> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> >> +			progname, fsdevice);
> >> +	}
> > For reasons like the above, I think xfs_db shouldn't be in the business
> > of repair like validation (xfs_check notwithstanding). That said,
> > dropping into a fixed single AG mode seems less risky than trying to
> > surmise a valid geometry. I'd get rid of the "this might be your
> > agcount" messaging entirely though and just replace it with something
> > that explicitly states the filesystem is corrupted, the runtime geometry
> > is invalid and that the user should probably run xfs_repair before doing
> > anything.
> 
> So keep in mind that xfs_db is for people with super xfs powers. (*)
> 
> I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state 
> that clearly, and punt the problem to the admin with no other specific
> suggestions.
> 
> > I still like the idea of the single AG mode thing as a command line flag
> > rather than default behavior because it requires user acknowledgement,
> > but this is a debug tool after all, so I'll defer to Eric on that. I do
> > think that if we create this kind of invalid runtime mode, this should
> > be split into two patches. First, a bugfix patch for the core OOM
> > problem (i.e., detect a wacky superblock and exit). Second, replace the
> > exit with the single AG runtime mode thing.
> 
> Well, the problem with a flag, I think, is that you might have already
> unwittingly OOMed your box to find out that you need it.
> Rebooting to try again with a flag sucks.
> 

I don't see how that is relevant. I'm not suggesting a
--please-don't-oom-in-case-of-corruption flag. :) As mentioned
previously, I think the bug fix here is a simple patch to detect the
bogus superblock and exit gracefully rather than go off the rails and
end up OOM killed.

From there the OOM is irrelevant and we can optionally enhance xfs_db to
try and allow it to run in such situations. To be honest, I'm perfectly
happy for xfs_db to exit gracefully in this situation and to leave it at
that. I think the majority of cases where this problem occurs, the next
logical step is to run xfs_repair. I suggested the flag approach more
because I think it's more appropriate to do things like fabricate fs
geometry behind a flag rather than by default. The larger point is that
if we want this kind of enhancement, it should probably be driven more
by a use case than an unfortunate (and probably rare) bug. I don't see
why we need to complicate the bug fix with the fancy enhancement.

Brian

> (*) unless you are invoking it via xfs_admin.sh, dammit.  We sure wouldn't
> want xfs_admin to exit happily, having updated only one AG.  Dammit!
> 
> Perhaps it should set exitcode, and then xfs_admin could do something
> like:
> 
> 	xfs_db -c quit $DEV
> 
> first, and check that db is able to initialize sanely before using it again
> to perform normal admin functions.
> 
> -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
--
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 Jan. 14, 2017, 3:51 a.m. UTC | #4
On 1/13/17 9:44 PM, Brian Foster wrote:
> On Fri, Jan 13, 2017 at 08:25:47PM -0600, Eric Sandeen wrote:
>> On 1/13/17 7:35 AM, Brian Foster wrote:
>>>> +		fprintf(stderr,
>>>> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
>>>> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
>>>> +	} else {
>>>> +		fprintf(stderr,
>>>> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
>>>> +			progname, fsdevice);
>>>> +	}
>>> For reasons like the above, I think xfs_db shouldn't be in the business
>>> of repair like validation (xfs_check notwithstanding). That said,
>>> dropping into a fixed single AG mode seems less risky than trying to
>>> surmise a valid geometry. I'd get rid of the "this might be your
>>> agcount" messaging entirely though and just replace it with something
>>> that explicitly states the filesystem is corrupted, the runtime geometry
>>> is invalid and that the user should probably run xfs_repair before doing
>>> anything.
>>
>> So keep in mind that xfs_db is for people with super xfs powers. (*)
>>
>> I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state 
>> that clearly, and punt the problem to the admin with no other specific
>> suggestions.
>>
>>> I still like the idea of the single AG mode thing as a command line flag
>>> rather than default behavior because it requires user acknowledgement,
>>> but this is a debug tool after all, so I'll defer to Eric on that. I do
>>> think that if we create this kind of invalid runtime mode, this should
>>> be split into two patches. First, a bugfix patch for the core OOM
>>> problem (i.e., detect a wacky superblock and exit). Second, replace the
>>> exit with the single AG runtime mode thing.
>>
>> Well, the problem with a flag, I think, is that you might have already
>> unwittingly OOMed your box to find out that you need it.
>> Rebooting to try again with a flag sucks.
>>
> 
> I don't see how that is relevant. I'm not suggesting a
> --please-don't-oom-in-case-of-corruption flag. :) As mentioned
> previously, I think the bug fix here is a simple patch to detect the
> bogus superblock and exit gracefully rather than go off the rails and
> end up OOM killed.

sorry, misunderstood the 
"idea of the single AG mode thing as a command line flag" idea, I guess.
 
> From there the OOM is irrelevant and we can optionally enhance xfs_db to
> try and allow it to run in such situations. To be honest, I'm perfectly
> happy for xfs_db to exit gracefully in this situation and to leave it at
> that. I think the majority of cases where this problem occurs, the next
> logical step is to run xfs_repair. I suggested the flag approach more
> because I think it's more appropriate to do things like fabricate fs
> geometry behind a flag rather than by default. The larger point is that
> if we want this kind of enhancement, it should probably be driven more
> by a use case than an unfortunate (and probably rare) bug. I don't see
> why we need to complicate the bug fix with the fancy enhancement.

*nod*  ok, I had understood the flag idea backwards-ly I guess.

I do think that mere mortal invocations via xfs_admin need to be
handled in this "ignore agcount" case, though...

-Eric
 
> Brian
> 
>> (*) unless you are invoking it via xfs_admin.sh, dammit.  We sure wouldn't
>> want xfs_admin to exit happily, having updated only one AG.  Dammit!
>>
>> Perhaps it should set exitcode, and then xfs_admin could do something
>> like:
>>
>> 	xfs_db -c quit $DEV
>>
>> first, and check that db is able to initialize sanely before using it again
>> to perform normal admin functions.
>>
>> -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

--
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 Jan. 14, 2017, 12:53 p.m. UTC | #5
On Fri, Jan 13, 2017 at 09:51:20PM -0600, Eric Sandeen wrote:
> On 1/13/17 9:44 PM, Brian Foster wrote:
> > On Fri, Jan 13, 2017 at 08:25:47PM -0600, Eric Sandeen wrote:
> >> On 1/13/17 7:35 AM, Brian Foster wrote:
> >>>> +		fprintf(stderr,
> >>>> +_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
> >>>> +			progname, fsdevice, dblocks / sbp->sb_agblocks);
> >>>> +	} else {
> >>>> +		fprintf(stderr,
> >>>> +_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
> >>>> +			progname, fsdevice);
> >>>> +	}
> >>> For reasons like the above, I think xfs_db shouldn't be in the business
> >>> of repair like validation (xfs_check notwithstanding). That said,
> >>> dropping into a fixed single AG mode seems less risky than trying to
> >>> surmise a valid geometry. I'd get rid of the "this might be your
> >>> agcount" messaging entirely though and just replace it with something
> >>> that explicitly states the filesystem is corrupted, the runtime geometry
> >>> is invalid and that the user should probably run xfs_repair before doing
> >>> anything.
> >>
> >> So keep in mind that xfs_db is for people with super xfs powers. (*)
> >>
> >> I wouldn't suggest repair, I'd start with 1 ag to avoid the OOM, state 
> >> that clearly, and punt the problem to the admin with no other specific
> >> suggestions.
> >>
> >>> I still like the idea of the single AG mode thing as a command line flag
> >>> rather than default behavior because it requires user acknowledgement,
> >>> but this is a debug tool after all, so I'll defer to Eric on that. I do
> >>> think that if we create this kind of invalid runtime mode, this should
> >>> be split into two patches. First, a bugfix patch for the core OOM
> >>> problem (i.e., detect a wacky superblock and exit). Second, replace the
> >>> exit with the single AG runtime mode thing.
> >>
> >> Well, the problem with a flag, I think, is that you might have already
> >> unwittingly OOMed your box to find out that you need it.
> >> Rebooting to try again with a flag sucks.
> >>
> > 
> > I don't see how that is relevant. I'm not suggesting a
> > --please-don't-oom-in-case-of-corruption flag. :) As mentioned
> > previously, I think the bug fix here is a simple patch to detect the
> > bogus superblock and exit gracefully rather than go off the rails and
> > end up OOM killed.
> 
> sorry, misunderstood the 
> "idea of the single AG mode thing as a command line flag" idea, I guess.
>  
> > From there the OOM is irrelevant and we can optionally enhance xfs_db to
> > try and allow it to run in such situations. To be honest, I'm perfectly
> > happy for xfs_db to exit gracefully in this situation and to leave it at
> > that. I think the majority of cases where this problem occurs, the next
> > logical step is to run xfs_repair. I suggested the flag approach more
> > because I think it's more appropriate to do things like fabricate fs
> > geometry behind a flag rather than by default. The larger point is that
> > if we want this kind of enhancement, it should probably be driven more
> > by a use case than an unfortunate (and probably rare) bug. I don't see
> > why we need to complicate the bug fix with the fancy enhancement.
> 
> *nod*  ok, I had understood the flag idea backwards-ly I guess.
> 
> I do think that mere mortal invocations via xfs_admin need to be
> handled in this "ignore agcount" case, though...
> 

We're talking about invocations intended to modify things (i.e., version
flags, fs uuid, etc.), right? If so, wouldn't we want those things to
fail if the superblock is busted?

Brian

> -Eric
>  
> > Brian
> > 
> >> (*) unless you are invoking it via xfs_admin.sh, dammit.  We sure wouldn't
> >> want xfs_admin to exit happily, having updated only one AG.  Dammit!
> >>
> >> Perhaps it should set exitcode, and then xfs_admin could do something
> >> like:
> >>
> >> 	xfs_db -c quit $DEV
> >>
> >> first, and check that db is able to initialize sanely before using it again
> >> to perform normal admin functions.
> >>
> >> -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
> 
> --
> 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 Jan. 14, 2017, 2:59 p.m. UTC | #6
On 1/14/17 6:53 AM, Brian Foster wrote:
>> I do think that mere mortal invocations via xfs_admin need to be
>> handled in this "ignore agcount" case, though...
>>
> We're talking about invocations intended to modify things (i.e., version
> flags, fs uuid, etc.), right? If so, wouldn't we want those things to
> fail if the superblock is busted?

Yes - I'm saying that if xfs_db continues with only 1 ag, those tools
will not work as expected.  (they normally modify all superblocks).

setting exitcode to 1 in the corrupted agcount case would allow
xfs_admin to return failure as well, for example.

-Eric

> Brian
> 
--
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 Jan. 15, 2017, 2:10 p.m. UTC | #7
On Sat, Jan 14, 2017 at 08:59:00AM -0600, Eric Sandeen wrote:
> On 1/14/17 6:53 AM, Brian Foster wrote:
> >> I do think that mere mortal invocations via xfs_admin need to be
> >> handled in this "ignore agcount" case, though...
> >>
> > We're talking about invocations intended to modify things (i.e., version
> > flags, fs uuid, etc.), right? If so, wouldn't we want those things to
> > fail if the superblock is busted?
> 
> Yes - I'm saying that if xfs_db continues with only 1 ag, those tools
> will not work as expected.  (they normally modify all superblocks).
> 

Right..

> setting exitcode to 1 in the corrupted agcount case would allow
> xfs_admin to return failure as well, for example.
> 

Ok, that makes sense to me. I wasn't sure if by "handled," you meant we
wanted those operations to try and do something useful in this
situation.

Brian

> -Eric
> 
> > Brian
> > 
--
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

Patch

diff --git a/db/init.c b/db/init.c
index ec1e274..43a9409 100644
--- a/db/init.c
+++ b/db/init.c
@@ -51,13 +51,94 @@  usage(void)
 	exit(1);
 }
 
+/* Try to load a superblock for the given agno, no verifiers. */
+static bool
+load_sb(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	struct xfs_sb		*sbp)
+{
+	struct xfs_buf		*bp;
+
+	bp = libxfs_readbuf(mp->m_ddev_targp,
+			    XFS_AG_DADDR(mp, agno, XFS_SB_DADDR),
+			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
+
+	if (!bp || bp->b_error)
+		return false;
+
+	/* copy SB from buffer to in-core, converting architecture as we go */
+	libxfs_sb_from_disk(sbp, XFS_BUF_TO_SBP(bp));
+	libxfs_putbuf(bp);
+	libxfs_purgebuf(bp);
+
+	return true;
+}
+
+/*
+ * If the agcount doesn't look sane, suggest a real agcount to the user,
+ * and pretend agcount = 1 to avoid OOMing libxfs_initialize_perag.
+ */
+static void
+sanitize_geometry(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	struct xfs_sb		sb;
+	unsigned int		blocklog;
+	unsigned int		blocksize;
+	unsigned int		agblocks;
+	unsigned long long	dblocks;
+
+	/* If the geometry looks ok, we're done. */
+	if (sbp->sb_blocklog >= XFS_MIN_BLOCKSIZE_LOG &&
+	    sbp->sb_blocklog <= XFS_MAX_BLOCKSIZE_LOG &&
+	    sbp->sb_blocksize == (1 << sbp->sb_blocklog) &&
+	    sbp->sb_dblocks * sbp->sb_blocksize <= x.dsize * x.dbsize &&
+	    sbp->sb_dblocks <= XFS_MAX_DBLOCKS(sbp) &&
+	    sbp->sb_dblocks >= XFS_MIN_DBLOCKS(sbp))
+		return;
+
+	/* Check blocklog and blocksize */
+	blocklog = sbp->sb_blocklog;
+	blocksize = sbp->sb_blocksize;
+	if (blocklog < XFS_MIN_BLOCKSIZE_LOG ||
+	    blocklog > XFS_MAX_BLOCKSIZE_LOG)
+		blocklog = libxfs_log2_roundup(blocksize);
+	if (blocksize != (1 << blocklog))
+		blocksize = (1 << blocksize);
+
+	/* Clamp dblocks to the size of the device. */
+	dblocks = sbp->sb_dblocks;
+	if (dblocks > x.dsize * x.dbsize / blocksize)
+		dblocks = x.dsize * x.dbsize / blocksize;
+
+	/*
+	 * See if agblocks helps us find a superblock.
+	 * blkbb_log is later (re)set by libxfs_mount.
+	 */
+	mp->m_blkbb_log = blocklog - BBSHIFT;
+	if (sbp->sb_agblocks > 0 && sbp->sb_agblocks <= MAXEXTNUM &&
+	    load_sb(mp, 1, &sb) && sb.sb_magicnum == XFS_SB_MAGIC) {
+		fprintf(stderr,
+_("%s: device %s AG count is insane, but could be %u.  Limiting reads to AG 0.\n"),
+			progname, fsdevice, dblocks / sbp->sb_agblocks);
+	} else {
+		fprintf(stderr,
+_("%s: device %s AG count is insane.  Limiting reads to AG 0.\n"),
+			progname, fsdevice);
+	}
+
+	/* Assume 1 AG to avoid OOM. */
+	sbp->sb_agcount = 1;
+}
+
 void
 init(
 	int		argc,
 	char		**argv)
 {
 	struct xfs_sb	*sbp;
-	struct xfs_buf	*bp;
 	int		c;
 
 	setlocale(LC_ALL, "");
@@ -124,20 +205,12 @@  init(
 	 */
 	memset(&xmount, 0, sizeof(struct xfs_mount));
 	libxfs_buftarg_init(&xmount, x.ddev, x.logdev, x.rtdev);
-	bp = libxfs_readbuf(xmount.m_ddev_targp, XFS_SB_DADDR,
-			    1 << (XFS_MAX_SECTORSIZE_LOG - BBSHIFT), 0, NULL);
-
-	if (!bp || bp->b_error) {
+	if (!load_sb(&xmount, 0, &xmount.m_sb)) {
 		fprintf(stderr, _("%s: %s is invalid (cannot read first 512 "
 			"bytes)\n"), progname, fsdevice);
 		exit(1);
 	}
 
-	/* copy SB from buffer to in-core, converting architecture as we go */
-	libxfs_sb_from_disk(&xmount.m_sb, XFS_BUF_TO_SBP(bp));
-	libxfs_putbuf(bp);
-	libxfs_purgebuf(bp);
-
 	sbp = &xmount.m_sb;
 	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
 		fprintf(stderr, _("%s: %s is not a valid XFS filesystem (unexpected SB magic number 0x%08x)\n"),
@@ -148,6 +221,8 @@  init(
 		}
 	}
 
+	sanitize_geometry(&xmount, sbp);
+
 	mp = libxfs_mount(&xmount, sbp, x.ddev, x.logdev, x.rtdev,
 			  LIBXFS_MOUNT_DEBUGGER);
 	if (!mp) {