Re: Rare memory leakage

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

On Tue, Sep 22, 2020 at 02:12:19PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 22, 2020 at 12:44:51PM +0100, Matthew Wilcox wrote:
> > I've just kicked off a test run using this:
> > 
> > +/*
> > + * Have to be careful when freeing a non-compound allocation in case somebody
> > + * else takes a temporary reference on the first page and then calls put_page()
> > + */
> >  void __free_pages(struct page *page, unsigned int order)
> >  {
> > -       if (put_page_testzero(page))
> > -               free_the_page(page, order);
> > +       if (likely(page_ref_freeze(page, 1)))
> > +               goto free;
> > +       if (likely(order == 0 || PageHead(page))) {
> > +               if (put_page_testzero(page))
> > +                       goto free;
> > +               return;
> > +       }
> > +
> > +       prep_compound_page(page, order);
> > +       put_page(page);
> > +       return;
> > +free:
> > +       free_the_page(page, order);
> >  }
> >  EXPORT_SYMBOL(__free_pages);
> > 
> > > > Better ideas?
> > > 
> > > IMHO, alloc_pages() with order > 0 and without __GFP_COMP is a weird beast. In
> > > that case it would be probably best to refcount each base page separately. I
> > > don't know how many assumptions this would break :/
> > 
> > You sound like a man who's never dealt with device drivers.  Multiorder,
> > non-compound allocations are the norm in that world.
> 
> This seems easier to reason about:
> 
> +/*
> + * If we free a non-compound allocation, another thread may have a
> + * transient reference to the first page.  It has no way of knowing
> + * about the rest of the allocation, so we have to free all but the
> + * first page here.
> + */
>  void __free_pages(struct page *page, unsigned int order)
>  {
>         if (put_page_testzero(page))
>                 free_the_page(page, order);
> +       else
> +               while (order-- > 0)
> +                       free_the_page(page + 1 << order, order);
>  }
>  EXPORT_SYMBOL(__free_pages);

To be honest, I have not dealt with device drivers much myself, but this
looks bogus. We free pages beynd the first one if the first is pinned.
How is it correct?

-- 
 Kirill A. Shutemov