* Re: [patch 22/23] fs: check for statfs overflow [not found] ` <483C42B9.7090102@linux.vnet.ibm.com> @ 2008-05-28 9:02 ` Nick Piggin 2008-05-29 23:56 ` Andreas Dilger 0 siblings, 1 reply; 7+ messages in thread From: Nick Piggin @ 2008-05-28 9:02 UTC (permalink / raw) To: Jon Tollefson Cc: Nishanth Aravamudan, linux-mm, andi, agl, abh, joachim.deguara, linux-fsdevel 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 */ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 22/23] fs: check for statfs overflow 2008-05-28 9:02 ` [patch 22/23] fs: check for statfs overflow Nick Piggin @ 2008-05-29 23:56 ` Andreas Dilger 2008-05-30 0:12 ` Nishanth Aravamudan 2008-05-30 1:14 ` Nick Piggin 0 siblings, 2 replies; 7+ messages in thread From: Andreas Dilger @ 2008-05-29 23:56 UTC (permalink / raw) To: Nick Piggin Cc: Jon Tollefson, Nishanth Aravamudan, linux-mm, andi, agl, abh, joachim.deguara, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 22/23] fs: check for statfs overflow 2008-05-29 23:56 ` Andreas Dilger @ 2008-05-30 0:12 ` Nishanth Aravamudan 2008-05-30 1:14 ` Nick Piggin 1 sibling, 0 replies; 7+ messages in thread From: Nishanth Aravamudan @ 2008-05-30 0:12 UTC (permalink / raw) To: Andreas Dilger Cc: Nick Piggin, Jon Tollefson, linux-mm, andi, agl, abh, joachim.deguara, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 22/23] fs: check for statfs overflow 2008-05-29 23:56 ` Andreas Dilger 2008-05-30 0:12 ` Nishanth Aravamudan @ 2008-05-30 1:14 ` Nick Piggin 2008-06-02 3:16 ` Andreas Dilger 1 sibling, 1 reply; 7+ messages in thread From: Nick Piggin @ 2008-05-30 1:14 UTC (permalink / raw) To: Andreas Dilger Cc: Jon Tollefson, Nishanth Aravamudan, linux-mm, andi, agl, abh, joachim.deguara, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 22/23] fs: check for statfs overflow 2008-05-30 1:14 ` Nick Piggin @ 2008-06-02 3:16 ` Andreas Dilger 2008-06-03 3:27 ` Nick Piggin 0 siblings, 1 reply; 7+ messages in thread From: Andreas Dilger @ 2008-06-02 3:16 UTC (permalink / raw) To: Nick Piggin Cc: Jon Tollefson, Nishanth Aravamudan, linux-mm, andi, agl, abh, joachim.deguara, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 22/23] fs: check for statfs overflow 2008-06-02 3:16 ` Andreas Dilger @ 2008-06-03 3:27 ` Nick Piggin 2008-06-03 17:17 ` Andreas Dilger 0 siblings, 1 reply; 7+ messages in thread From: Nick Piggin @ 2008-06-03 3:27 UTC (permalink / raw) To: Andreas Dilger Cc: Jon Tollefson, Nishanth Aravamudan, linux-mm, andi, agl, abh, joachim.deguara, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch 22/23] fs: check for statfs overflow 2008-06-03 3:27 ` Nick Piggin @ 2008-06-03 17:17 ` Andreas Dilger 0 siblings, 0 replies; 7+ messages in thread From: Andreas Dilger @ 2008-06-03 17:17 UTC (permalink / raw) To: Nick Piggin Cc: Jon Tollefson, Nishanth Aravamudan, linux-mm, andi, agl, abh, joachim.deguara, linux-fsdevel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-06-03 17:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20080525142317.965503000@nick.local0.net>
[not found] ` <20080525143454.453947000@nick.local0.net>
[not found] ` <20080527171452.GJ20709@us.ibm.com>
[not found] ` <483C42B9.7090102@linux.vnet.ibm.com>
2008-05-28 9:02 ` [patch 22/23] fs: check for statfs overflow Nick Piggin
2008-05-29 23:56 ` Andreas Dilger
2008-05-30 0:12 ` Nishanth Aravamudan
2008-05-30 1:14 ` Nick Piggin
2008-06-02 3:16 ` Andreas Dilger
2008-06-03 3:27 ` Nick Piggin
2008-06-03 17:17 ` Andreas Dilger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).