Re: [patch 22/23] fs: check for statfs overflow

Re: [patch 22/23] fs: check for statfs overflow

[Nick Piggin]

On Tue, May 27, 2008 at 12:19:53PM -0500, Jon Tollefson wrote:
> Nishanth Aravamudan wrote:
> > On 26.05.2008 [00:23:39 +1000], npiggin@suse.de wrote:
> >   
> >> Adds a check for an overflow in the filesystem size so if someone is
> >> checking with statfs() on a 16G hugetlbfs  in a 32bit binary that it
> >> will report back EOVERFLOW instead of a size of 0.
> >>
> >> Are other places that need a similar check?  I had tried a similar
> >> check in put_compat_statfs64 too but it didn't seem to generate an
> >> EOVERFLOW in my test case.
> >>     
> >
> > I think this part of the changelog was meant to be a post-"---"
> > question, which I don't have an answer for, but probably shouldn't go in
> > the final changelog?
> >   
> You are correct.

I think the question is OK for the changelog. Unless we can get
somebody answering it yes or no, I'll leave it (but I'd rather get
an answer first).

I'm pretty unfamiliar with how the APIs work, but I'd think statfs64
is less likely to overflow because f_blocks is likely to be 8 bytes.
But I still think the check might be good to have.

The non-compat stat() (and stat64 even) might also need the eoverflow
check. cc'ing fsdevel with the patch attached again.

---

fs: check for statfs overflow

Adds a check for an overflow in the filesystem size so if someone is
checking with statfs() on a 16G hugetlbfs  in a 32bit binary that it
will report back EOVERFLOW instead of a size of 0.

Are other places that need a similar check?  I had tried a similar
check in put_compat_statfs64 too but it didn't seem to generate an
EOVERFLOW in my test case.

Signed-off-by: Jon Tollefson <kniht@linux.vnet.ibm.com>
Signed-off-by: Nick Piggin <npiggin@suse.de>
---

 fs/compat.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


