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

[Alex Elder <aelder@sgi.com>]

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