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

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

[Lukas Czerner]

In xfs_ioc_trim it is possible that computing the last allocation group
to discard might overflow for big start & len values, because the result
might be bigger then xfs_agnumber_t which is 32 bit long. Fix this by not
allowing the start and end block of the range to be beyond the end of the
file system.

Note that if the start is beyond the end of the file system we have to
return -EINVAL, but in the "end" case we have to truncate it to the fs
size.

Also introduce "end" variable, rather than using start+len which which
might be more confusing to get right as this bug shows.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
v2: Use sb_dblocks instead of XFS_MAX_DBLOCKS to get max block count
v3: Rework the patch

 fs/xfs/xfs_discard.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 244e797..8a24f0c 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -38,7 +38,7 @@ xfs_trim_extents(
 	struct xfs_mount	*mp,
 	xfs_agnumber_t		agno,
 	xfs_fsblock_t		start,
-	xfs_fsblock_t		len,
+	xfs_fsblock_t		end,
 	xfs_fsblock_t		minlen,
 	__uint64_t		*blocks_trimmed)
 {
@@ -100,7 +100,7 @@ xfs_trim_extents(
 		 * down partially overlapping ranges for now.
 		 */
 		if (XFS_AGB_TO_FSB(mp, agno, fbno) + flen < start ||
-		    XFS_AGB_TO_FSB(mp, agno, fbno) >= start + len) {
+		    XFS_AGB_TO_FSB(mp, agno, fbno) > end) {
 			trace_xfs_discard_exclude(mp, agno, fbno, flen);
 			goto next_extent;
 		}
@@ -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, len, minlen;
+	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,19 @@ xfs_ioc_trim(
 	 * matter as trimming blocks is an advisory interface.
 	 */
 	start = XFS_B_TO_FSBT(mp, range.start);
-	len = XFS_B_TO_FSBT(mp, range.len);
+	end = start + XFS_B_TO_FSBT(mp, range.len) - 1;
 	minlen = XFS_B_TO_FSB(mp, max_t(u64, granularity, range.minlen));
 
-	start_agno = XFS_FSB_TO_AGNO(mp, start);
-	if (start_agno >= mp->m_sb.sb_agcount)
+	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;
 
-	end_agno = XFS_FSB_TO_AGNO(mp, start + len);
-	if (end_agno >= mp->m_sb.sb_agcount)
-		end_agno = mp->m_sb.sb_agcount - 1;
+	start_agno = XFS_FSB_TO_AGNO(mp, start);
+	end_agno = XFS_FSB_TO_AGNO(mp, end);
 
 	for (agno = start_agno; agno <= end_agno; agno++) {
-		error = -xfs_trim_extents(mp, agno, start, len, minlen,
+		error = -xfs_trim_extents(mp, agno, start, end, minlen,
 					  &blocks_trimmed);
 		if (error)
 			last_error = error;
-- 
1.7.4.4

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

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

[Christoph Hellwig]

On Wed, Sep 21, 2011 at 11:42:30AM +0200, Lukas Czerner wrote:
> In xfs_ioc_trim it is possible that computing the last allocation group
> to discard might overflow for big start & len values, because the result
> might be bigger then xfs_agnumber_t which is 32 bit long. Fix this by not
> allowing the start and end block of the range to be beyond the end of the
> file system.
> 
> Note that if the start is beyond the end of the file system we have to
> return -EINVAL, but in the "end" case we have to truncate it to the fs
> size.
> 
> Also introduce "end" variable, rather than using start+len which which
> might be more confusing to get right as this bug shows.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

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

[Alex Elder]

On Wed, 2011-09-21 at 11:42 +0200, Lukas Czerner wrote:
> In xfs_ioc_trim it is possible that computing the last allocation group
> to discard might overflow for big start & len values, because the result
> might be bigger then xfs_agnumber_t which is 32 bit long. Fix this by not
> allowing the start and end block of the range to be beyond the end of the
> file system.
> 
> Note that if the start is beyond the end of the file system we have to
> return -EINVAL, but in the "end" case we have to truncate it to the fs
> size.
> 
> Also introduce "end" variable, rather than using start+len which which
> might be more confusing to get right as this bug shows.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

There are cases where we're (still) not trimming
blocks within the range specified when we could.
I have an idea about how to do that but I'll send
it out later and will commit this as-is.

Reviewed-by: Alex Elder <aelder@sgi.com>



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

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

[Lukas Czerner]

On Fri, 23 Sep 2011, Alex Elder wrote:

> On Wed, 2011-09-21 at 11:42 +0200, Lukas Czerner wrote:
> > In xfs_ioc_trim it is possible that computing the last allocation group
> > to discard might overflow for big start & len values, because the result
> > might be bigger then xfs_agnumber_t which is 32 bit long. Fix this by not
> > allowing the start and end block of the range to be beyond the end of the
> > file system.
> > 
> > Note that if the start is beyond the end of the file system we have to
> > return -EINVAL, but in the "end" case we have to truncate it to the fs
> > size.
> > 
> > Also introduce "end" variable, rather than using start+len which which
> > might be more confusing to get right as this bug shows.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> There are cases where we're (still) not trimming
> blocks within the range specified when we could.
> I have an idea about how to do that but I'll send
> it out later and will commit this as-is.

What cases do you have in mind ? I have actually noticed that you are
discarding even more than user asked for in case that the free extent
and the range to trim overlaps, but that perfectly fine since the
duration of the discard command does not usually depends on the extent
size, so it is good thing to do.

> 
> Reviewed-by: Alex Elder <aelder@sgi.com>
> 

Thanks!
-Lukas

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

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

[Alex Elder]

On Fri, 2011-09-23 at 19:17 +0200, Lukas Czerner wrote:
> On Fri, 23 Sep 2011, Alex Elder wrote:
> 
> > On Wed, 2011-09-21 at 11:42 +0200, Lukas Czerner wrote:
> > > In xfs_ioc_trim it is possible that computing the last allocation group
> > > to discard might overflow for big start & len values, because the result
> > > might be bigger then xfs_agnumber_t which is 32 bit long. Fix this by not
> > > allowing the start and end block of the range to be beyond the end of the
> > > file system.
> > > 
> > > Note that if the start is beyond the end of the file system we have to
> > > return -EINVAL, but in the "end" case we have to truncate it to the fs
> > > size.
> > > 
> > > Also introduce "end" variable, rather than using start+len which which
> > > might be more confusing to get right as this bug shows.
> > > 
> > > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > 
> > There are cases where we're (still) not trimming
> > blocks within the range specified when we could.
> > I have an idea about how to do that but I'll send
> > it out later and will commit this as-is.
> 
> What cases do you have in mind ? I have actually noticed that you are
> discarding even more than user asked for in case that the free extent
> and the range to trim overlaps, but that perfectly fine since the
> duration of the discard command does not usually depends on the extent
> size, so it is good thing to do.

Suppose blocks from byte offset 4096 through 20480 are free,
a FSB is 4096 bytes, and minlen computed in xfs_ioc_trim()
is computed to be 4096.

Now suppose a request comes in to trim range 7680 = (8KB - 512B)
for 5KB = 5120 bytes (i.e., bytes 7680 through 12799).

In xfs_ioc_trim(), "start" will be truncated to FSB 1, byte offset
4096.  In computing "end", range.len will be truncated to 1 FSB
or 4096 bytes.  So the resulting "end" byte will be 4096 + 4096 - 1
or 8191, thereby *not* trimming FSB 2 at offset 8192B even though
it is entirely within the requested range, and was free.

As pointed out by Christoph this isn't a big deal because
it's an advisory interface.  I have a way to tighten this up
but it'll be easier to see with code, which I'll try to post
next week.

					-Alex

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