On Thu, 11 Oct 2012, Dave Chinner wrote: > Date: Thu, 11 Oct 2012 10:22:41 +1100 > From: Dave Chinner <david@xxxxxxxxxxxxx> > To: Lukáš Czerner <lczerner@xxxxxxxxxx> > Cc: xfs@xxxxxxxxxxx > 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@xxxxxxxxxxxxx> > > > To: Lukas Czerner <lczerner@xxxxxxxxxx> > > > Cc: xfs@xxxxxxxxxxx > > > 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@xxxxxxxxxx> > > > > --- > > > > 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. >
_______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
