Re: [PATCH 1/2] xfs: handle bad flags in xfs_recover_inode_owner_change

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Dec 18, 2018 at 01:23:22PM -0600, Eric Sandeen wrote:
> On 12/18/18 1:15 PM, Darrick J. Wong wrote:
> > On Tue, Dec 18, 2018 at 01:02:56PM -0600, Eric Sandeen wrote:
> >> Today, xfs_recover_inode_owner_change() indicates that if XFS_ILOG_DOWNER
> >> is set, XFS_ILOG_DBROOT must be as well, via an assert.  However, this 
> >> could fail to be true due to fuzzing or corruption, so we really
> >> should handle it gracefully rather than calling ASSERT() and crashing,
> >> or blowing past it on a non-debug build and BUGging later.
> >>
> >> Return -EFSCORRUPTED and fail the log replay if we find this
> >> inconsistent state.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> >> index 1fc9e9042e0e..56148a3083b8 100644
> >> --- a/fs/xfs/xfs_log_recover.c
> >> +++ b/fs/xfs/xfs_log_recover.c
> >> @@ -2964,7 +2964,10 @@ xfs_recover_inode_owner_change(
> >>  	}
> >>  
> >>  	if (in_f->ilf_fields & XFS_ILOG_DOWNER) {
> >> -		ASSERT(in_f->ilf_fields & XFS_ILOG_DBROOT);
> >> +		if (!(in_f->ilf_fields & XFS_ILOG_DBROOT)) {
> >> +			error = -EFSCORRUPTED;
> >> +			goto out_free_ip;
> >> +		}
> >>  		error = xfs_bmbt_change_owner(NULL, ip, XFS_DATA_FORK,
> >>  					      ip->i_ino, buffer_list);
> >>  		if (error)
> >> @@ -2972,7 +2975,10 @@ xfs_recover_inode_owner_change(
> >>  	}
> >>  
> >>  	if (in_f->ilf_fields & XFS_ILOG_AOWNER) {
> >> -		ASSERT(in_f->ilf_fields & XFS_ILOG_ABROOT);
> >> +		if (!(in_f->ilf_fields & XFS_ILOG_ABROOT)) {
> >> +			error = -EFSCORRUPTED;
> >> +			goto out_free_ip;
> > 
> > Are there any downsides to changing the data fork owner and bailing out
> > afterwards if we encounter the combination of (DOWNER | DBROOT | AOWNER)?
> 
> Not sure I understand the Q.
> 
> (Maybe you mean DOWNER && !DBROOT?)

No, I really did mean the case where DOWNER and DBROOT are set properly,
but it's the AOWNER/ABROOT flags that aren't set properly.  I was
wondering why not check DOWNER/DBROOT and AOWNER/ABROOT before touching
*anything* and was typing my way through it.

> > Thinking this through, the log won't continue recovering, so you
> > have to
> > run xfs_repair -L which zaps the log and checks everything.  We already
> > finished the data fork bmbt update so (barring other problems) it should
> > be fine.  The attr fork won't have been updated, but its log entries
> > were unrecoverable, so at worst we lose the attr fork, right?
> 
> TBH, I hadn't really thought through "recover as much as we can before
> deciding we have a problem" - if we encounter this, it's an inconsistent
> state in the log for whatever, and we should stop.  I don't ... think
> we're in the business of trying to second guess or fix on the fly here,
> right?

If that's true then we ought to validate all four flags before calling
xfs_bmbt_change_owner(), right?

> > And we don't want the ABROOT check any earlier, because we don't want to
> > forego a data fork owner update that might have succeeded anyway and
> > we'll definitely lose it if we don't update it and xfs_repair encounters
> > it.  Right?
> 
> Again, my caveman coder brain just said "inconsistent state -> stop now."
> 
> Should we be doing more?

See my reply to the second patch, sorry. :/

--D

> -Eric
> 
> > If so, then,
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > --D
> > 
> >> +		}
> >>  		error = xfs_bmbt_change_owner(NULL, ip, XFS_ATTR_FORK,
> >>  					      ip->i_ino, buffer_list);
> >>  		if (error)
> >>
> >