From: Boaz Harrosh <bharrosh@panasas.com> To: Paolo Bonzini <pbonzini@redhat.com> Cc: Wang Sen <senwang@linux.vnet.ibm.com>, <linux-scsi@vger.kernel.org>, <JBottomley@parallels.com>, <stefanha@linux.vnet.ibm.com>, <mc@linux.vnet.ibm.com>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list Date: Wed, 25 Jul 2012 12:22:48 +0300 [thread overview] Message-ID: <500FBAE8.2050107@panasas.com> (raw) In-Reply-To: <500FB1DE.1000100@redhat.com> On 07/25/2012 11:44 AM, Paolo Bonzini wrote: > Il 25/07/2012 10:29, Wang Sen ha scritto: >> When using the commands below to write some data to a virtio-scsi LUN of the >> QEMU guest(32-bit) with 1G physical memory(qemu -m 1024), the qemu will crash. >> >> # sudo mkfs.ext4 /dev/sdb (/dev/sdb is the virtio-scsi LUN.) >> # sudo mount /dev/sdb /mnt >> # dd if=/dev/zero of=/mnt/file bs=1M count=1024 >> >> In current implementation, sg_set_buf is called to add buffers to sg list which >> is put into the virtqueue eventually. But there are some HighMem pages in >> table->sgl can not get virtual address by sg_virt. So, sg_virt(sg_elem) may >> return NULL value. This will cause QEMU exit when virtqueue_map_sg is called >> in QEMU because an invalid GPA is passed by virtqueue. > > Heh, I was compiling (almost) the same patch as we speak. :) > > I've never seen QEMU crash; the VM would more likely just fail to boot > with a panic. But it's the same bug anyway. > >> My solution is using sg_set_page instead of sg_set_buf. >> >> I have tested the patch on my workstation. QEMU would not crash any more. >> >> Signed-off-by: Wang Sen <senwang@linux.vnet.ibm.com> >> --- >> drivers/scsi/virtio_scsi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c >> index 1b38431..fc5c88a 100644 >> --- a/drivers/scsi/virtio_scsi.c >> +++ b/drivers/scsi/virtio_scsi.c >> @@ -198,7 +198,8 @@ static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx, >> int i; >> >> for_each_sg(table->sgl, sg_elem, table->nents, i) >> - sg_set_buf(&sg[idx++], sg_virt(sg_elem), sg_elem->length); >> + sg_set_page(&sg[idx++], sg_page(sg_elem), sg_elem->length, >> + sg_elem->offset); > > This can simply be > > sg[idx++] = *sg_elem; > > Can you repost it with this change, and also add stable@vger.kernel.org > to the Cc? Thanks very much! > No! Please use sg_set_page()! Look at sg_set_page(), which calls sg_assign_page(). It has all these jump over chained arrays. When you'll start using long sg_lists (which you should) then jumping from chain to chain must go through sg_page(sg_elem) && sg_assign_page(), As in the original patch. Thanks Boaz > Paolo > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Boaz Harrosh <bharrosh@panasas.com> To: Paolo Bonzini <pbonzini@redhat.com> Cc: Wang Sen <senwang@linux.vnet.ibm.com>, linux-scsi@vger.kernel.org, JBottomley@parallels.com, stefanha@linux.vnet.ibm.com, mc@linux.vnet.ibm.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list Date: Wed, 25 Jul 2012 12:22:48 +0300 [thread overview] Message-ID: <500FBAE8.2050107@panasas.com> (raw) In-Reply-To: <500FB1DE.1000100@redhat.com> On 07/25/2012 11:44 AM, Paolo Bonzini wrote: > Il 25/07/2012 10:29, Wang Sen ha scritto: >> When using the commands below to write some data to a virtio-scsi LUN of the >> QEMU guest(32-bit) with 1G physical memory(qemu -m 1024), the qemu will crash. >> >> # sudo mkfs.ext4 /dev/sdb (/dev/sdb is the virtio-scsi LUN.) >> # sudo mount /dev/sdb /mnt >> # dd if=/dev/zero of=/mnt/file bs=1M count=1024 >> >> In current implementation, sg_set_buf is called to add buffers to sg list which >> is put into the virtqueue eventually. But there are some HighMem pages in >> table->sgl can not get virtual address by sg_virt. So, sg_virt(sg_elem) may >> return NULL value. This will cause QEMU exit when virtqueue_map_sg is called >> in QEMU because an invalid GPA is passed by virtqueue. > > Heh, I was compiling (almost) the same patch as we speak. :) > > I've never seen QEMU crash; the VM would more likely just fail to boot > with a panic. But it's the same bug anyway. > >> My solution is using sg_set_page instead of sg_set_buf. >> >> I have tested the patch on my workstation. QEMU would not crash any more. >> >> Signed-off-by: Wang Sen <senwang@linux.vnet.ibm.com> >> --- >> drivers/scsi/virtio_scsi.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c >> index 1b38431..fc5c88a 100644 >> --- a/drivers/scsi/virtio_scsi.c >> +++ b/drivers/scsi/virtio_scsi.c >> @@ -198,7 +198,8 @@ static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx, >> int i; >> >> for_each_sg(table->sgl, sg_elem, table->nents, i) >> - sg_set_buf(&sg[idx++], sg_virt(sg_elem), sg_elem->length); >> + sg_set_page(&sg[idx++], sg_page(sg_elem), sg_elem->length, >> + sg_elem->offset); > > This can simply be > > sg[idx++] = *sg_elem; > > Can you repost it with this change, and also add stable@vger.kernel.org > to the Cc? Thanks very much! > No! Please use sg_set_page()! Look at sg_set_page(), which calls sg_assign_page(). It has all these jump over chained arrays. When you'll start using long sg_lists (which you should) then jumping from chain to chain must go through sg_page(sg_elem) && sg_assign_page(), As in the original patch. Thanks Boaz > Paolo > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2012-07-25 9:23 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2012-07-25 8:29 [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list Wang Sen 2012-07-25 8:44 ` Paolo Bonzini 2012-07-25 9:22 ` Boaz Harrosh [this message] 2012-07-25 9:22 ` Boaz Harrosh 2012-07-25 9:41 ` Paolo Bonzini 2012-07-25 12:34 ` Boaz Harrosh 2012-07-25 12:34 ` Boaz Harrosh 2012-07-25 12:49 ` Paolo Bonzini 2012-07-25 13:26 ` Boaz Harrosh 2012-07-25 13:26 ` Boaz Harrosh 2012-07-25 13:36 ` Paolo Bonzini 2012-07-25 14:36 ` Boaz Harrosh 2012-07-25 14:36 ` Boaz Harrosh 2012-07-25 15:09 ` performance improvements for the sglist API (Re: [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list) Paolo Bonzini 2012-07-25 15:16 ` Paolo Bonzini 2012-07-25 14:17 ` virtio(-scsi) vs. chained sg_lists (was " Paolo Bonzini 2012-07-25 15:28 ` Boaz Harrosh 2012-07-25 15:28 ` Boaz Harrosh 2012-07-25 17:43 ` Paolo Bonzini 2012-07-25 19:16 ` Boaz Harrosh 2012-07-25 19:16 ` Boaz Harrosh 2012-07-25 20:06 ` Paolo Bonzini 2012-07-25 21:04 ` Boaz Harrosh 2012-07-25 21:04 ` Boaz Harrosh 2012-07-26 7:23 ` Paolo Bonzini 2012-07-26 7:56 ` Boaz Harrosh 2012-07-26 7:56 ` Boaz Harrosh 2012-07-26 7:58 ` Paolo Bonzini 2012-07-26 13:05 ` Paolo Bonzini 2012-07-27 6:27 ` Rusty Russell 2012-07-27 6:27 ` Rusty Russell 2012-07-27 8:11 ` Paolo Bonzini 2012-07-29 23:50 ` Rusty Russell 2012-07-29 23:50 ` Rusty Russell 2012-07-30 7:12 ` Paolo Bonzini 2012-07-30 8:56 ` Boaz Harrosh 2012-07-30 8:56 ` Boaz Harrosh 2012-07-25 10:41 ` [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list Stefan Hajnoczi 2012-07-25 11:48 ` Sen Wang 2012-07-25 11:44 ` Sen Wang 2012-07-25 12:40 ` Boaz Harrosh 2012-07-25 12:40 ` Boaz Harrosh 2012-07-27 3:12 ` Wang Sen 2012-07-27 6:50 ` Paolo Bonzini 2012-07-25 10:04 ` Rolf Eike Beer 2012-07-25 10:04 ` Rolf Eike Beer 2012-07-25 11:46 ` Sen Wang [not found] <1343203219-19190-1-git-send-email-senwang@linux.vnet.ibm.com> 2012-07-25 10:05 ` Stefan Hajnoczi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=500FBAE8.2050107@panasas.com \ --to=bharrosh@panasas.com \ --cc=JBottomley@parallels.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-scsi@vger.kernel.org \ --cc=mc@linux.vnet.ibm.com \ --cc=pbonzini@redhat.com \ --cc=senwang@linux.vnet.ibm.com \ --cc=stefanha@linux.vnet.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.