Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

[Benjamin Herrenschmidt]

Hi, I just spotted this in the patch (but the code itself have been
there since 2.4.20-pre2).

> -			if (((unsigned long) bh->b_data) < PAGE_SIZE)
> +			if ((unsigned long) bh->b_data < PAGE_SIZE)

Didn't you meant PAGE_OFFSET and not PAGE_SIZE here ? I fail to see why
it would make any sense to compare a virtual address to PAGE_SIZE ;)

Ben.

Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

[Jens Axboe]

On Wed, Jan 29 2003, Benjamin Herrenschmidt wrote:
> Hi, I just spotted this in the patch (but the code itself have been
> there since 2.4.20-pre2).
> 
> > -			if (((unsigned long) bh->b_data) < PAGE_SIZE)
> > +			if ((unsigned long) bh->b_data < PAGE_SIZE)
> 
> Didn't you meant PAGE_OFFSET and not PAGE_SIZE here ? I fail to see why
> it would make any sense to compare a virtual address to PAGE_SIZE ;)

For highmem buffer heads, b_data is the offset into the page. Does look
confusing, I'll give you that :-)

The test should most likely just be removed, if anything.

-- 
Jens Axboe

Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

[Benjamin Herrenschmidt]

On Wed, 2003-01-29 at 16:00, Jens Axboe wrote:
> On Wed, Jan 29 2003, Benjamin Herrenschmidt wrote:
> > Hi, I just spotted this in the patch (but the code itself have been
> > there since 2.4.20-pre2).
> > 
> > > -			if (((unsigned long) bh->b_data) < PAGE_SIZE)
> > > +			if ((unsigned long) bh->b_data < PAGE_SIZE)
> > 
> > Didn't you meant PAGE_OFFSET and not PAGE_SIZE here ? I fail to see why
> > it would make any sense to compare a virtual address to PAGE_SIZE ;)
> 
> For highmem buffer heads, b_data is the offset into the page. Does look
> confusing, I'll give you that :-)
> 
> The test should most likely just be removed, if anything.

I would agree with you if you were actually testing that ;) Look
closely, the test is in the non-b_page case, that is when b_data
contains a kernel virtual address.

So the test should be either removed or moved to the first part of the 
if () statement.

In our case (virtual address, not page), though, it makes some sense to
test that it's higher or equal than PAGE_OFFSET since virt_to_page()
won't work on addresses not part of the lowmem linear mapping.

Ben.

Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

[Jens Axboe]

On Wed, Jan 29 2003, Benjamin Herrenschmidt wrote:
> On Wed, 2003-01-29 at 16:00, Jens Axboe wrote:
> > On Wed, Jan 29 2003, Benjamin Herrenschmidt wrote:
> > > Hi, I just spotted this in the patch (but the code itself have been
> > > there since 2.4.20-pre2).
> > > 
> > > > -			if (((unsigned long) bh->b_data) < PAGE_SIZE)
> > > > +			if ((unsigned long) bh->b_data < PAGE_SIZE)
> > > 
> > > Didn't you meant PAGE_OFFSET and not PAGE_SIZE here ? I fail to see why
> > > it would make any sense to compare a virtual address to PAGE_SIZE ;)
> > 
> > For highmem buffer heads, b_data is the offset into the page. Does look
> > confusing, I'll give you that :-)
> > 
> > The test should most likely just be removed, if anything.
> 
> I would agree with you if you were actually testing that ;) Look
> closely, the test is in the non-b_page case, that is when b_data
> contains a kernel virtual address.
> 
> So the test should be either removed or moved to the first part of the 
> if () statement.
> 
> In our case (virtual address, not page), though, it makes some sense to
> test that it's higher or equal than PAGE_OFFSET since virt_to_page()
> won't work on addresses not part of the lowmem linear mapping.

Ehm no, if b_data is < PAGE_SIZE, it's probably an offset and not a
valid address. So it should be exactly where it is -- for b_page, it's
_not_ buggy for b_data to be < PAGE_SIZE. That's expected. Submitter
would have to be buggy for it to trigger, though, so you can just remove
it if you want.

