public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xfs: avoid underflow in xfs_ioc_trim()
@ 2012-10-09 12:15 Lukas Czerner
  2012-10-09 13:42 ` Carlos Maiolino
  2012-10-09 19:39 ` Dave Chinner
  0 siblings, 2 replies; 6+ messages in thread
From: Lukas Czerner @ 2012-10-09 12:15 UTC (permalink / raw)
  To: xfs; +Cc: Lukas Czerner

Currently if len argument in xfs_ioc_trim() is smaller than one BB
(basic block) the 'end' variable underflow. Avoid that by bailing out if
len is smaller than BB.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/xfs/xfs_discard.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 69cf4fc..54dc58a 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -183,8 +183,12 @@ xfs_ioc_trim(
 	    range.minlen > XFS_FSB_TO_B(mp, XFS_ALLOC_AG_MAX_USABLE(mp)))
 		return -XFS_ERROR(EINVAL);
 
+	end = BTOBBT(range.len);
+	if (0 == end)
+		goto out;
+
 	start = BTOBB(range.start);
-	end = start + BTOBBT(range.len) - 1;
+	end += start - 1;
 	minlen = BTOBB(max_t(u64, granularity, range.minlen));
 
 	if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1)
@@ -203,6 +207,7 @@ xfs_ioc_trim(
 	if (last_error)
 		return last_error;
 
+out:
 	range.len = XFS_FSB_TO_B(mp, blocks_trimmed);
 	if (copy_to_user(urange, &range, sizeof(range)))
 		return -XFS_ERROR(EFAULT);
-- 
1.7.7.6

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
  2012-10-09 12:15 [PATCH] xfs: avoid underflow in xfs_ioc_trim() Lukas Czerner
@ 2012-10-09 13:42 ` Carlos Maiolino
  2012-10-09 19:39 ` Dave Chinner
  1 sibling, 0 replies; 6+ messages in thread
From: Carlos Maiolino @ 2012-10-09 13:42 UTC (permalink / raw)
  To: xfs

On Tue, Oct 09, 2012 at 02:15:45PM +0200, Lukas Czerner wrote:
> Currently if len argument in xfs_ioc_trim() is smaller than one BB
> (basic block) the 'end' variable underflow. Avoid that by bailing out if
> len is smaller than BB.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/xfs/xfs_discard.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 69cf4fc..54dc58a 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -183,8 +183,12 @@ xfs_ioc_trim(
>  	    range.minlen > XFS_FSB_TO_B(mp, XFS_ALLOC_AG_MAX_USABLE(mp)))
>  		return -XFS_ERROR(EINVAL);
>  
> +	end = BTOBBT(range.len);
> +	if (0 == end)
> +		goto out;
> +
>  	start = BTOBB(range.start);
> -	end = start + BTOBBT(range.len) - 1;
> +	end += start - 1;
>  	minlen = BTOBB(max_t(u64, granularity, range.minlen));
>  
>  	if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1)
> @@ -203,6 +207,7 @@ xfs_ioc_trim(
>  	if (last_error)
>  		return last_error;
>  
> +out:
>  	range.len = XFS_FSB_TO_B(mp, blocks_trimmed);
>  	if (copy_to_user(urange, &range, sizeof(range)))
>  		return -XFS_ERROR(EFAULT);
> -- 
> 1.7.7.6
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

Looks good,

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
-- 
--Carlos

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
  2012-10-09 12:15 [PATCH] xfs: avoid underflow in xfs_ioc_trim() Lukas Czerner
  2012-10-09 13:42 ` Carlos Maiolino
@ 2012-10-09 19:39 ` Dave Chinner
  2012-10-10  7:50   ` Lukáš Czerner
  1 sibling, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2012-10-09 19:39 UTC (permalink / raw)
  To: Lukas Czerner; +Cc: xfs

On Tue, Oct 09, 2012 at 02:15:45PM +0200, Lukas Czerner wrote:
> Currently if len argument in xfs_ioc_trim() is smaller than one BB
> (basic block) the 'end' variable underflow. Avoid that by bailing out if
> len is smaller than BB.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/xfs/xfs_discard.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 69cf4fc..54dc58a 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -183,8 +183,12 @@ xfs_ioc_trim(
>  	    range.minlen > XFS_FSB_TO_B(mp, XFS_ALLOC_AG_MAX_USABLE(mp)))
>  		return -XFS_ERROR(EINVAL);
>  
> +	end = BTOBBT(range.len);
> +	if (0 == end)
> +		goto out;

Uggh. "if (end == 0)", please.
> +
>  	start = BTOBB(range.start);
> -	end = start + BTOBBT(range.len) - 1;
> +	end += start - 1;

Better would be to check if end <= start. That way it also catches
start+len overflows.


>  	minlen = BTOBB(max_t(u64, granularity, range.minlen));
>  
>  	if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1)
> @@ -203,6 +207,7 @@ xfs_ioc_trim(
>  	if (last_error)
>  		return last_error;
>  
> +out:
>  	range.len = XFS_FSB_TO_B(mp, blocks_trimmed);
>  	if (copy_to_user(urange, &range, sizeof(range)))
>  		return -XFS_ERROR(EFAULT);

I think it should return EINVAL, not silently do nothing. If the
user application uses a loop that increments start/len based on the
returned amount of blocks trimmed, returning zero could send it into
an endless loop.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
  2012-10-09 19:39 ` Dave Chinner
@ 2012-10-10  7:50   ` Lukáš Czerner
  2012-10-10 23:22     ` Dave Chinner
  0 siblings, 1 reply; 6+ messages in thread
From: Lukáš Czerner @ 2012-10-10  7:50 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Lukas Czerner, xfs

On Wed, 10 Oct 2012, Dave Chinner wrote:

> Date: Wed, 10 Oct 2012 06:39:08 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Lukas Czerner <lczerner@redhat.com>
> Cc: xfs@oss.sgi.com
> Subject: Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
> 
> On Tue, Oct 09, 2012 at 02:15:45PM +0200, Lukas Czerner wrote:
> > Currently if len argument in xfs_ioc_trim() is smaller than one BB
> > (basic block) the 'end' variable underflow. Avoid that by bailing out if
> > len is smaller than BB.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  fs/xfs/xfs_discard.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> > index 69cf4fc..54dc58a 100644
> > --- a/fs/xfs/xfs_discard.c
> > +++ b/fs/xfs/xfs_discard.c
> > @@ -183,8 +183,12 @@ xfs_ioc_trim(
> >  	    range.minlen > XFS_FSB_TO_B(mp, XFS_ALLOC_AG_MAX_USABLE(mp)))
> >  		return -XFS_ERROR(EINVAL);
> >  
> > +	end = BTOBBT(range.len);
> > +	if (0 == end)
> > +		goto out;
> 
> Uggh. "if (end == 0)", please.

Not a Star Wars fan then ? ;). Ok, I'll change that.

