xfs
[Top] [All Lists]

Re: [PATCH] xfs: shutdown filesystem if xfs_perag_get fails

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH] xfs: shutdown filesystem if xfs_perag_get fails
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Sun, 21 Apr 2013 16:55:06 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130421174107.007313126@xxxxxxx>
References: <20130419204102.736961610@xxxxxxx> <20130421174107.007313126@xxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/20130328 Thunderbird/17.0.5
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
> 

<Prev in Thread] Current Thread [Next in Thread>