linux-kernel - Re: [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <500FBF37.50603@redhat.com>
Date:	Wed, 25 Jul 2012 11:41:11 +0200
From:	Paolo Bonzini <pbonzini@...hat.com>
To:	Boaz Harrosh <bharrosh@...asas.com>
CC:	Wang Sen <senwang@...ux.vnet.ibm.com>, linux-scsi@...r.kernel.org,
	JBottomley@...allels.com, stefanha@...ux.vnet.ibm.com,
	mc@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: virtio-scsi: Fix address translation failure of
 HighMem pages used by sg list

Il 25/07/2012 11:22, Boaz Harrosh ha scritto:
>>> >>  	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@...r.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.

Hi Boaz,

actually it seems to me that using sg_set_page is wrong, because it will
not copy the end marker from table->sgl to sg[].  If something chained
the sg[] scatterlist onto something else, sg_next's test for sg_is_last
would go beyond the table->nents-th item and access invalid memory.

Using chained sglists is on my to-do list, I expect that it would make a
nice performance improvement.  However, I was a bit confused as to
what's the plan there; there is hardly any user, and many arches still
do not define ARCH_HAS_SG_CHAIN.  Do you have any pointer to discussions
or LWN articles?

I would need to add support for the long sglists to virtio; this is not
a problem, but in the past Rusty complained that long sg-lists are not
well suited to virtio (which would like to add elements not just at the
beginning of a given sglist, but also at the end).  It seems to me that
virtio would prefer to work with a struct scatterlist ** rather than a
long sglist.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/