On 4/21/13 12:41 PM, Mark Tinguely wrote:
> This problem happened locally with a bad inode number from xfs
> recovery. xfs_perag_get() can return NULL if given a bad agno.
> Most callers of xfs_perag_get() do not check for a NULL before
> using the pointer. This patch forces a shutdown of the filesystem
> for those callers that do not check the return value rather than
> crashing on a dereferenced NULL pointer.
Hi Mark -
I'm curious, what was the callchain when this happened? Was it
during recovery? If so, would aborting recovery be more prudent?
I might be missing something, but I'm not sure how shutting
down avoids a subsequent null ptr deref & crash.
i.e. if a caller does something like:
pag = xfs_perag_get(mp, agno);
spin_lock(&pag->pagb_lock);
shutting down in xfs_perag_get doesn't save us from a
null pag pointer, would it?
Thanks,
-Eric
> Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> ---
> fs/xfs/xfs_icache.c | 2 +-
> fs/xfs/xfs_mount.c | 4 ++--
> fs/xfs/xfs_mount.h | 17 ++++++++++++++++-
> 3 files changed, 19 insertions(+), 4 deletions(-)
>
> Index: b/fs/xfs/xfs_icache.c
> ===================================================================
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -656,7 +656,7 @@ xfs_inode_ag_iterator(
> xfs_agnumber_t ag;
>
> ag = 0;
> - while ((pag = xfs_perag_get(mp, ag))) {
> + while ((pag = __xfs_perag_get(mp, ag))) {
> ag = pag->pag_agno + 1;
> error = xfs_inode_ag_walk(mp, pag, execute, flags, args, -1);
> xfs_perag_put(pag);
> Index: b/fs/xfs/xfs_mount.c
> ===================================================================
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -193,7 +193,7 @@ xfs_uuid_unmount(
> * have to protect against changes is the tree structure itself.
> */
> struct xfs_perag *
> -xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
> +__xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
> {
> struct xfs_perag *pag;
> int ref = 0;
> @@ -442,7 +442,7 @@ xfs_initialize_perag(
> * AGs we don't find ready for initialisation.
> */
> for (index = 0; index < agcount; index++) {
> - pag = xfs_perag_get(mp, index);
> + pag = __xfs_perag_get(mp, index);
> if (pag) {
> xfs_perag_put(pag);
> continue;
> Index: b/fs/xfs/xfs_mount.h
> ===================================================================
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -336,12 +336,27 @@ xfs_daddr_to_agbno(struct xfs_mount *mp,
> /*
> * perag get/put wrappers for ref counting
> */
> -struct xfs_perag *xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno);
> +struct xfs_perag *__xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno);
> struct xfs_perag *xfs_perag_get_tag(struct xfs_mount *mp, xfs_agnumber_t agno,
> int tag);
> void xfs_perag_put(struct xfs_perag *pag);
>
> /*
> + * Ensure the per AG entry was found. Shutting down the filesystem
> + * is better than crashing the OS.
> + */
> +static inline struct xfs_perag *
> +xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
> +{
> + struct xfs_perag *pag;
> +
> + pag = __xfs_perag_get(mp, agno);
> + if (!pag)
> + xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE);
> + return pag;
> +}
> +
> +/*
> * Per-cpu superblock locking functions
> */
> #ifdef HAVE_PERCPU_SB
>
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
