lkml.org 
[lkml]   [2010]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2010-06-07 15:35    [W:0.023 / U:30.044 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site