Messages in this thread | | | Date | Mon, 7 Jun 2010 15:33:21 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] kmmio/mmiotrace: fix double free of kmmio_fault_pages |
| |
* Marcin Slusarz <marcin.slusarz@gmail.com> wrote:
> On Sat, Jun 05, 2010 at 06:49:42PM +0200, Marcin Slusarz wrote: > > After every iounmap mmiotrace has to free kmmio_fault_pages, but it > > can't do it directly, so it defers freeing by RCU. > > > > It usually works, but when mmiotraced code calls ioremap-iounmap > > multiple times without sleeping between (so RCU won't kick in and > > start freeing) it can be given the same virtual address, so at > > every iounmap mmiotrace will schedule the same pages for release. > > Obviously it will explode on second free. > > > > Fix it by marking kmmio_fault_pages which are scheduled for release > > and not adding them second time. > > > > Attached patch for mmiotrace testing module allows to reliably reproduce > the bug. It can be folded into the main patch. > > --- > diff --git a/arch/x86/mm/testmmiotrace.c b/arch/x86/mm/testmmiotrace.c > index 8565d94..5f0937b 100644 > --- a/arch/x86/mm/testmmiotrace.c > +++ b/arch/x86/mm/testmmiotrace.c > @@ -90,6 +90,19 @@ static void do_test(unsigned long size) > iounmap(p); > } > > +static void do_test2(void)
Please add a comment about what the test function achieves.
> +{ > + void __iomem *p; > + int i; > + > + for (i = 0; i < 10; ++i) { > + p = ioremap_nocache(mmio_address, 4096);
s/4096/PAGE_SIZE
> + if (p) > + iounmap(p); > + } > + synchronize_rcu(); /* will freeing work? */ > +} > + > static int __init init(void) > { > unsigned long size = (read_far) ? (8 << 20) : (16 << 10); > @@ -104,6 +117,7 @@ static int __init init(void) > "and writing 16 kB of rubbish in there.\n", > size >> 10, mmio_address); > do_test(size); > + do_test2();
Please name the new function in a bit more meaningful way (such as do_test_remap()).
Looks good, please send the full (folded back) patch anew, with Pekka's Ack in place as well.
Thanks,
Ingo
| |