Re: [PATCH 2/9] libxfs: don't UAF a requeued EFI

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Nov 22, 2023 at 10:36:57PM -0800, Christoph Hellwig wrote:
> On Wed, Nov 22, 2023 at 03:06:59PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > In the kernel, commit 8ebbf262d4684 ("xfs: don't block in busy flushing
> > when freeing extents") changed the allocator behavior such that AGFL
> > fixing can return -EAGAIN in response to detection of a deadlock with
> > the transaction busy extent list.  If this happens, we're supposed to
> > requeue the EFI so that we can roll the transaction and try the item
> > again.
> > 
> > If a requeue happens, we should not free the xefi pointer in
> > xfs_extent_free_finish_item or else the retry will walk off a dangling
> > pointer.  There is no extent busy list in userspace so this should
> > never happen, but let's fix the logic bomb anyway.
> > 
> > We should have ported kernel commit 0853b5de42b47 ("xfs: allow extent
> > free intents to be retried") to userspace, but neither Carlos nor I
> > noticed this fine detail. :(
> 
> It might be time to move this code into shared files?

I think Chandan started looking into what it would take to port the log
code from the kernel into userspace.  Then xfs_trans_commit in userspace
could actually write transactions to the log and write them back
atomically; and xfs_repair could finally lose the -L switch.

> Btw, I still keep getting annoyed a bit about minor difference
> in some of the libxfs file due to header inclusion.  Maybe we also
> need to be able to automate this more and require libxfs to only
> include libxfs headers and xfs.h?

<shrug> Ages ago Dave had a sketch to make xfs_{log,mount,inode}.h pull
in the relevant headers so that all the other source files only had to
#include at most three files.

I wish the kernel had precompiled headers, then it would make sense to
have one xfs.h file that pulled in the world and took advantage of
caching.

--D

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>