Re: [PATCH] hfsplus: release bnode pages after use, not before

[Anton Altaparmakov <anton@tuxera.com>]

Hi Andrew,

Forgot to reply to one point you made:

> On 10 Jun 2015, at 01:15, Andrew Morton <akpm@linux-foundation.org> wrote:
> Yes, I too would like to hear much more about your thinking on this,
> and a detailed description of the bug and how the patch fixes it.

Perhaps the patch description is lacking but here is what the current code does:

struct page *page = read_mapping_page();
page_cache_release(page);
u8 *kaddr = kmap(page);
memcpy(..., kaddr, ...);
kunmap(page);

Now in what world is that a valid thing to do?  When the page_cache_release() happens the page is no longer allocated and the kmap() is referencing not-in-use memory and so is the memcpy() and so is the kunmap().

The only reason the code gets away with it is that the kmap/memcpy/kunmap follow very quickly after the page_cache_release() so the kernel has not had a chance to reuse the memory for something else.

Sergei said that he got a problem when he was running memory intensive processes at same time so the kernel was thrashing/evicting/resuing page cache pages at high speed and then obviously the kmap() actually mapped a different page to the original that was page_cache_release()d and thus the memcpy() effectively copied random data which was then considered corrupt by the verification code and thus the entire B tree node was considered corrupt and in Sergei's case the volume thus failed to mount.

And his patch changes the above to this instead:

struct page *page = read_mapping_page();
u8 *kaddr = kmap(page);
memcpy(..., kaddr, ...);
kunmap(page);
page_cache_release(page);

Which is the correct sequence of events.

Although perhaps there should also be a mark_page_accessed(page); thrown in there, too, before the page_cache_release() in the expectation that the B tree node is likely to be used again?

Best regards,

	Anton
-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer