Re: [GIT PULL] for-2.6.32/bug-fixes

Re: [GIT PULL] for-2.6.32/bug-fixes

[Jan Beulich]

[-- Attachment #1.1: Type: text/plain, Size: 969 bytes --]

>>> Konrad Rzeszutek Wilk  05/17/11 6:44 PM >>>
>On Tue, May 17, 2011 at 05:24:43PM +0100, Jan Beulich wrote:
>> >>> On 17.05.11 at 17:57, Konrad Rzeszutek Wilk  wrote:
>> >> > No attaching of data to the barrier.
>> >> 
>> >> Sure, this direction we agree about. But your change is enforcing
>> >> it the other way around (if barrier then no data), which wasn't the
>> >> case so far.
>> > 
>> > OK, even if the code that actually does the bio submission does
>> > not attach any data to the bio? The end result is the same - no
>> > data with barriers.
>> 
>> My problem is that I can't see where attaching data would be
>> skipped. The only thing I see is the BUG_ON() you pointed at
>
>Well, req->ns_segments = 0, so nseg is zero, which means all
>of those for loops never get executed.

This you say is the case for the request you saw the failure with, or
*all* barrier requests? In the latter case, what do you conclude this
from?

Jan


[-- Attachment #1.2: HTML --]
[-- Type: text/html, Size: 1468 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [GIT PULL] for-2.6.32/bug-fixes

[Konrad Rzeszutek Wilk]

> >Well, req->ns_segments = 0, so nseg is zero, which means all
> >of those for loops never get executed.
> 
> This you say is the case for the request you saw the failure with, or
> *all* barrier requests? In the latter case, what do you conclude this

Good question. It was the first barrier request sent when guest tried to
mount the filesystem. I will instrument the code to see what the other
barriers contained when they were sent.
> from?

Re: [GIT PULL] for-2.6.32/bug-fixes

[Jan Beulich]

>>> On 18.05.11 at 15:24, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> >Well, req->ns_segments = 0, so nseg is zero, which means all
>> >of those for loops never get executed.
>> 
>> This you say is the case for the request you saw the failure with, or
>> *all* barrier requests? In the latter case, what do you conclude this
> 
> Good question. It was the first barrier request sent when guest tried to
> mount the filesystem. I will instrument the code to see what the other
> barriers contained when they were sent.

That wouldn't tell you anything if they're all empty, as there's nothing
preventing other guests (including other guest OSes) to still send
non-empty ones - after all the protocol allows for this.

Jan

>> from?

Re: [GIT PULL] for-2.6.32/bug-fixes

[Konrad Rzeszutek Wilk]

On Wed, May 18, 2011 at 03:31:35PM +0100, Jan Beulich wrote:
> >>> On 18.05.11 at 15:24, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >> >Well, req->ns_segments = 0, so nseg is zero, which means all
> >> >of those for loops never get executed.
> >> 
> >> This you say is the case for the request you saw the failure with, or
> >> *all* barrier requests? In the latter case, what do you conclude this
> > 
> > Good question. It was the first barrier request sent when guest tried to
> > mount the filesystem. I will instrument the code to see what the other
> > barriers contained when they were sent.
> 
> That wouldn't tell you anything if they're all empty, as there's nothing
> preventing other guests (including other guest OSes) to still send
> non-empty ones - after all the protocol allows for this.

Aha! That is what you been trying to tell me. I will make a patch to make
sure to not overwrite the req->sector_number blindly. What other guest OSes
use barriers? I looked at Solaris (it uses 'feature-flush-cache'), NetBSD
('feature-flush-cache') and Linux ('feature-barrier' and now in 2.6.40
'feature-flush-cache').

The GPLV Windows drivers have no barrier implementation - do you know if
the Novell ones are using barriers?

Re: [GIT PULL] for-2.6.32/bug-fixes

[Jan Beulich]

>>> On 18.05.11 at 16:56, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Wed, May 18, 2011 at 03:31:35PM +0100, Jan Beulich wrote:
>> >>> On 18.05.11 at 15:24, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> >> >Well, req->ns_segments = 0, so nseg is zero, which means all
>> >> >of those for loops never get executed.
>> >> 
>> >> This you say is the case for the request you saw the failure with, or
>> >> *all* barrier requests? In the latter case, what do you conclude this
>> > 
>> > Good question. It was the first barrier request sent when guest tried to
>> > mount the filesystem. I will instrument the code to see what the other
>> > barriers contained when they were sent.
>> 
>> That wouldn't tell you anything if they're all empty, as there's nothing
>> preventing other guests (including other guest OSes) to still send
>> non-empty ones - after all the protocol allows for this.
> 
> Aha! That is what you been trying to tell me. I will make a patch to make
> sure to not overwrite the req->sector_number blindly. What other guest OSes

Or use the patch I proposed?

> use barriers? I looked at Solaris (it uses 'feature-flush-cache'), NetBSD
> ('feature-flush-cache') and Linux ('feature-barrier' and now in 2.6.40
> 'feature-flush-cache').
> 
> The GPLV Windows drivers have no barrier implementation - do you know if
> the Novell ones are using barriers?

No, I don't.

Jan

Re: [GIT PULL] for-2.6.32/bug-fixes

[Konrad Rzeszutek Wilk]

> >> That wouldn't tell you anything if they're all empty, as there's nothing
> >> preventing other guests (including other guest OSes) to still send
> >> non-empty ones - after all the protocol allows for this.
> > 
> > Aha! That is what you been trying to tell me. I will make a patch to make
> > sure to not overwrite the req->sector_number blindly. What other guest OSes
> 
> Or use the patch I proposed?

Yes. Much superior to mine.
> 
> > use barriers? I looked at Solaris (it uses 'feature-flush-cache'), NetBSD
> > ('feature-flush-cache') and Linux ('feature-barrier' and now in 2.6.40
> > 'feature-flush-cache').
> > 
> > The GPLV Windows drivers have no barrier implementation - do you know if
> > the Novell ones are using barriers?
> 
> No, I don't.

Any idea who might know?

Re: [GIT PULL] for-2.6.32/bug-fixes

[Jan Beulich]

>>> On 18.05.11 at 17:13, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> >> That wouldn't tell you anything if they're all empty, as there's nothing
>> >> preventing other guests (including other guest OSes) to still send
>> >> non-empty ones - after all the protocol allows for this.
>> > 
>> > Aha! That is what you been trying to tell me. I will make a patch to make
>> > sure to not overwrite the req->sector_number blindly. What other guest OSes
>> 
>> Or use the patch I proposed?
> 
> Yes. Much superior to mine.
>> 
>> > use barriers? I looked at Solaris (it uses 'feature-flush-cache'), NetBSD
>> > ('feature-flush-cache') and Linux ('feature-barrier' and now in 2.6.40
>> > 'feature-flush-cache').
>> > 
>> > The GPLV Windows drivers have no barrier implementation - do you know if
>> > the Novell ones are using barriers?
>> 
>> No, I don't.
> 
> Any idea who might know?

Kirk should.

Jan

Re: [GIT PULL] for-2.6.32/bug-fixes

[Konrad Rzeszutek Wilk]

On Tue, May 17, 2011 at 05:24:43PM +0100, Jan Beulich wrote:
> >>> On 17.05.11 at 17:57, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >> > No attaching of data to the barrier.
> >> 
> >> Sure, this direction we agree about. But your change is enforcing
> >> it the other way around (if barrier then no data), which wasn't the
> >> case so far.
> > 
> > OK, even if the code that actually does the bio submission does
> > not attach any data to the bio? The end result is the same - no
> > data with barriers.
> 
> My problem is that I can't see where attaching data would be
> skipped. The only thing I see is the BUG_ON() you pointed at

Well, req->ns_segments = 0, so nseg is zero, which means all
of those for loops never get executed.

> earlier, checking that if there is no data, then this must be a
> barrier request.
> 
> >> >> Additionally, looking at the check in vbd_translate(), wouldn't you
> >> >> think there ought to be overflow checking for the addition, too?
> >> > 
> >> > Sure, could add that in. Albeit it seems incorrect to do it in that
> >> > function. It checks to see if the sector is correct, and -1 is definitly
> >> > wrong.
> >> 
> >> Hmm, depends on your perspective - I'd say that any sector_number
> >> is valid when nr_sects is zero.
> > 
> > I concur. The value that is passed by the frontend is not zero. It is -1.
> 
> Oh, you say both sector_number and nr_sects are -1? Looking
> again... No, that can't be the case, the value starts out at zero
> in dispatch_rw_block_io().

No, wait. Argh.

The req->nr_sects is 0 and req->sector_number is -1.

This is what I got before the fix:

access denied: write of [18446744073709551615,18446744073709551615] on dev=ca0

And req->nr_segments is 0.

Re: [GIT PULL] for-2.6.32/bug-fixes

[Jan Beulich]

>>> On 17.05.11 at 17:57, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > No attaching of data to the barrier.
>> 
>> Sure, this direction we agree about. But your change is enforcing
>> it the other way around (if barrier then no data), which wasn't the
>> case so far.
> 
> OK, even if the code that actually does the bio submission does
> not attach any data to the bio? The end result is the same - no
> data with barriers.

My problem is that I can't see where attaching data would be
skipped. The only thing I see is the BUG_ON() you pointed at
earlier, checking that if there is no data, then this must be a
barrier request.

>> >> Additionally, looking at the check in vbd_translate(), wouldn't you
>> >> think there ought to be overflow checking for the addition, too?
>> > 
>> > Sure, could add that in. Albeit it seems incorrect to do it in that
>> > function. It checks to see if the sector is correct, and -1 is definitly
>> > wrong.
>> 
>> Hmm, depends on your perspective - I'd say that any sector_number
>> is valid when nr_sects is zero.
> 
> I concur. The value that is passed by the frontend is not zero. It is -1.

Oh, you say both sector_number and nr_sects are -1? Looking
again... No, that can't be the case, the value starts out at zero
in dispatch_rw_block_io().

Jan

Re: [GIT PULL] for-2.6.32/bug-fixes

[Konrad Rzeszutek Wilk]

> > No attaching of data to the barrier.
> 
> Sure, this direction we agree about. But your change is enforcing
> it the other way around (if barrier then no data), which wasn't the
> case so far.

OK, even if the code that actually does the bio submission does
not attach any data to the bio? The end result is the same - no
data with barriers.

> 
> >> Hence shouldn't you clear the sector number only when
> >> req->nr_segments is zero? Or alternatively, shouldn't
> > 
> > We could do that too.
> > 
> >> vbd_translate() simply not fail when req->nr_sects is zero?
> > 
> > It does not fail when req->nr_sects is zero. It fails when it is -1.
> >
> >> 
> >> Additionally, looking at the check in vbd_translate(), wouldn't you
> >> think there ought to be overflow checking for the addition, too?
> > 
> > Sure, could add that in. Albeit it seems incorrect to do it in that
> > function. It checks to see if the sector is correct, and -1 is definitly
> > wrong.
> 
> Hmm, depends on your perspective - I'd say that any sector_number
> is valid when nr_sects is zero.

I concur. The value that is passed by the frontend is not zero. It is -1.

Re: [GIT PULL] for-2.6.32/bug-fixes

[Jan Beulich]

>>> On 17.05.11 at 16:16, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> On Tue, May 17, 2011 at 10:48:00AM +0100, Jan Beulich wrote:
>> >>> On 16.05.11 at 22:35, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > with xen-blkback wherein a barrier request would have been discarded (and an 
> error
>> > returned) b/c the sector provided via the request was -1. The -1 sector made
>> > vbd_translate return an error (it checked the sector number against the 
> size of
>> > the disk) and it would never go through trying to do a barrier. The second 
> bug-fix
>> > is also in my devel/xen-blkback-v3.2 upstream tree.
>> 
>> Is this really correct? You appear to assume that BLKIF_OP_WRITE_BARRIER
>> always has no data, but the rest of the code in the driver (and
>> the frontend) doesn't seem to imply that (see e.g. the check
>> immediately following the switch statement your patch modifies).
> 
> That is correct. Look at the end of the code logic (this is from the 2.6.18
> hg tree):
> 
> 528         if (!bio) {
> 529                 BUG_ON(operation != WRITE_BARRIER);
> 530                 bio = bio_alloc(GFP_KERNEL, 0);
> 531                 if (unlikely(bio == NULL))
> 532                         goto fail_put_bio;
> 533 
> 534                 bio->bi_bdev    = preq.bdev;
> 535                 bio->bi_private = pending_req;
> 536                 bio->bi_end_io  = end_block_io_op;
> 537                 bio->bi_sector  = -1;
> 538         }
> 
> No attaching of data to the barrier.

Sure, this direction we agree about. But your change is enforcing
it the other way around (if barrier then no data), which wasn't the
case so far.

>> Hence shouldn't you clear the sector number only when
>> req->nr_segments is zero? Or alternatively, shouldn't
> 
> We could do that too.
> 
>> vbd_translate() simply not fail when req->nr_sects is zero?
> 
> It does not fail when req->nr_sects is zero. It fails when it is -1.
>
>> 
>> Additionally, looking at the check in vbd_translate(), wouldn't you
>> think there ought to be overflow checking for the addition, too?
> 
> Sure, could add that in. Albeit it seems incorrect to do it in that
> function. It checks to see if the sector is correct, and -1 is definitly
> wrong.

Hmm, depends on your perspective - I'd say that any sector_number
is valid when nr_sects is zero.

Jan

Re: [GIT PULL] for-2.6.32/bug-fixes

[Konrad Rzeszutek Wilk]

On Tue, May 17, 2011 at 10:48:00AM +0100, Jan Beulich wrote:
> >>> On 16.05.11 at 22:35, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > with xen-blkback wherein a barrier request would have been discarded (and an error
> > returned) b/c the sector provided via the request was -1. The -1 sector made
> > vbd_translate return an error (it checked the sector number against the size of
> > the disk) and it would never go through trying to do a barrier. The second bug-fix
> > is also in my devel/xen-blkback-v3.2 upstream tree.
> 
> Is this really correct? You appear to assume that BLKIF_OP_WRITE_BARRIER
> always has no data, but the rest of the code in the driver (and
> the frontend) doesn't seem to imply that (see e.g. the check
> immediately following the switch statement your patch modifies).

That is correct. Look at the end of the code logic (this is from the 2.6.18
hg tree):

528         if (!bio) {
529                 BUG_ON(operation != WRITE_BARRIER);
530                 bio = bio_alloc(GFP_KERNEL, 0);
531                 if (unlikely(bio == NULL))
532                         goto fail_put_bio;
533 
534                 bio->bi_bdev    = preq.bdev;
535                 bio->bi_private = pending_req;
536                 bio->bi_end_io  = end_block_io_op;
537                 bio->bi_sector  = -1;
538         }

No attaching of data to the barrier.

> Hence shouldn't you clear the sector number only when
> req->nr_segments is zero? Or alternatively, shouldn't

We could do that too.

> vbd_translate() simply not fail when req->nr_sects is zero?

It does not fail when req->nr_sects is zero. It fails when it is -1.

> 
> Additionally, looking at the check in vbd_translate(), wouldn't you
> think there ought to be overflow checking for the addition, too?

Sure, could add that in. Albeit it seems incorrect to do it in that
function. It checks to see if the sector is correct, and -1 is definitly
wrong.

> 
> Jan

Re: [GIT PULL] for-2.6.32/bug-fixes

[Jan Beulich]

>>> On 17.05.11 at 11:48, "Jan Beulich" <JBeulich@novell.com> wrote:
>>>> On 16.05.11 at 22:35, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> with xen-blkback wherein a barrier request would have been discarded (and an 
> error
>> returned) b/c the sector provided via the request was -1. The -1 sector made
>> vbd_translate return an error (it checked the sector number against the size 
> of
>> the disk) and it would never go through trying to do a barrier. The second 
> bug-fix
>> is also in my devel/xen-blkback-v3.2 upstream tree.
> 
> Is this really correct? You appear to assume that BLKIF_OP_WRITE_BARRIER
> always has no data, but the rest of the code in the driver (and
> the frontend) doesn't seem to imply that (see e.g. the check
> immediately following the switch statement your patch modifies).
> Hence shouldn't you clear the sector number only when
> req->nr_segments is zero? Or alternatively, shouldn't
> vbd_translate() simply not fail when req->nr_sects is zero?
> 
> Additionally, looking at the check in vbd_translate(), wouldn't you
> think there ought to be overflow checking for the addition, too?

Altogether e.g.

Subject: xen/blkback: don't fail empty barrier requests

The sector number on empty barrier requests may (will?) be -1, which,
given that it's being treated as unsigned 64-bit quantity, will almost
always exceed the actual (virtual) disk's size.

Inspired by Konrad's "When writting barriers set the sector number to
zero...".

While at it also add overflow checking to the math in vbd_translate().

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

--- a/drivers/xen/blkback/vbd.c
+++ b/drivers/xen/blkback/vbd.c
@@ -114,8 +114,14 @@ int vbd_translate(struct phys_req *req, 
 	if (vbd->bdev == NULL)
 		goto out;
 
-	if (unlikely((req->sector_number + req->nr_sects) > vbd_sz(vbd)))
-		goto out;
+	if (likely(req->nr_sects)) {
+		blkif_sector_t end = req->sector_number + req->nr_sects;
+
+		if (unlikely(end < req->sector_number))
+			goto out;
+		if (unlikely(end > vbd_sz(vbd)))
+			goto out;
+	}
 
 	req->dev  = vbd->pdevice;
 	req->bdev = vbd->bdev;

Re: [GIT PULL] for-2.6.32/bug-fixes

[Jan Beulich]

>>> On 16.05.11 at 22:35, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> with xen-blkback wherein a barrier request would have been discarded (and an error
> returned) b/c the sector provided via the request was -1. The -1 sector made
> vbd_translate return an error (it checked the sector number against the size of
> the disk) and it would never go through trying to do a barrier. The second bug-fix
> is also in my devel/xen-blkback-v3.2 upstream tree.

Is this really correct? You appear to assume that BLKIF_OP_WRITE_BARRIER
always has no data, but the rest of the code in the driver (and
the frontend) doesn't seem to imply that (see e.g. the check
immediately following the switch statement your patch modifies).
Hence shouldn't you clear the sector number only when
req->nr_segments is zero? Or alternatively, shouldn't
vbd_translate() simply not fail when req->nr_sects is zero?

