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

From: Andrew Morton <akpm@linux-foundation.org>
To: Sergei Antonov <saproj@gmail.com>
Cc: Viacheslav Dubeyko <slava@dubeyko.com>,
	Anton Altaparmakov <aia21@cam.ac.uk>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	Sasha Levin <sasha.levin@oracle.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Christoph Hellwig <hch@infradead.org>,
	Hin-Tak Leung <htl10@users.sourceforge.net>,
	Sougata Santra <sougata@tuxera.com>
Subject: Re: [PATCH] hfsplus: release bnode pages after use, not before
Date: Tue, 9 Jun 2015 15:15:45 -0700	[thread overview]
Message-ID: <20150609151545.8a146bb5d29051e604d4d211@linux-foundation.org> (raw)
In-Reply-To: <CABikg9wGLwmF8SkYdQs2Fw99gD14kSeiF6uWMYqQ_HRYqwNntg@mail.gmail.com>

On Mon, 8 Jun 2015 18:50:00 +0200 Sergei Antonov <saproj@gmail.com> wrote:

> >> You are basically saying you don___t understand it. Too bad, because the
> >> bug is very simple. It is the ___use after free___ type of bug, and it can
> >> be illustrated by this:
> >> (1) void *ptr = malloc(___);
> >> (2) free(ptr);
> >> (3) memcpy(___, ptr, 1);
> >> Guess which two of these three lines are executed in wrong order.
> >>
> >> My patch is about the same type of bug, but with memory pages mapping.
> >> The driver currently accesses pages that may be unavailable, or
> >> contain different data. The problem is more likely to occur when
> >> memory is a limited resource. I reproduced it while running a
> >> memory-hungry program.
> >
> > I worried not about myself but about potential readers of description of
> > the fix. The description is completely obscure. And it needs to describe
> > the fix in clear and descriptive manner. This is my request. Please,
> > describe the fix in a clear way.
> 
> The description is just right.

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.

The code is distressingly undocumented and has been that way since
Roman Zippel's original commit in 2004.

>From the looks of it, that loop in __hfs_bnode_create() is simply doing
readahead and is designed as a performance optimisation.  The pages are
pulled into pagecache in the expectation that they will soon be
accessed.  What your patch does is to instead pin the pages in
pagecache until the bnode is freed.  If we're going to do this then we
need to be very careful about worst-case scenarios: we could even run
the machine out of memory.

If I'm correct, and this is just readahead then the bug lies elsewhere:
if other parts of hfsplus are assuming that this memory is in pagecache
then that's an error - that code (where is it?) should instead be performing
a pagecache lookup and if the page isn't present, read it from disk
again.

But for others to be able to review and understand this change and
suggest alternatives, we'll need a much much better changelog!