| Message ID | 20211005223155.GD24307@magnolia (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | libxfs: fix call_rcu crash when unmounting the fake mount in mkfs | expand |
On 10/5/21 5:31 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > In commit a6fb6abe, we simplified the process by which mkfs.xfs computes > the minimum log size calculation by creating a dummy xfs_mount with the > draft superblock image, using the dummy to compute the log geometry, and > then unmounting the dummy. > > Note that creating a dummy mount with no data device is supported by > libxfs, though with the caveat that we don't set up any perag structures > at all. Up until this point this has worked perfectly well since free() > (and hence kmem_free()) are perfectly happy to ignore NULL pointers. > > Unfortunately, this will cause problems with the upcoming patch to shift > per-AG setup and teardown to libxfs because call_rcu in the liburcu > library actually tries to access the rcu_head of the passed-in perag > structure, but they're all NULL in the dummy mount case. IOWs, > xfs_free_perag requires that every AG have a per-AG structure, and it's > too late to change the 5.14 kernel libxfs now, so work around this by > altering libxfs_mount to remember when it has initialized the perag > structures and libxfs_umount to skip freeing them when the flag isn't > set. > > Just to be clear: This fault has no user-visible consequences right now; > it's a fixup to avoid problems in the libxfs sync series for 5.14. Thanks Darrick - I think this seems fine for now. We could re-evaluate whether we want to change xfs_free_perag() to handle NULLs later, but since this is a special case for userspace anyway, I think it might be most clear to just keep this workaround in the caller as you've done here. Reviewed-by: Eric Sandeen <sandeen@redhat.com> > > Fixes: a6fb6abe ("mkfs: simplify minimum log size calculation") > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > include/xfs_mount.h | 1 + > libxfs/init.c | 13 ++++++++++--- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/include/xfs_mount.h b/include/xfs_mount.h > index 2f320880..9e43cd23 100644 > --- a/include/xfs_mount.h > +++ b/include/xfs_mount.h > @@ -190,6 +190,7 @@ xfs_perag_resv( > #define LIBXFS_MOUNT_COMPAT_ATTR 0x0008 > #define LIBXFS_MOUNT_ATTR2 0x0010 > #define LIBXFS_MOUNT_WANT_CORRUPTED 0x0020 > +#define LIBXFS_MOUNT_PERAG_DATA_LOADED 0x0040 > > #define LIBXFS_BHASHSIZE(sbp) (1<<10) > > diff --git a/libxfs/init.c b/libxfs/init.c > index 17fc1102..d0753ce5 100644 > --- a/libxfs/init.c > +++ b/libxfs/init.c > @@ -912,6 +912,7 @@ libxfs_mount( > progname); > exit(1); > } > + mp->m_flags |= LIBXFS_MOUNT_PERAG_DATA_LOADED; > > return mp; > } > @@ -1031,9 +1032,15 @@ libxfs_umount( > libxfs_bcache_purge(); > error = libxfs_flush_mount(mp); > > - for (agno = 0; agno < mp->m_maxagi; agno++) { > - pag = radix_tree_delete(&mp->m_perag_tree, agno); > - kmem_free(pag); > + /* > + * Only try to free the per-AG structures if we set them up in the > + * first place. > + */ > + if (mp->m_flags & LIBXFS_MOUNT_PERAG_DATA_LOADED) { > + for (agno = 0; agno < mp->m_maxagi; agno++) { > + pag = radix_tree_delete(&mp->m_perag_tree, agno); > + kmem_free(pag); > + } > } > > kmem_free(mp->m_attr_geo); >
diff --git a/include/xfs_mount.h b/include/xfs_mount.h index 2f320880..9e43cd23 100644 --- a/include/xfs_mount.h +++ b/include/xfs_mount.h @@ -190,6 +190,7 @@ xfs_perag_resv( #define LIBXFS_MOUNT_COMPAT_ATTR 0x0008 #define LIBXFS_MOUNT_ATTR2 0x0010 #define LIBXFS_MOUNT_WANT_CORRUPTED 0x0020 +#define LIBXFS_MOUNT_PERAG_DATA_LOADED 0x0040 #define LIBXFS_BHASHSIZE(sbp) (1<<10) diff --git a/libxfs/init.c b/libxfs/init.c index 17fc1102..d0753ce5 100644 --- a/libxfs/init.c +++ b/libxfs/init.c @@ -912,6 +912,7 @@ libxfs_mount( progname); exit(1); } + mp->m_flags |= LIBXFS_MOUNT_PERAG_DATA_LOADED; return mp; } @@ -1031,9 +1032,15 @@ libxfs_umount( libxfs_bcache_purge(); error = libxfs_flush_mount(mp); - for (agno = 0; agno < mp->m_maxagi; agno++) { - pag = radix_tree_delete(&mp->m_perag_tree, agno); - kmem_free(pag); + /* + * Only try to free the per-AG structures if we set them up in the + * first place. + */ + if (mp->m_flags & LIBXFS_MOUNT_PERAG_DATA_LOADED) { + for (agno = 0; agno < mp->m_maxagi; agno++) { + pag = radix_tree_delete(&mp->m_perag_tree, agno); + kmem_free(pag); + } } kmem_free(mp->m_attr_geo);