Additionally, looking at the check in vbd_translate(), wouldn't you
think there ought to be overflow checking for the addition, too?

Jan

[GIT PULL] for-2.6.32/bug-fixes

[Konrad Rzeszutek Wilk]

Hey Jeremy,


Please pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git for-2.6.32/bug-fixes

It has two bug-fixes in it. The first one is quite specific and it only handles
the case if you run v2.6.32 on an IBM Summit machine. The other fixes an issue
with xen-blkback wherein a barrier request would have been discarded (and an error
returned) b/c the sector provided via the request was -1. The -1 sector made
vbd_translate return an error (it checked the sector number against the size of
the disk) and it would never go through trying to do a barrier. The second bug-fix
is also in my devel/xen-blkback-v3.2 upstream tree.

Konrad Rzeszutek Wilk (2):
      xen/apic: Provide an 'apic_xen' to set the override the apic->[read|write] for all cases.
      xen/blkback: When writting barriers set the sector number to zero...

 arch/x86/kernel/apic/probe_32.c |    4 ++++
 arch/x86/kernel/apic/probe_64.c |    4 ++++
 arch/x86/xen/enlighten.c        |   26 ++++++++++++++++++++++++++
 drivers/xen/blkback/blkback.c   |    2 ++
 4 files changed, 36 insertions(+)

[GIT PULL] for-2.6.32/bug-fixes

[Konrad Rzeszutek Wilk]

Jeremy,

Please pull in your stable/2.6.32.x the following git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git for-2.6.32/bug-fixes

which has patches since git commit 75cc13f5aa29b4f3227d269ca165dfa8937c94fe
Jeremy Fitzhardinge (1):

    Merge commit 'v2.6.32.27' into xen/next-2.6.32


It has the bootup fixes found in 2.6.37/2.6.38 tree back-ported to 2.6.32.
I've done the normal boot-up tests on Intel and AMD hardware and while
they are not introducing any regressions - the 2.6.32.x suffers from some
other types of bugs (if I use dom0_mem it ends up OOM-ing).

Stefan Bader (1):
      xen/p2m: Mark INVALID_P2M_ENTRY the mfn_list past max_pfn.

Stefano Stabellini (1):
      xen/e820: Guard against E820_RAM not having page-aligned size or start.

 arch/x86/xen/mmu.c   |   12 ++++++++++++
 arch/x86/xen/setup.c |    7 ++++++-
 2 files changed, 18 insertions(+), 1 deletions(-)