Re: [PATCH 1/2] xfs: reset xfs_inode struct on reclaim in debug mode

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Apr 05, 2018 at 08:11:46AM -0400, Brian Foster wrote:
> A test case to reproduce a filestream/MRU use-after-free of a
> reclaimed inode requires bits (e.g., ip->i_mount) of the inode to be
> reset/reused once the inode memory is freed. This normally only
> occurs when a new page is cycled into the zone, however.
> 
> Perform the "one-time" inode init immediately prior to freeing
> inodes when in DEBUG mode. This will zero the inode, init the low
> level structures (locks, lists, etc.) and otherwise ensure each
> inode is in a purely uninitialized state while sitting in the zone
> as free memory.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> I'll post a test that depends on this once this is worked out... one
> concern this raised is if we consider any future bugs in the inode
> initialization code (suppose we initialize some field once that should
> be initialized on every allocation, for example), this code has the
> potential to suppress such problems in debug mode. So an alternative to
> this approach is to perhaps tie this to an errortag and let the
> associated xfstests test enable it appropriately. Thoughts or
> preferences?

How about memset()ing the entire inode with a known poison value in
xfs_inode_free_callback and calling _init_once in xfs_inode_alloc
instead?  That way it'll be obvious that someone touched a poisoned
(free) inode.

--D

> Brian
> 
>  fs/xfs/xfs_icache.c | 5 ++++-
>  fs/xfs/xfs_super.c  | 2 +-
>  fs/xfs/xfs_super.h  | 1 +
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 9a18f69f6e96..86dc4c8a4e1d 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -111,7 +111,10 @@ xfs_inode_free_callback(
>  		xfs_inode_item_destroy(ip);
>  		ip->i_itemp = NULL;
>  	}
> -
> +#ifdef DEBUG
> +	/* facilitate catching use-after-free problems */
> +	xfs_fs_inode_init_once(ip);
> +#endif
>  	kmem_zone_free(xfs_inode_zone, ip);
>  }
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 612c1d5348b3..29b1be5dfebf 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1030,7 +1030,7 @@ xfs_fs_dirty_inode(
>   * fields in the xfs inode that left in the initialise state
>   * when freeing the inode.
>   */
> -STATIC void
> +void
>  xfs_fs_inode_init_once(
>  	void			*inode)
>  {
> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
> index 8cee8e8050e3..aae8a778f378 100644
> --- a/fs/xfs/xfs_super.h
> +++ b/fs/xfs/xfs_super.h
> @@ -70,6 +70,7 @@ struct block_device;
>  
>  extern void xfs_quiesce_attr(struct xfs_mount *mp);
>  extern void xfs_flush_inodes(struct xfs_mount *mp);
> +extern void xfs_fs_inode_init_once(void *);
>  extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
>  extern xfs_agnumber_t xfs_set_inode_alloc(struct xfs_mount *,
>  					   xfs_agnumber_t agcount);
> -- 
> 2.13.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html