> > +
> >  	start = BTOBB(range.start);
> > -	end = start + BTOBBT(range.len) - 1;
> > +	end += start - 1;
> 
> Better would be to check if end <= start. That way it also catches
> start+len overflows.

That's not possible. Even if range.start and range.len would be
2^64-1 (which is not possible for range.start) we always do the
BTOBB conversion and ((2^64-1) / 512) * 2 always fits the _s64
type.

> 
> 
> >  	minlen = BTOBB(max_t(u64, granularity, range.minlen));
> >  
> >  	if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1)
> > @@ -203,6 +207,7 @@ xfs_ioc_trim(
> >  	if (last_error)
> >  		return last_error;
> >  
> > +out:
> >  	range.len = XFS_FSB_TO_B(mp, blocks_trimmed);
> >  	if (copy_to_user(urange, &range, sizeof(range)))
> >  		return -XFS_ERROR(EFAULT);
> 
> I think it should return EINVAL, not silently do nothing. If the
> user application uses a loop that increments start/len based on the
> returned amount of blocks trimmed, returning zero could send it into
> an endless loop.

That's not what the application would do. At least it would not set
len to what's returned as number of blocs discarded, it does not
make sense.

However if user specify length smaller than what we're able to
discard (in xfs it is BBSIZE if I am not mistaken?), then it
probably make sense to return -EINVAL. It is similar situation of
minlen, where we return -EINVAL if it is bigger than AG. However
this will make it to fail at different threshold on different file
system / block sizes so I am on the fence about it. What do you
think, is it worth it ?

Thanks!
-Lukas

> 
> Cheers,
> 
> Dave.
> 

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
  2012-10-10  7:50   ` Lukáš Czerner
@ 2012-10-10 23:22     ` Dave Chinner
  2012-10-11  6:05       ` Lukáš Czerner
  0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2012-10-10 23:22 UTC (permalink / raw)
  To: Lukáš Czerner; +Cc: xfs

On Wed, Oct 10, 2012 at 09:50:38AM +0200, Lukáš Czerner wrote:
> On Wed, 10 Oct 2012, Dave Chinner wrote:
> 
> > Date: Wed, 10 Oct 2012 06:39:08 +1100
> > From: Dave Chinner <david@fromorbit.com>
> > To: Lukas Czerner <lczerner@redhat.com>
> > Cc: xfs@oss.sgi.com
> > Subject: Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
> > 
> > On Tue, Oct 09, 2012 at 02:15:45PM +0200, Lukas Czerner wrote:
> > > Currently if len argument in xfs_ioc_trim() is smaller than one BB
> > > (basic block) the 'end' variable underflow. Avoid that by bailing out if
> > > len is smaller than BB.
> > > 
> > > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > > ---
> > >  fs/xfs/xfs_discard.c |    7 ++++++-
> > >  1 files changed, 6 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> > > index 69cf4fc..54dc58a 100644
> > > --- a/fs/xfs/xfs_discard.c
> > > +++ b/fs/xfs/xfs_discard.c
> > > @@ -183,8 +183,12 @@ xfs_ioc_trim(
> > >  	    range.minlen > XFS_FSB_TO_B(mp, XFS_ALLOC_AG_MAX_USABLE(mp)))
> > >  		return -XFS_ERROR(EINVAL);
> > >  
> > > +	end = BTOBBT(range.len);
> > > +	if (0 == end)
> > > +		goto out;
> > 
> > Uggh. "if (end == 0)", please.
> 
> Not a Star Wars fan then ? ;). Ok, I'll change that.

I might be, but I don't get the reference - something to do with the
second moon of Endor, or just letting the wookie win so you don't
come to an armless end? :)

As it is, I don't like reverse logic notation because it makes code
strange and unreadable. It doesn't provide any real benefit because
the compiler will warn about typos that result in assignments in
such statements (i.e.  if (end = 0)). Hence writing code so that the
compilation will fail if you make a typo isn't necessary - Clear,
consistent and easily readable code is much more important....

> > > +
> > >  	start = BTOBB(range.start);
> > > -	end = start + BTOBBT(range.len) - 1;
> > > +	end += start - 1;
> > 
> > Better would be to check if end <= start. That way it also catches
> > start+len overflows.
> 
> That's not possible. Even if range.start and range.len would be
> 2^64-1 (which is not possible for range.start) we always do the
> BTOBB conversion and ((2^64-1) / 512) * 2 always fits the _s64
> type.

Regardless, checking that the end of the range is after the start is a
much more robust check than individual underflow/overflow tests. We
do that all over the place in XFS when converting byte ranges to
block based start/end pairs.

> > >  	minlen = BTOBB(max_t(u64, granularity, range.minlen));
> > >  
> > >  	if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1)
> > > @@ -203,6 +207,7 @@ xfs_ioc_trim(
> > >  	if (last_error)
> > >  		return last_error;
> > >  
> > > +out:
> > >  	range.len = XFS_FSB_TO_B(mp, blocks_trimmed);
> > >  	if (copy_to_user(urange, &range, sizeof(range)))
> > >  		return -XFS_ERROR(EFAULT);
> > 
> > I think it should return EINVAL, not silently do nothing. If the
> > user application uses a loop that increments start/len based on the
> > returned amount of blocks trimmed, returning zero could send it into
> > an endless loop.
> 
> That's not what the application would do. At least it would not set
> len to what's returned as number of blocs discarded, it does not
> make sense.

Oh, right, my mistake.

> However if user specify length smaller than what we're able to
> discard (in xfs it is BBSIZE if I am not mistaken?), then it
> probably make sense to return -EINVAL. It is similar situation of
> minlen, where we return -EINVAL if it is bigger than AG. However
> this will make it to fail at different threshold on different file
> system / block sizes so I am on the fence about it. What do you
> think, is it worth it ?

Actually, the minimum length that can be discarded is the filesystem
block size, and applications can get that easily enough from
stat(2).  IOWs, we probably should be rejecting minlen < 1 FSB,
range < 1 FSB with EINVAL....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
  2012-10-10 23:22     ` Dave Chinner
@ 2012-10-11  6:05       ` Lukáš Czerner
  0 siblings, 0 replies; 6+ messages in thread
From: Lukáš Czerner @ 2012-10-11  6:05 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Lukáš Czerner, xfs

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4888 bytes --]

On Thu, 11 Oct 2012, Dave Chinner wrote:

> Date: Thu, 11 Oct 2012 10:22:41 +1100
> From: Dave Chinner <david@fromorbit.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: xfs@oss.sgi.com
> Subject: Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
> 
> On Wed, Oct 10, 2012 at 09:50:38AM +0200, Lukáš Czerner wrote:
> > On Wed, 10 Oct 2012, Dave Chinner wrote:
> > 
> > > Date: Wed, 10 Oct 2012 06:39:08 +1100
> > > From: Dave Chinner <david@fromorbit.com>
> > > To: Lukas Czerner <lczerner@redhat.com>
> > > Cc: xfs@oss.sgi.com
> > > Subject: Re: [PATCH] xfs: avoid underflow in xfs_ioc_trim()
> > > 
> > > On Tue, Oct 09, 2012 at 02:15:45PM +0200, Lukas Czerner wrote:
> > > > Currently if len argument in xfs_ioc_trim() is smaller than one BB
> > > > (basic block) the 'end' variable underflow. Avoid that by bailing out if
> > > > len is smaller than BB.
> > > > 
> > > > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > > > ---
> > > >  fs/xfs/xfs_discard.c |    7 ++++++-
> > > >  1 files changed, 6 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> > > > index 69cf4fc..54dc58a 100644
> > > > --- a/fs/xfs/xfs_discard.c
> > > > +++ b/fs/xfs/xfs_discard.c
> > > > @@ -183,8 +183,12 @@ xfs_ioc_trim(
> > > >  	    range.minlen > XFS_FSB_TO_B(mp, XFS_ALLOC_AG_MAX_USABLE(mp)))
> > > >  		return -XFS_ERROR(EINVAL);
> > > >  
> > > > +	end = BTOBBT(range.len);
> > > > +	if (0 == end)
> > > > +		goto out;
> > > 
> > > Uggh. "if (end == 0)", please.
> > 
> > Not a Star Wars fan then ? ;). Ok, I'll change that.
> 
> I might be, but I don't get the reference - something to do with the
> second moon of Endor, or just letting the wookie win so you don't
> come to an armless end? :)

I know it under the name "yoda notation", or "yoda condition",
probably based on the Yoda's strange sentence composition which is
usually kind of backwards :).

> 
> As it is, I don't like reverse logic notation because it makes code
> strange and unreadable. It doesn't provide any real benefit because
> the compiler will warn about typos that result in assignments in
> such statements (i.e.  if (end = 0)). Hence writing code so that the
> compilation will fail if you make a typo isn't necessary - Clear,
> consistent and easily readable code is much more important....

Good point.

> 
> > > > +
> > > >  	start = BTOBB(range.start);
> > > > -	end = start + BTOBBT(range.len) - 1;
> > > > +	end += start - 1;
> > > 
> > > Better would be to check if end <= start. That way it also catches
> > > start+len overflows.
> > 
> > That's not possible. Even if range.start and range.len would be
> > 2^64-1 (which is not possible for range.start) we always do the
> > BTOBB conversion and ((2^64-1) / 512) * 2 always fits the _s64
> > type.
> 
> Regardless, checking that the end of the range is after the start is a
> much more robust check than individual underflow/overflow tests. We
> do that all over the place in XFS when converting byte ranges to
> block based start/end pairs.

Fair enough.

> 
> > > >  	minlen = BTOBB(max_t(u64, granularity, range.minlen));
> > > >  
> > > >  	if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1)
> > > > @@ -203,6 +207,7 @@ xfs_ioc_trim(
> > > >  	if (last_error)
> > > >  		return last_error;
> > > >  
> > > > +out:
> > > >  	range.len = XFS_FSB_TO_B(mp, blocks_trimmed);
> > > >  	if (copy_to_user(urange, &range, sizeof(range)))
> > > >  		return -XFS_ERROR(EFAULT);
> > > 
> > > I think it should return EINVAL, not silently do nothing. If the
> > > user application uses a loop that increments start/len based on the
> > > returned amount of blocks trimmed, returning zero could send it into
> > > an endless loop.
> > 
> > That's not what the application would do. At least it would not set
> > len to what's returned as number of blocs discarded, it does not
> > make sense.
> 
> Oh, right, my mistake.
> 
> > However if user specify length smaller than what we're able to
> > discard (in xfs it is BBSIZE if I am not mistaken?), then it
> > probably make sense to return -EINVAL. It is similar situation of
> > minlen, where we return -EINVAL if it is bigger than AG. However
> > this will make it to fail at different threshold on different file
> > system / block sizes so I am on the fence about it. What do you
> > think, is it worth it ?
> 
> Actually, the minimum length that can be discarded is the filesystem
> block size, and applications can get that easily enough from
> stat(2).  IOWs, we probably should be rejecting minlen < 1 FSB,
> range < 1 FSB with EINVAL....

Agreed, we should return EINVAL when range < 1 FSB. However 'minlen'
is a different case, when minlen < 1 FSB. Minlen says that we should
not discard ranges smaller than certain value which is perfectly
feasible with minlen < 1 FSB.

I'll resend the patch, thanks!
-Lukas

> 
> Cheers,
> 
> Dave.
> 

[-- Attachment #2: Type: text/plain, Size: 121 bytes --]

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-11  6:03 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-09 12:15 [PATCH] xfs: avoid underflow in xfs_ioc_trim() Lukas Czerner
2012-10-09 13:42 ` Carlos Maiolino
2012-10-09 19:39 ` Dave Chinner
2012-10-10  7:50   ` Lukáš Czerner
2012-10-10 23:22     ` Dave Chinner
2012-10-11  6:05       ` Lukáš Czerner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox