Re: [PATCH 0/8 V2] xfs: log fixes for for-next

[Dave Chinner <david@fromorbit.com>]

On Thu, Jun 17, 2021 at 06:26:09PM +1000, Dave Chinner wrote:
> Hi folks,
> 
> This is followup from the first set of log fixes for for-next that
> were posted here:
> 
> https://lore.kernel.org/linux-xfs/20210615175719.GD158209@locust/T/#mde2cf0bb7d2ac369815a7e9371f0303efc89f51b
> 
> The first two patches of this series are updates for those patches,
> change log below. The rest is the fix for the bigger issue we
> uncovered in investigating the generic/019 failures, being that
> we're triggering a zero-day bug in the way log recovery assigns LSNs
> to checkpoints.
> 
> The "simple" fix of using the same ordering code as the commit
> record for the start records in the CIL push turned into a lot of
> patches once I started cleaning it up, separating out all the
> different bits and finally realising all the things I needed to
> change to avoid unintentional logic/behavioural changes. Hence
> there's some code movement, some factoring, API changes to
> xlog_write(), changing where we attach callbacks to commit iclogs so
> they remain correctly ordered if there are multiple commit records
> in the one iclog and then, finally, strictly ordering the start
> records....
> 
> The original "simple fix" I tested last night ran almost a thousand
> cycles of generic/019 without a log hang or recovery failure of any
> kind. The refactored patchset has run a couple hundred cycles of
> g/019 and g/475 over the last few hours without a failure, so I'm
> posting this so we can get a review iteration done while I sleep so
> we can - hopefully - get this sorted out before the end of the week.

Update on this so people know what's happening.

Yesterday I found another zero-day bug in the CIL code that triggers
when a shutdown occurs.

The shutdown processing runs asynchronously and without caring about
the current state or users of the iclogs. SO when it runs
xlog_state_do_callbacks() after changing the state of all iclogs to
XLOG_STATE_IOERROR, it runs the callbacks on all the iclogs and
frees everything associated with them.

That includes the CIL context structure that xlog_cil_push_now() is
still working on because it has a referenced iclog that it hasn't
yet released.

Hence the initial CIL commit that stamps the CIL context with the
commit lsn -after- it has attached the context to the commit_iclog
callback list can race with shutdown. This results in a UAF
situation and an 8 byte memory corruption when we stamp the LSN into
the context.

The current for-next tree does *much more* with the context after
the callbacks are attached, which opens up this UAF to both reads
and writes of free memory. The fix in patch 2, which adds a sleep on
the previous iclog after attaching the callbacks to the commit iclog
opens this window even futher.

ANd then the start record ordering patch set moves the commit iclog
into CIL context structure which we dereference after waiting on the
previous iclog means we are dereferencing pointers freed memory.

So, basically, before any of these fixes can go forwards, I first
need to fix the pre-existing CIL push/shutdown race.

And then, after I've rebased all these fixes on that fix and we're
back to square one and before we do anything else in the log, we
need to fix the mess that is caused by unco-ordinated shutdown
changing iclog state and running completions while we still have
active references to the iclogs and are preparing the iclog for IO.
XLOG_STATE_IOERROR must be considered harmful at this point in time.

-Dave.
-- 
Dave Chinner
david@fromorbit.com