linux-kernel - Re: [PATCH 18 of 38] x86: unify pci iommu setup and allow swiotlb to compile for 32 bit

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: <1227806087.11549.44.camel@zakaz.uk.xensource.com>
Date:	Thu, 27 Nov 2008 17:14:47 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
Cc:	jeremy@...p.org, mingo@...e.hu, linux-kernel@...r.kernel.org,
	xen-devel@...ts.xensource.com, x86@...nel.org
Subject: Re: [PATCH 18 of 38] x86: unify pci iommu setup and allow swiotlb
	to	compile for 32 bit

On Thu, 2008-11-27 at 12:43 +0900, FUJITA Tomonori wrote: 
> On Wed, 26 Nov 2008 09:36:49 +0000
> Ian Campbell <Ian.Campbell@...rix.com> wrote:
> 
> > On Wed, 2008-11-26 at 11:53 +0900, FUJITA Tomonori wrote:
> > > 
> > > > +     BUG_ON(max_slots > 1UL << (BITS_PER_LONG - IO_TLB_SHIFT));
> > > 
> > > How can this BUG_ON happen? Using u64 for the mask is fine though.
> > 
> > It covers the cases where the previous code would have overflowed. It
> > can't happen right now because although mask is 64 bits the value
> > assigned to it is currently sizeof(unsigned long). If someone changes
> > the type of that field then we would start seeing unexpected values.
> 
> If someone changes dma_get_seg_boundary to return a u64 value instead
> of unsigned long, this BUG_ON could happen on 32bit architectures. But
> you don't need to trigger BUG_ON for it. max_slots > 1UL <<
> (BITS_PER_LONG - IO_TLB_SHIFT) should be fine for
> iommu_is_span_boundary().
> 
> Anyway, this is minor but would it be nice to make sure that anyone
> can easily understand the code without digging into the git log?
> 
> a) dropping this patch and adding some comments how the code works
> (especially about the overflow on 32bit architectures).
> 
> b) removing the BUG_ON in this patch and adding some comments.

Yes, I think adding a comment to the existing code (option a) would be
best. I actually have a small queue of other fixes which make swiotlb
work properly for x86 PAE and HighMem but they are not particularly well
baked at the moment. I'll include a patch to add a comment in that
series.

Ian.

--
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/