------------------------------
Stewart Smith
[email protected]
Ph: +61 4 3884 4332
ICQ: 6734154
http://www.flamingspork.com/ http://www.linux.org.au
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
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...
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, [email protected] 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
On Thursday, May 29, 2003, at 09:15 PM,
[email protected] 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
[email protected]
Ph: +61 4 3884 4332
ICQ: 6734154
http://www.flamingspork.com/ http://www.linux.org.au
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
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
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