Re: [PATCH] ipr: don't doublefree pages from scatterlist

[Hugh Dickins <hugh@veritas.com>]

On Fri, 3 Feb 2006, Brian King wrote:
> Hugh Dickins wrote:
> > On some architectures, mapping the scatterlist may coalesce entries:
> > if that coalesced list is then used for freeing the pages afterwards,
> > there's a danger that pages may be doubly freed (and others leaked).
> 
> I don't understand why this is necessary... Comparing the bug you fixed
> in st with ipr's usage of scatterlists, there is a bit of a difference.
> st is dealing with user pages and calling page_cache_release to release
> the pages, and I can begin to understand why there might be a problem
> in that code.

Yes, certainly the st and ipr cases seem different, and originally
I was thinking it was only an issue in connection with get_user_pages.

> page_cache_release looks at the pages themselves to figure
> out how many compound pages there are. This could certainly result in
> double free's occurring.

I don't follow you there, but better if I try to explain how I see it.

> Looking at ipr, however, it is simply doing alloc_pages
> and __free_pages. __free_pages passes in the page allocation order, so
> I don't think I would have the double free problem.
> 
> As I understand it, pci_map_sg only modifies the dma_address and dma_length
> fields when things get coalesced. If it were to coalese pages by
> turning them into compound pages then I would agree that ipr might have
> a problem, but I don't think this to be the case...

It's not fully defined what it does (intentionally: internal detail).
But as I understand it, what it's likely to do is coalesce entries as
far as it can (causes no problem in itself) then shift down and attempt
to coalesce the entries above.  It's the shifting down that gives the
problem.

Imagine we start with sglist

struct page *a, length A
struct page *b, length B
struct page *c, length C

and pci_map_sg finds a+A brings it to b (I'm writing loosely, mixing
the page pointers and their corresponding virtual addresses), but b+B
does not match c.  Then it'll coalesce the first two entries, and
shift down the third to second place, turning sglist into

struct page *a, length A+B
struct page *c, length C
struct page *c, length C

So (again writing loosely, mixing up lengths and orders) on return the
caller will __free_pages(a, A+B) (itself probably wrong since a and b
were likely not buddies to start out with), __free_pages(c, C),
__free_pages(c, C) - doubly freeing c, ensuing mayhem.

Maybe it doesn't change length A to A+B, just doing that in dma_length;
but caller is still going to free c twice.

Agree?

Hugh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/