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@xxxxxxxxxx>
> >> ---
> >>
> >> 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@xxxxxxxxxx>
> >
> > --D
> >
> >> + }
> >> error = xfs_bmbt_change_owner(NULL, ip, XFS_ATTR_FORK,
> >> ip->i_ino, buffer_list);
> >> if (error)
> >>
> >
