Re: [PATCH] iommu/vt-d: Fix an off-by-one bug in __domain_mapping()

[David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]

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

On Mon, 2014-12-01 at 17:27 +0100, Joerg Roedel wrote:
> On Wed, Nov 26, 2014 at 09:42:10AM +0800, Jiang Liu wrote:
> > There's an off-by-one bug in function __domain_mapping(), which may
> > trigger the BUG_ON(nr_pages < lvl_pages) when
> > 	(nr_pages + 1) & superpage_mask == 0
> 
> What is the superpage_mask?
> 
> > The issue was introduced by commit 9051aa0268dc "intel-iommu: Combine
> > domain_pfn_mapping() and domain_sg_mapping()", which sets sg_res to
> > "nr_pages + 1" to avoid some of the 'sg_res==0' code paths.
> > 
> > It's safe to remove extra "+1" because sg_res is only used to calculate
> > page size now.
> 
> From your description and the (hard to read) code in __domain_mapping I
> don't really understand the issue yet. Can you please elaborate on this
> issue can be triggered?

> Is the BUG_ON the only issue and, if yes, can that be fixed by just
> changing the BUG_ON condition?

__domain_mapping() is an amalgamation of the old domain_pfn_mapping()
and domain_sg_mapping() functions. When I did that, in commit 9051aa026,
the 'sg_res' variable was used *only* for tracking how many pages were
left in the current scatterlist element, before we had to get the next
one from the sglist.

For reasons which are lost now, in the case of a simple pfn range I was
setting 'sg_res = nr_pages + 1' to ensure that we *never* got down to
sg_res=0 and tried to look for more from the (non-existent, in this
case) sglist.

Later in commit 6dd9a7c73 we added large page support, using sg_res in a
way which actually required it to be accurate. And now we have an
off-by-one because we'll actually *try* to use a 2GiB large page for a
mapping of size 0x1ff000, because of that '+1'.

The BUG_ON is entirely correct here, and correctly highlighted the
problem.

However, the +1 is no longer necessary, because the check that needed it
was also modified to read 'if (sg_res && nr_pages)', which is perfectly
sufficient and arguably how it should have been done in the first place.

I had an almost identical patch last week for internal testing, because
I stupidly hadn't noticed that Jiang had beaten me to it.

Acked-By: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

> > 	This issue was introduced in v2.6.31, but intel-iommu.c has
> > been moved into drivers/iommu in v3.1. So what's the preferred way
> > to deal with stable kernels between v2.6.31 and v3.1?
> 
> Just remove the kernel version marker from the stable tag. The stable
> kernel maintainers for kernels >3.1 will ask you to backport the patch
> or just backport it by themselfes.

I think this is only an issue since commit 6dd9a7c737 added super page
support in 3.0, isn't it? Before that, the +1 was *needed*.

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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