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