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

[Eric Sandeen <sandeen@sandeen.net>]

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@sgi.com>
> ---
>  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@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs