buffer_head.b_bsize type

buffer_head.b_bsize type

[Stewart Smith]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

The buffer_head structure (include/linux/buffer_head.h) uses a u32 type 
while everywhere else (e.g. bread) the size parameter is of type int.

Currently on all architectures u32 is defined as unsigned int. We 
should probably not be doing unsigned and signed swaps. And you should 
never really have a negative size of a buffer.

So, there are two solutions: either change the buffer_head struct to be 
int so it matches everywhere else, or change everywhere else.

The attached patch does the change in one place. Although perhaps 
changing everywhere else would be better. Thoughts? I'm happy to make 
up the patch if needed.


Applies cleanly to 2.5.69 and 2.5.70 and has been tested on i386 
without causing any further problems (that I can see at least).

diff -ur linux-2.5.69-orig/include/linux/buffer_head.h 
linux-2.5.69/include/linux/buffer_head.h
--- linux-2.5.69-orig/include/linux/buffer_head.h	Mon May  5 09:52:49 
2003
+++ linux-2.5.69/include/linux/buffer_head.h	Fri May 16 12:05:16 2003
@@ -51,7 +51,7 @@
  	struct page *b_page;		/* the page this bh is mapped to */

  	sector_t b_blocknr;		/* block number */
-	u32 b_size;			/* block size */
+	int b_size;			/* block size */
  	char *b_data;			/* pointer to data block */

  	struct block_device *b_bdev;



