Re: [PATCH] amd_iommu: Fix leak in free_pagetable()

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2013-06-20 at 21:26 +0200, Joerg Roedel wrote:
> On Thu, Jun 20, 2013 at 01:08:00PM -0600, Alex Williamson wrote:
> > On Thu, 2013-06-20 at 20:28 +0200, Joerg Roedel wrote:
> > > Hmm, actually a recursive version would make more sense here. But since
> > > recursion is a bad idea in the kernel, how about this approach instead:
> > 
> > It's a fixed maximum depth of recursion though, is it really that taboo?
> 
> Well, a recursive version would be better here because it has lower icache
> impact than my patch. But the risk is if there is a bug in recursive
> code and it goes on forever it will overwrite random kernel memory, with
> undefined, potentially very bad, results (like data corruption, killed
> filesystems and so on).
> 
> So in my opinion it is better to not take that risk whenever possible.

But that's true of a bug in any kernel code.  I think the only danger
unique to recursion is using too much stack space, but I doubt that's
really an issue for a tiny function with a fixed depth like this.  In
case you didn't notice, I did send a recursive version along with the
flat version, but I stupidly used the same subject for both.  It's a
little less code and a tiny bit easier to understand than the macro
version.  Up to you though.  Thanks,

Alex

> > > The IOMMU pagetables can have up to 3 levels, but the code
> > 
> > s/3/6/
> 
> Right, thanks.
> 
> > Seems like it should do the right thing
> 
> Ok, I also gave it some testing (started a vm, assigned and de-assigned
> a device), so it should work as expected.
> 
> > Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Thanks,
> 
> 	Joerg
> 
>