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