linux-kernel - RE: [Xen-devel] [PATCH] xen/swiotlb: Exchange to contiguous memory for map

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: <40776A41FC278F40B59438AD47D147A90FECC3EC@SHSMSX102.ccr.corp.intel.com>
Date:	Thu, 20 Dec 2012 01:23:17 +0000
From:	"Xu, Dongxiao" <dongxiao.xu@...el.com>
To:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Jan Beulich <JBeulich@...e.com>
CC:	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [Xen-devel] [PATCH] xen/swiotlb: Exchange to contiguous memory
 for map_sg hook

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@...cle.com]
> Sent: Thursday, December 20, 2012 4:09 AM
> To: Jan Beulich
> Cc: Xu, Dongxiao; xen-devel@...ts.xen.org; linux-kernel@...r.kernel.org
> Subject: Re: [Xen-devel] [PATCH] xen/swiotlb: Exchange to contiguous memory
> for map_sg hook
> 
> On Wed, Dec 12, 2012 at 09:38:23AM +0000, Jan Beulich wrote:
> > >>> On 12.12.12 at 02:03, "Xu, Dongxiao" <dongxiao.xu@...el.com> wrote:
> > >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@...cle.com] On Tue,
> > >> Dec 11, 2012 at 06:39:35AM +0000, Xu, Dongxiao wrote:
> > >> > > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@...cle.com]
> > >> > > What if this check was done in the routines that provide the
> > >> > > software static buffers and there try to provide a nice DMA
> > >> > > contingous
> > >> swatch of pages?
> > >> >
> > >> > Yes, this approach also came to our mind, which needs to modify
> > >> > the driver
> > >> itself.
> > >> > If so, it requires driver not using such static buffers (e.g.,
> > >> > from
> > > kmalloc) to do
> > >> DMA even if the buffer is continuous in native.
> > >>
> > >> I am bit loss here.
> > >>
> > >> Is the issue you found only with drivers that do not use DMA API?
> > >> Can you perhaps point me to the code that triggered this fix in the
> > >> first
> > > place?
> > >
> > > Yes, we met this issue on a specific SAS device/driver, and it calls
> > > into libata-core code, you can refer to function ata_dev_read_id()
> > > called from
> > > ata_dev_reread_id() in drivers/ata/libata-core.c.
> > >
> > > In the above function, the target buffer is (void
> > > *)dev->link->ap->sector_buf, which is 512 bytes static buffer and
> > > unfortunately it across the page boundary.
> >
> > I wonder whether such use of sg_init_one()/sg_set_buf() is correct in
> > the first place. While there aren't any restrictions documented for
> > its use, one clearly can't pass in whatever one wants (a pointer into
> > vmalloc()-ed memory, for instance, won't work afaict).
> >
> > I didn't go through all other users of it, but quite a few of the uses
> > elsewhere look similarly questionable.
> >
> > >> I am still not completely clear on what you had in mind. The one
> > >> method I thought about that might help in this is to have
> > >> Xen-SWIOTLB track which memory ranges were exchanged (so
> > >> xen_swiotlb_fixup would save the *buf and the size for each call to
> > >> xen_create_contiguous_region in a list or
> > > array).
> > >>
> > >> When xen_swiotlb_map/xen_swiotlb_map_sg_attrs care called they
> > >> would consult said array/list to see if the region they retrieved
> > >> crosses said 2MB chunks. If so.. and here I am unsure of what would
> > >> be the best way to
> > > proceed.
> 
> And from finally looking at the code I misunderstood your initial description.
> 
> > >
> > > We thought we can solve the issue in several ways:
> > >
> > > 1) Like the previous patch I sent out, we check the DMA region in
> > > xen_swiotlb_map_page() and xen_swiotlb_map_sg_attr(), and if DMA
> > > region crosses page boundary, we exchange the memory and copy the
> > > content. However it has race condition that when copying the memory
> > > content (we introduced two memory copies in the patch), some other
> > > code may also visit the page, which may encounter incorrect values.
> >
> > That's why, after mapping a buffer (or SG list) one has to call the
> > sync functions before looking at data. Any race as described by you is
> > therefore a programming error.
> 
> I am with Jan here. It looks like the libata is depending on the dma_unmap to
> do this. But it is unclear to me when the ata_qc_complete is actually called to
> unmap the buffer (and hence sync it).

Sorry, maybe I am still not describing this issue clearly.

Take the libata case as an example, the static DMA buffer locates (dev->link->ap->sector_buf , here we use Data Structure B in the graph) in the following structure:

-------------------------------------Page boundary
<Data Structure A>
<Data Structure B>
-------------------------------------Page boundary
<Data Structure B (cross page)>
<Data Structure C>
-------------------------------------Page boundary

Where Structure B is our DMA target.

For Data Structure B, we didn't care about the simultaneous access, either lock or sync function will take care of it.
What we are not sure is "read/write of A and C from other processor". As we will have memory copy for the pages, and at the same time, other CPU may access A/C.

Thanks,
Dongxiao
> 
> >
> > Jan
> >
> >
--
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/