Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()

[Christoph Hellwig <hch@infradead.org>]

On Wed, Sep 07, 2011 at 02:26:54PM +0200, Lukas Czerner wrote:
> I do not think that start + len can overflow since we are doing
> XFS_B_TO_FSBT() on it first. Am I missing something ?

They don't have to overflow, but they can easily be outside
the range of valid AGs.

> > Care to update the test case to cover these cases as well?
> > 
> 
> I am not sure what do you mean ? There already is a check when both
> start and len are huge numbers. I am not sure if we can do more without
> significantly complicating the test to cover various start, or len
> numbers where can the fsblock->group_number overflow for various file
> systems.

Add a testcase where start is a relatively small number (smaller than an
AG/BG), but start + len is outside the fs.

> @@ -145,7 +145,7 @@ xfs_ioc_trim(
>  	struct request_queue	*q = mp->m_ddev_targp->bt_bdev->bd_disk->queue;
>  	unsigned int		granularity = q->limits.discard_granularity;
>  	struct fstrim_range	range;
> +	xfs_fsblock_t		start, end, minlen;
>  	xfs_agnumber_t		start_agno, end_agno, agno;
>  	__uint64_t		blocks_trimmed = 0;
>  	int			error, last_error = 0;
> @@ -165,19 +165,21 @@ xfs_ioc_trim(
>  	 * matter as trimming blocks is an advisory interface.
>  	 */
>  	start = XFS_B_TO_FSBT(mp, range.start);
> +	end = start + XFS_B_TO_FSBT(mp, range.len) - 1;
>  	minlen = XFS_B_TO_FSB(mp, max_t(u64, granularity, range.minlen));
>  
> +	if (start >= mp->m_sb.sb_dblocks)
>  		return -XFS_ERROR(EINVAL);
> +	start_agno = XFS_FSB_TO_AGNO(mp, start);
>  
> +	if (end >= mp->m_sb.sb_dblocks) {
> +		end = mp->m_sb.sb_dblocks - 1;
>  		end_agno = mp->m_sb.sb_agcount - 1;
> +	} else
> +		end_agno = XFS_FSB_TO_AGNO(mp, end);

I'd rather do something like:

	if (start >= mp->m_sb.sb_dblocks)
  		return -XFS_ERROR(EINVAL);
	if (end > mp->m_sb.sb_dblocks - 1)
		end = mp->m_sb.sb_dblocks - 1;


	start_agno = XFS_FSB_TO_AGNO(mp, start);
	end_agno = XFS_FSB_TO_AGNO(mp, end)

here.

Otherwise the patch looks fine.

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