Index: linux-2.6/fs/compat.c
===================================================================
--- linux-2.6.orig/fs/compat.c
+++ linux-2.6/fs/compat.c
@@ -197,8 +197,8 @@ static int put_compat_statfs(struct comp
 {
 	
 	if (sizeof ubuf->f_blocks == 4) {
-		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail) &
-		    0xffffffff00000000ULL)
+		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
+		     kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
 			return -EOVERFLOW;
 		/* f_files and f_ffree may be -1; it's okay
 		 * to stuff that into 32 bits */

Re: [patch 22/23] fs: check for statfs overflow

[Andreas Dilger]

On May 28, 2008  11:02 +0200, Nick Piggin wrote:
> fs: check for statfs overflow
> 
> Adds a check for an overflow in the filesystem size so if someone is
> checking with statfs() on a 16G hugetlbfs  in a 32bit binary that it
> will report back EOVERFLOW instead of a size of 0.
> 
> Are other places that need a similar check?  I had tried a similar
> check in put_compat_statfs64 too but it didn't seem to generate an
> EOVERFLOW in my test case.
> 
> Signed-off-by: Jon Tollefson <kniht@linux.vnet.ibm.com>
> Signed-off-by: Nick Piggin <npiggin@suse.de>
> ---
> 
>  fs/compat.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> Index: linux-2.6/fs/compat.c
> ===================================================================
> --- linux-2.6.orig/fs/compat.c
> +++ linux-2.6/fs/compat.c
> @@ -197,8 +197,8 @@ static int put_compat_statfs(struct comp
>  {
>  	
>  	if (sizeof ubuf->f_blocks == 4) {
> -		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail) &
> -		    0xffffffff00000000ULL)
> +		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
> +		     kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
>  			return -EOVERFLOW;

Hmm, doesn't this check break every filesystem > 16TB on 4kB PAGE_SIZE
nodes?  It would be better, IMHO, to scale down f_blocks, f_bfree, and
f_bavail and correspondingly scale up f_bsize to fit into the 32-bit
statfs structure.

We did this for several years with Lustre, as the first installation was
already larger than 16TB on 32-bit clients at the time.  There was never
a problem with statfs returning a larger f_bsize, since applications
generally use the fstat() st_blocksize to determine IO size and not the
statfs() data.

Returning statfs data accurate to within a few kB is better than failing
the request outright, IMHO.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [patch 22/23] fs: check for statfs overflow

[Nishanth Aravamudan]

On 29.05.2008 [17:56:07 -0600], Andreas Dilger wrote:
> On May 28, 2008  11:02 +0200, Nick Piggin wrote:
> > fs: check for statfs overflow
> > 
> > Adds a check for an overflow in the filesystem size so if someone is
> > checking with statfs() on a 16G hugetlbfs  in a 32bit binary that it
> > will report back EOVERFLOW instead of a size of 0.
> > 
> > Are other places that need a similar check?  I had tried a similar
> > check in put_compat_statfs64 too but it didn't seem to generate an
> > EOVERFLOW in my test case.
> > 
> > Signed-off-by: Jon Tollefson <kniht@linux.vnet.ibm.com>
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > ---
> > 
> >  fs/compat.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > Index: linux-2.6/fs/compat.c
> > ===================================================================
> > --- linux-2.6.orig/fs/compat.c
> > +++ linux-2.6/fs/compat.c
> > @@ -197,8 +197,8 @@ static int put_compat_statfs(struct comp
> >  {
> >  	
> >  	if (sizeof ubuf->f_blocks == 4) {
> > -		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail) &
> > -		    0xffffffff00000000ULL)
> > +		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
> > +		     kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
> >  			return -EOVERFLOW;
> 
> Hmm, doesn't this check break every filesystem > 16TB on 4kB PAGE_SIZE
> nodes?  It would be better, IMHO, to scale down f_blocks, f_bfree, and
> f_bavail and correspondingly scale up f_bsize to fit into the 32-bit
> statfs structure.

Being a FS newbie, I'm not entirely sure I follow, could you say that
again in patch-form? :) Seriously, it might make it clear to me.

> We did this for several years with Lustre, as the first installation
> was already larger than 16TB on 32-bit clients at the time.  There was
> never a problem with statfs returning a larger f_bsize, since
> applications generally use the fstat() st_blocksize to determine IO
> size and not the statfs() data.

I'm not sure that's a good reason to give bad data back to userspace...
We have both interfaces and both should work?

> Returning statfs data accurate to within a few kB is better than
> failing the request outright, IMHO.

Well, currently (iirc), we see statfs() give bad values for 16gb
hugetlbfs mountpoints. That's not good, and is inconsistent with the
other hugetlbfs mountpoints. We actually do want to indicate EOVERFLOW
there, as the 32-bit binary, or some kind of error, although the binary
will notice it can't use the pages from that mountpoint when mmap()
fails :)

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

Re: [patch 22/23] fs: check for statfs overflow

[Nick Piggin]

On Thu, May 29, 2008 at 05:56:07PM -0600, Andreas Dilger wrote:
> On May 28, 2008  11:02 +0200, Nick Piggin wrote:
> > fs: check for statfs overflow
> > 
> > Adds a check for an overflow in the filesystem size so if someone is
> > checking with statfs() on a 16G hugetlbfs  in a 32bit binary that it
> > will report back EOVERFLOW instead of a size of 0.
> > 
> > Are other places that need a similar check?  I had tried a similar
> > check in put_compat_statfs64 too but it didn't seem to generate an
> > EOVERFLOW in my test case.
> > 
> > Signed-off-by: Jon Tollefson <kniht@linux.vnet.ibm.com>
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > ---
> > 
> >  fs/compat.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > Index: linux-2.6/fs/compat.c
> > ===================================================================
> > --- linux-2.6.orig/fs/compat.c
> > +++ linux-2.6/fs/compat.c
> > @@ -197,8 +197,8 @@ static int put_compat_statfs(struct comp
> >  {
> >  	
> >  	if (sizeof ubuf->f_blocks == 4) {
> > -		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail) &
> > -		    0xffffffff00000000ULL)
> > +		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
> > +		     kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
> >  			return -EOVERFLOW;
> 
> Hmm, doesn't this check break every filesystem > 16TB on 4kB PAGE_SIZE
> nodes?  It would be better, IMHO, to scale down f_blocks, f_bfree, and
> f_bavail and correspondingly scale up f_bsize to fit into the 32-bit
> statfs structure.

Oh? Hmm, from my reading, such filesystems will already overflow f_blocks
check which is already there. Jon's patch only adds checks for f_bsize
and f_frsize.

One thing I'm a little worried about is the _exact_ semantics required
of the syscall wrt overflow, and  type sizes. In the man page here for
example, ubuf->f_blocks is a differnt type to f_bsize and f_frsize...


Thanks,
Nick

> We did this for several years with Lustre, as the first installation was
> already larger than 16TB on 32-bit clients at the time.  There was never
> a problem with statfs returning a larger f_bsize, since applications
> generally use the fstat() st_blocksize to determine IO size and not the
> statfs() data.
> 
> Returning statfs data accurate to within a few kB is better than failing
> the request outright, IMHO.
> 
> Cheers, Andreas

Re: [patch 22/23] fs: check for statfs overflow

[Andreas Dilger]

On May 30, 2008  03:14 +0200, Nick Piggin wrote:
> On Thu, May 29, 2008 at 05:56:07PM -0600, Andreas Dilger wrote:
> > On May 28, 2008  11:02 +0200, Nick Piggin wrote:
> > > @@ -197,8 +197,8 @@ static int put_compat_statfs(struct comp
> > >  	if (sizeof ubuf->f_blocks == 4) {
> > > +		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
> > > +		     kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
> > >  			return -EOVERFLOW;
> > 
> > Hmm, doesn't this check break every filesystem > 16TB on 4kB PAGE_SIZE
> > nodes?  It would be better, IMHO, to scale down f_blocks, f_bfree, and
> > f_bavail and correspondingly scale up f_bsize to fit into the 32-bit
> > statfs structure.
> 
> Oh? Hmm, from my reading, such filesystems will already overflow f_blocks
> check which is already there. Jon's patch only adds checks for f_bsize
> and f_frsize.

Sorry, you are right - I meant that the whole f_blocks check is broken
for filesystems > 16TB.  Scaling f_bsize is easy, and prevents gratuitous
breakage of old applications for a few kB of accuracy.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [patch 22/23] fs: check for statfs overflow

[Nick Piggin]

On Sun, Jun 01, 2008 at 09:16:02PM -0600, Andreas Dilger wrote:
> On May 30, 2008  03:14 +0200, Nick Piggin wrote:
> > On Thu, May 29, 2008 at 05:56:07PM -0600, Andreas Dilger wrote:
> > > On May 28, 2008  11:02 +0200, Nick Piggin wrote:
> > > > @@ -197,8 +197,8 @@ static int put_compat_statfs(struct comp
> > > >  	if (sizeof ubuf->f_blocks == 4) {
> > > > +		if ((kbuf->f_blocks | kbuf->f_bfree | kbuf->f_bavail |
> > > > +		     kbuf->f_bsize | kbuf->f_frsize) & 0xffffffff00000000ULL)
> > > >  			return -EOVERFLOW;
> > > 
> > > Hmm, doesn't this check break every filesystem > 16TB on 4kB PAGE_SIZE
> > > nodes?  It would be better, IMHO, to scale down f_blocks, f_bfree, and
> > > f_bavail and correspondingly scale up f_bsize to fit into the 32-bit
> > > statfs structure.
> > 
> > Oh? Hmm, from my reading, such filesystems will already overflow f_blocks
> > check which is already there. Jon's patch only adds checks for f_bsize
> > and f_frsize.
> 
> Sorry, you are right - I meant that the whole f_blocks check is broken
> for filesystems > 16TB.  Scaling f_bsize is easy, and prevents gratuitous
> breakage of old applications for a few kB of accuracy.

Oh... hmm OK but they do have stat64 I guess, although maybe they aren't
coded for it.

Anyway, point is noted, but I'm not the person (nor is this the patchset)
to make such changes.

Do you agree that if we have these checks in coimpat_statfs, then we
should put the same ones in the non-compat as well as the 64 bit
versions?

Thanks,
Nick

Re: [patch 22/23] fs: check for statfs overflow

[Andreas Dilger]

On Jun 03, 2008  05:27 +0200, Nick Piggin wrote:
> On Sun, Jun 01, 2008 at 09:16:02PM -0600, Andreas Dilger wrote:
> > On May 30, 2008  03:14 +0200, Nick Piggin wrote:
> > > Oh? Hmm, from my reading, such filesystems will already overflow f_blocks
> > > check which is already there. Jon's patch only adds checks for f_bsize
> > > and f_frsize.
> > 
> > Sorry, you are right - I meant that the whole f_blocks check is broken
> > for filesystems > 16TB.  Scaling f_bsize is easy, and prevents gratuitous
> > breakage of old applications for a few kB of accuracy.
> 
> Oh... hmm OK but they do have stat64 I guess, although maybe they aren't
> coded for it.

Right - we had this problem with all of the tools with some older distros
being compiled against the old statfs syscall and we had to put the statfs
scaling inside Lustre to avoid the 16TB overflow.

The problem with the current kernel VFS interface is that the filesystem
doesn't know whether the 32-bit or 64-bit statfs interface is being called,
and rather than returning an error to an application we'd prefer to return
scaled statfs results (with some small amount of rounding error).  Even
for 20PB filesystems (the largest planned for this year) the free/used/avail
space would only be rounded to 4MB sizes, which isn't so bad.

> Anyway, point is noted, but I'm not the person (nor is this the patchset)
> to make such changes.

Right...

> Do you agree that if we have these checks in coimpat_statfs, then we
> should put the same ones in the non-compat as well as the 64 bit
> versions?

If it only affects hugetlbfs then I'm not too concerned.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.