[-- Attachment #2: patch-buffer_head-u32toint.diff --]
[-- Type: application/octet-stream, Size: 509 bytes --]

diff -ur linux-2.5.69-orig/include/linux/buffer_head.h linux-2.5.69/include/linux/buffer_head.h
--- linux-2.5.69-orig/include/linux/buffer_head.h	Mon May  5 09:52:49 2003
+++ linux-2.5.69/include/linux/buffer_head.h	Fri May 16 12:05:16 2003
@@ -51,7 +51,7 @@
 	struct page *b_page;		/* the page this bh is mapped to */
 
 	sector_t b_blocknr;		/* block number */
-	u32 b_size;			/* block size */
+	int b_size;			/* block size */
 	char *b_data;			/* pointer to data block */
 
 	struct block_device *b_bdev;


[-- Attachment #3: Type: text/plain, Size: 158 bytes --]


------------------------------
Stewart Smith
stewartsmith@mac.com
Ph: +61 4 3884 4332
ICQ: 6734154
http://www.flamingspork.com/       http://www.linux.org.au

Re: buffer_head.b_bsize type

[William Lee Irwin III]

On Thu, May 29, 2003 at 08:29:40PM +1000, Stewart Smith wrote:
> The buffer_head structure (include/linux/buffer_head.h) uses a u32 type 
> while everywhere else (e.g. bread) the size parameter is of type int.
> Currently on all architectures u32 is defined as unsigned int. We 
> should probably not be doing unsigned and signed swaps. And you should 
> never really have a negative size of a buffer.
> So, there are two solutions: either change the buffer_head struct to be 
> int so it matches everywhere else, or change everywhere else.
> The attached patch does the change in one place. Although perhaps 
> changing everywhere else would be better. Thoughts? I'm happy to make 
> up the patch if needed.
> Applies cleanly to 2.5.69 and 2.5.70 and has been tested on i386 
> without causing any further problems (that I can see at least).

Could we go the other way and make all users of b_size use unsigned?


Thanks.


-- wli

Re: buffer_head.b_bsize type

[viro]

On Thu, May 29, 2003 at 03:35:03AM -0700, William Lee Irwin III wrote:
> On Thu, May 29, 2003 at 08:29:40PM +1000, Stewart Smith wrote:
> > The buffer_head structure (include/linux/buffer_head.h) uses a u32 type 
> > while everywhere else (e.g. bread) the size parameter is of type int.
> > Currently on all architectures u32 is defined as unsigned int. We 
> > should probably not be doing unsigned and signed swaps. And you should 
> > never really have a negative size of a buffer.
> > So, there are two solutions: either change the buffer_head struct to be 
> > int so it matches everywhere else, or change everywhere else.
> > The attached patch does the change in one place. Although perhaps 
> > changing everywhere else would be better. Thoughts? I'm happy to make 
> > up the patch if needed.
> > Applies cleanly to 2.5.69 and 2.5.70 and has been tested on i386 
> > without causing any further problems (that I can see at least).
> 
> Could we go the other way and make all users of b_size use unsigned?

Who the hell cares?  Size of buffer does not exceed the page size.
Unless you can show a platform with 2Gb pages...

Re: buffer_head.b_bsize type

[William Lee Irwin III]

On Thu, May 29, 2003 at 03:35:03AM -0700, William Lee Irwin III wrote:
>> Could we go the other way and make all users of b_size use unsigned?

On Thu, May 29, 2003 at 12:15:17PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> Who the hell cares?  Size of buffer does not exceed the page size.
> Unless you can show a platform with 2Gb pages...

The thought behind my comment was that it didn't make sense to allow
the representation to go negative. There of course shouldn't ever be
any need to allow >= 2GB b_size to be representable.

I'll defer to viro here.


-- wli

Re: buffer_head.b_bsize type

[Stewart Smith]

On Thursday, May 29, 2003, at 09:15  PM, 
viro@parcelfarce.linux.theplanet.co.uk wrote:
>> Could we go the other way and make all users of b_size use unsigned?
>
> Who the hell cares?  Size of buffer does not exceed the page size.
> Unless you can show a platform with 2Gb pages...

I would argue the same that the size of a buffer should never be able 
to be negative and the structure says this but the rest of the code 
does not. (Theoretically) if a negative size was asked for, then we'd 
actually try to allocate a large amount of memory for the buffer, 
possibly causing badness somewhere along the line.

Any maybe we shouldn't make it too hard for archs with 2GB pages :) 
It'll probably happen in 20 years. :)

------------------------------
Stewart Smith
stewartsmith@mac.com
Ph: +61 4 3884 4332
ICQ: 6734154
http://www.flamingspork.com/       http://www.linux.org.au

Re: buffer_head.b_bsize type

[Andries Brouwer]

On Thu, May 29, 2003 at 04:28:41AM -0700, William Lee Irwin III wrote:

> The thought behind my comment was that it didn't make sense to allow
> the representation to go negative. There of course shouldn't ever be
> any need to allow >= 2GB b_size to be representable.

Not about this particular case, but as a general remark:
Use of unsigned is dangerous - use of int is far preferable,
everywhere that is possible.

With ints the test a+b > c is equivalent to the test a > c-b.
Intuition works.

As soon as there is some unsigned in an expression comparisons
get counterintuitive because -1 is very large.
Thus, 1+sizeof(int) > 3 is true, but 1 > 3-sizeof(int) is false.

It has happened several times that kernel code was broken because
some variable (that always was nonnegative) was made unsigned.

Andries

Re: buffer_head.b_bsize type

[Andries Brouwer]

On Fri, May 30, 2003 at 04:24:34PM +0200, Andries Brouwer wrote:
> On Thu, May 29, 2003 at 04:28:41AM -0700, William Lee Irwin III wrote:
> 
> > The thought behind my comment was that it didn't make sense to allow
> > the representation to go negative. There of course shouldn't ever be
> > any need to allow >= 2GB b_size to be representable.
> 
> Not about this particular case, but as a general remark:
> Use of unsigned is dangerous - use of int is far preferable,
> everywhere that is possible.
> 
> With ints the test a+b > c is equivalent to the test a > c-b.

[of course I meant: With smallish ints, so that no overflow occurs]

> As soon as there is some unsigned in an expression comparisons
> get counterintuitive because -1 is very large.
> Thus, 1+sizeof(int) > 3 is true, but 1 > 3-sizeof(int) is false.
> 
> It has happened several times that kernel code was broken because
> some variable (that always was nonnegative) was made unsigned.
> 
> Andries

Re: buffer_head.b_bsize type

[William Lee Irwin III]

On Fri, May 30, 2003 at 04:24:34PM +0200, Andries Brouwer wrote:
> Not about this particular case, but as a general remark:
> Use of unsigned is dangerous - use of int is far preferable,
> everywhere that is possible.
> With ints the test a+b > c is equivalent to the test a > c-b.
> Intuition works.
> As soon as there is some unsigned in an expression comparisons
> get counterintuitive because -1 is very large.
> Thus, 1+sizeof(int) > 3 is true, but 1 > 3-sizeof(int) is false.
> It has happened several times that kernel code was broken because
> some variable (that always was nonnegative) was made unsigned.

I don't see what the big deal is. Arithmetic in Z/2**32Z or whatever
doesn't really define a comparison, we just artificially impose our
favorite residues and have to check various preconditions for the
results of comparisons to make sense (which obviously aren't checked
in your example of garbage produced by a comparison).

You are right in that some attempt at an audit should be done if it
were to be changed, of course. I generally think of it as easy, and
assume it will be done.

-- wli