-- 
Jens Axboe

Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

[Benjamin Herrenschmidt]

On Wed, 2003-01-29 at 16:22, Jens Axboe wrote:

> Ehm no, if b_data is < PAGE_SIZE, it's probably an offset and not a
> valid address. So it should be exactly where it is -- for b_page, it's
> _not_ buggy for b_data to be < PAGE_SIZE. That's expected. Submitter
> would have to be buggy for it to trigger, though, so you can just remove
> it if you want.

Ah, I see what you wanted to check now :) Ok, I won't remove it, though
it would still make sense to extend the test to PAGE_OFFSET I beleive,
any b_data < PAGE_OFFSET is wrong.

Anyway, let's leave 2.4 as it is now.

-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

[Jens Axboe]

On Wed, Jan 29 2003, Benjamin Herrenschmidt wrote:
> On Wed, 2003-01-29 at 16:22, Jens Axboe wrote:
> 
> > Ehm no, if b_data is < PAGE_SIZE, it's probably an offset and not a
> > valid address. So it should be exactly where it is -- for b_page, it's
> > _not_ buggy for b_data to be < PAGE_SIZE. That's expected. Submitter
> > would have to be buggy for it to trigger, though, so you can just remove
> > it if you want.
> 
> Ah, I see what you wanted to check now :) Ok, I won't remove it, though
> it would still make sense to extend the test to PAGE_OFFSET I beleive,
> any b_data < PAGE_OFFSET is wrong.

No, any b_data < PAGE_OFFSET is not wrong, that's the point. For highmem
b_page, b_data will be the offset into the page. So it could be 2048,
for instance.

The test is meant to catch an invalid buffer_head, where b_page is not
set but b_data isn't valid either. So to make it complete, you could do:

	if (bh->b_page) {
		...
		if (bh->b_data >= PAGE_SIZE)
			BUG();
	} else {
		...
		if (bh->b_data < PAGE_SIZE)
			BUG();
		if (bh->b_data < PAGE_OFFSET)
			BUG();
	}

as they are two different bugs.

> Anyway, let's leave 2.4 as it is now.

:-)

-- 
Jens Axboe

Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

[Benjamin Herrenschmidt]

On Wed, 2003-01-29 at 16:38, Jens Axboe wrote:
> No, any b_data < PAGE_OFFSET is not wrong, that's the point. For highmem
> b_page, b_data will be the offset into the page. So it could be 2048,
> for instance.

In the other if() case, yes ;)

> The test is meant to catch an invalid buffer_head, where b_page is not
> set but b_data isn't valid either. So to make it complete, you could do:

Yup, I undestood that.

> 		if (bh->b_data < PAGE_SIZE)
> 			BUG();
> 		if (bh->b_data < PAGE_OFFSET)
> 			BUG();
> 	}

All I wanted to spot is that < PAGE_OFFSET would catch the PAGE_SIZE bug
as well ;) But that's not a problem in real life anyway it seems.

-- 
Benjamin Herrenschmidt <benh@kernel.crashing.org>

Re: [PATCH] IDE: Do not call bh_phys() on buffers with invalid b_page.

[Jens Axboe]

On Wed, Jan 29 2003, Benjamin Herrenschmidt wrote:
> On Wed, 2003-01-29 at 16:38, Jens Axboe wrote:
> > No, any b_data < PAGE_OFFSET is not wrong, that's the point. For highmem
> > b_page, b_data will be the offset into the page. So it could be 2048,
> > for instance.
> 
> In the other if() case, yes ;)

b_data < PAGE_SIZE would not be a bug in the other case, if
PageHighmem(bh->b_page). If !PageHighmem(bh->b_page), then b_data <
PAGE_OFFSET would be a bug, yes.

But lets drop this now, it's getting way silly! :)

> All I wanted to spot is that < PAGE_OFFSET would catch the PAGE_SIZE bug
> as well ;) But that's not a problem in real life anyway it seems.

That is correct, but as I said it's nice to separate them because it's
two bugs really. And the one I wanted to catch was number 1, and that's
the one I put in. EOD?

-- 
Jens Axboe