All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
@ 2011-09-06 15:29 Lukas Czerner
  2011-09-06 15:33 ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Lukas Czerner @ 2011-09-06 15:29 UTC (permalink / raw)
  To: xfs; +Cc: hch, Lukas Czerner

In xfs_ioc_trim it is possible that start+len might overflow. Fix it by
decrementing the len so that start+len equals to the file system size in
the worst case.

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

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

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 244e797..b45e3c9 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -146,6 +146,7 @@ xfs_ioc_trim(
 	unsigned int		granularity = q->limits.discard_granularity;
 	struct fstrim_range	range;
 	xfs_fsblock_t		start, len, minlen;
+	xfs_fsblock_t		max_blks = mp->m_sb.sb_dblocks;
 	xfs_agnumber_t		start_agno, end_agno, agno;
 	__uint64_t		blocks_trimmed = 0;
 	int			error, last_error = 0;
@@ -171,7 +172,8 @@ xfs_ioc_trim(
 	start_agno = XFS_FSB_TO_AGNO(mp, start);
 	if (start_agno >= mp->m_sb.sb_agcount)
 		return -XFS_ERROR(EINVAL);
-
+	if (len > max_blks)
+		len = max_blks - start;
 	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;
-- 
1.7.4.4

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
  2011-09-06 15:29 [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim() Lukas Czerner
@ 2011-09-06 15:33 ` Christoph Hellwig
  2011-09-07 10:05   ` Lukas Czerner
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2011-09-06 15:33 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: hch, xfs

On Tue, Sep 06, 2011 at 05:29:37PM +0200, Lukas Czerner wrote:
> In xfs_ioc_trim it is possible that start+len might overflow. Fix it by
> decrementing the len so that start+len equals to the file system size in
> the worst case.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

> @@ -146,6 +146,7 @@ xfs_ioc_trim(
>  	unsigned int		granularity = q->limits.discard_granularity;
>  	struct fstrim_range	range;
>  	xfs_fsblock_t		start, len, minlen;
> +	xfs_fsblock_t		max_blks = mp->m_sb.sb_dblocks;
>  	xfs_agnumber_t		start_agno, end_agno, agno;
>  	__uint64_t		blocks_trimmed = 0;
>  	int			error, last_error = 0;
> @@ -171,7 +172,8 @@ xfs_ioc_trim(
>  	start_agno = XFS_FSB_TO_AGNO(mp, start);
>  	if (start_agno >= mp->m_sb.sb_agcount)
>  		return -XFS_ERROR(EINVAL);
> -
> +	if (len > max_blks)
> +		len = max_blks - start;

Is this really the correct check?

Shouldn't it be

	if (start + len > max_blks)
		len = max_blks - start;

I'd also just use the mp->m_sb.sb_dblocks value directly instead
of assigning it to a local variable.

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
  2011-09-06 15:33 ` Christoph Hellwig
@ 2011-09-07 10:05   ` Lukas Czerner
  2011-09-07 11:21     ` Christoph Hellwig
  0 siblings, 1 reply; 8+ messages in thread
From: Lukas Czerner @ 2011-09-07 10:05 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Lukas Czerner, xfs

On Tue, 6 Sep 2011, Christoph Hellwig wrote:

> On Tue, Sep 06, 2011 at 05:29:37PM +0200, Lukas Czerner wrote:
> > In xfs_ioc_trim it is possible that start+len might overflow. Fix it by
> > decrementing the len so that start+len equals to the file system size in
> > the worst case.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> > @@ -146,6 +146,7 @@ xfs_ioc_trim(
> >  	unsigned int		granularity = q->limits.discard_granularity;
> >  	struct fstrim_range	range;
> >  	xfs_fsblock_t		start, len, minlen;
> > +	xfs_fsblock_t		max_blks = mp->m_sb.sb_dblocks;
> >  	xfs_agnumber_t		start_agno, end_agno, agno;
> >  	__uint64_t		blocks_trimmed = 0;
> >  	int			error, last_error = 0;
> > @@ -171,7 +172,8 @@ xfs_ioc_trim(
> >  	start_agno = XFS_FSB_TO_AGNO(mp, start);
> >  	if (start_agno >= mp->m_sb.sb_agcount)
> >  		return -XFS_ERROR(EINVAL);
> > -
> > +	if (len > max_blks)
> > +		len = max_blks - start;
> 
> Is this really the correct check?
> 
> Shouldn't it be
> 
> 	if (start + len > max_blks)
> 		len = max_blks - start;
> 
> I'd also just use the mp->m_sb.sb_dblocks value directly instead
> of assigning it to a local variable.
> 

Agh, you're right. I am bit too hasty I guess. I thought that

if (start_agno >= mp->m_sb.sb_agcount)
	return -XFS_ERROR(EINVAL);

will cover us from the unreasonably big start, however if the file
system has really huge number of AGs than it will fail to prevent the
overflow, I am not sure if that is possible to happen, but what you
proposed is definitely better.

Thanks!
-Lukas

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
  2011-09-07 10:05   ` Lukas Czerner
@ 2011-09-07 11:21     ` Christoph Hellwig
  2011-09-07 12:26       ` Lukas Czerner
  0 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2011-09-07 11:21 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: Christoph Hellwig, xfs

On Wed, Sep 07, 2011 at 12:05:14PM +0200, Lukas Czerner wrote:
> > > +	if (len > max_blks)
> > > +		len = max_blks - start;
> > 
> > Is this really the correct check?
> > 
> > Shouldn't it be
> > 
> > 	if (start + len > max_blks)
> > 		len = max_blks - start;
> > 
> > I'd also just use the mp->m_sb.sb_dblocks value directly instead
> > of assigning it to a local variable.
> > 
> 
> Agh, you're right. I am bit too hasty I guess. I thought that
> 
> if (start_agno >= mp->m_sb.sb_agcount)
> 	return -XFS_ERROR(EINVAL);
> 
> will cover us from the unreasonably big start, however if the file
> system has really huge number of AGs than it will fail to prevent the
> overflow, I am not sure if that is possible to happen, but what you
> proposed is definitely better.

The problem is that start could be very far into the fs, so checking
len alone won't help very much.  And we probably want a check if 
start + len is overflowing, too.

Care to update the test case to cover these cases as well?

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
  2011-09-07 11:21     ` Christoph Hellwig
@ 2011-09-07 12:26       ` Lukas Czerner
  2011-09-20 13:36         ` Lukas Czerner
  2011-09-20 17:12         ` Christoph Hellwig
  0 siblings, 2 replies; 8+ messages in thread
From: Lukas Czerner @ 2011-09-07 12:26 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Lukas Czerner, xfs

On Wed, 7 Sep 2011, Christoph Hellwig wrote:

> On Wed, Sep 07, 2011 at 12:05:14PM +0200, Lukas Czerner wrote:
> > > > +	if (len > max_blks)
> > > > +		len = max_blks - start;
> > > 
> > > Is this really the correct check?
> > > 
> > > Shouldn't it be
> > > 
> > > 	if (start + len > max_blks)
> > > 		len = max_blks - start;
> > > 
> > > I'd also just use the mp->m_sb.sb_dblocks value directly instead
> > > of assigning it to a local variable.
> > > 
> > 
> > Agh, you're right. I am bit too hasty I guess. I thought that
> > 
> > if (start_agno >= mp->m_sb.sb_agcount)
> > 	return -XFS_ERROR(EINVAL);
> > 
> > will cover us from the unreasonably big start, however if the file
> > system has really huge number of AGs than it will fail to prevent the
> > overflow, I am not sure if that is possible to happen, but what you
> > proposed is definitely better.
> 
> The problem is that start could be very far into the fs, so checking
> len alone won't help very much.  And we probably want a check if 
> start + len is overflowing, too.

I do not think that start + len can overflow since we are doing
XFS_B_TO_FSBT() on it first. Am I missing something ?

The commit description is a bit misleading since the overflow can happen
when storing the value of XFS_FSB_TO_AGNO() rather than start
+ len alone. I'll update it as well.

Also this is wrong:

	start_agno = XFS_FSB_TO_AGNO(mp, start);
	if (start_agno >= mp->m_sb.sb_agcount)
		return -XFS_ERROR(EINVAL);

because XFS_FSB_TO_AGNO() might overflow as I mentioned above. This is
very similar problem to ext4 as well, but it is kind of hard to test
since the both block size and AF might be different, moreover in XFS
AG differs with the different fs size.

> 
> 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.

Thanks!
-Lukas

Anyway what about this patch ?

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

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>
---
 fs/xfs/xfs_discard.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 244e797..5ef3568 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,21 @@ 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);
+	start_agno = XFS_FSB_TO_AGNO(mp, start);
 
-	end_agno = XFS_FSB_TO_AGNO(mp, start + len);
-	if (end_agno >= mp->m_sb.sb_agcount)
+	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);
 
 	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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
  2011-09-07 12:26       ` Lukas Czerner
@ 2011-09-20 13:36         ` Lukas Czerner
  2011-09-20 17:12         ` Christoph Hellwig
  1 sibling, 0 replies; 8+ messages in thread
From: Lukas Czerner @ 2011-09-20 13:36 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: Christoph Hellwig, xfs

On Wed, 7 Sep 2011, Lukas Czerner wrote:

> On Wed, 7 Sep 2011, Christoph Hellwig wrote:
> 
> > On Wed, Sep 07, 2011 at 12:05:14PM +0200, Lukas Czerner wrote:
> > > > > +	if (len > max_blks)
> > > > > +		len = max_blks - start;
> > > > 
> > > > Is this really the correct check?
> > > > 
> > > > Shouldn't it be
> > > > 
> > > > 	if (start + len > max_blks)
> > > > 		len = max_blks - start;
> > > > 
> > > > I'd also just use the mp->m_sb.sb_dblocks value directly instead
> > > > of assigning it to a local variable.
> > > > 
> > > 
> > > Agh, you're right. I am bit too hasty I guess. I thought that
> > > 
> > > if (start_agno >= mp->m_sb.sb_agcount)
> > > 	return -XFS_ERROR(EINVAL);
> > > 
> > > will cover us from the unreasonably big start, however if the file
> > > system has really huge number of AGs than it will fail to prevent the
> > > overflow, I am not sure if that is possible to happen, but what you
> > > proposed is definitely better.
> > 
> > The problem is that start could be very far into the fs, so checking
> > len alone won't help very much.  And we probably want a check if 
> > start + len is overflowing, too.
> 
> I do not think that start + len can overflow since we are doing
> XFS_B_TO_FSBT() on it first. Am I missing something ?
> 
> The commit description is a bit misleading since the overflow can happen
> when storing the value of XFS_FSB_TO_AGNO() rather than start
> + len alone. I'll update it as well.
> 
> Also this is wrong:
> 
> 	start_agno = XFS_FSB_TO_AGNO(mp, start);
> 	if (start_agno >= mp->m_sb.sb_agcount)
> 		return -XFS_ERROR(EINVAL);
> 
> because XFS_FSB_TO_AGNO() might overflow as I mentioned above. This is
> very similar problem to ext4 as well, but it is kind of hard to test
> since the both block size and AF might be different, moreover in XFS
> AG differs with the different fs size.
> 
> > 
> > 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.
> 
> Thanks!
> -Lukas
> 
> Anyway what about this patch ?

Hi Christoph,

have you had a chance to look at this patch ?

Thanks!
-Lukas

> 
> Subject: [PATCH] xfs: fix possible overflow in xfs_ioc_trim()
> 
> 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>
> ---
>  fs/xfs/xfs_discard.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 244e797..5ef3568 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,21 @@ 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);
> +	start_agno = XFS_FSB_TO_AGNO(mp, start);
>  
> -	end_agno = XFS_FSB_TO_AGNO(mp, start + len);
> -	if (end_agno >= mp->m_sb.sb_agcount)
> +	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);
>  
>  	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;
> 

-- 

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
  2011-09-07 12:26       ` Lukas Czerner
  2011-09-20 13:36         ` Lukas Czerner
@ 2011-09-20 17:12         ` Christoph Hellwig
  2011-09-21  7:46           ` Lukas Czerner
  1 sibling, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2011-09-20 17:12 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: Christoph Hellwig, xfs

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim()
  2011-09-20 17:12         ` Christoph Hellwig
@ 2011-09-21  7:46           ` Lukas Czerner
  0 siblings, 0 replies; 8+ messages in thread
From: Lukas Czerner @ 2011-09-21  7:46 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Lukas Czerner, xfs

On Tue, 20 Sep 2011, Christoph Hellwig wrote:

> > 
> > 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.

Already done in the second version of the xfstests patch.

> 
> > @@ -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.
> 

Thanks!
-Lukas

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2011-09-21  7:47 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-06 15:29 [PATCH v2] xfs: fix possible overflow in xfs_ioc_trim() Lukas Czerner
2011-09-06 15:33 ` Christoph Hellwig
2011-09-07 10:05   ` Lukas Czerner
2011-09-07 11:21     ` Christoph Hellwig
2011-09-07 12:26       ` Lukas Czerner
2011-09-20 13:36         ` Lukas Czerner
2011-09-20 17:12         ` Christoph Hellwig
2011-09-21  7:46           ` Lukas Czerner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.