[SRU B][C][PATCH 1/1 v2] UBUNTU: SAUCE: cachefiles: Page leaking in cachefiles_read_backing_file while vmscan is active

[Seth Forshee]

On Fri, Sep 21, 2018 at 12:59:40PM +1000, Daniel Axtens wrote:
> From: Daniel Axtens <dja at axtens.net>
> 
> FYI, Kiran's response hasn't hit the list for some reason, so I've kept
> it in full below. Perhaps it's stuck in moderation.
> 
> 
> I found I was getting confused between the
> cachefiles_read_backing_file_one() function, which was the first hunk of
> the original patch, and cachefiles_read_backing_file(), which has the
> other 3 hunks of the original patch.
> 
> (For clarity, I'm referring to this bit:
> 
> --- a/fs/cachefiles/rdwr.c
> +++ b/fs/cachefiles/rdwr.c
> @@ -274,6 +274,8 @@ static int cachefiles_read_backing_file_one(struct cachefiles_object *object,
>                         goto installed_new_backing_page;
>                 if (ret != -EEXIST)
>                         goto nomem_page;
> +               put_page(newpage);
> +               newpage = NULL;
>         }
> 
> )
> 
> In cachefiles_read_backing_file(), there is an equivalent section to
> this already - this just brings the two functions in to alignment.
> 
> Seth's question was whether or not that would make a difference. The
> case we are concerned about is what happens if you reach the end of the
> loop, the find a backing page and goto backing_page_already_present.
> 
> In cachefiles_read_backing_file_one(), backing_page_already_present
> starts around line 317, and one of the first things it does is check if
> newpage is not NULL and puts it if so. So we don't have a leak.
> 
> On the other hand, the parallel backing_page_already_present in
> cachefiles_read_backing_file() (around line 577) does *not* free newpage
> if it exists. However, we also don't have a leak here as the loop in
> cachefiles_read_backing_file() *does* put_page(newpage) unconditionally
> at the end of the loop. Confusing!
> 
> While it would be desirable to bring these functions into alignment, we
> can do that upstream rather than in a stable release. So I think we can
> safely drop the changes to cachefiles_read_backing_file_one() and just
> focus on cachefiles_read_backing_file().
> 
> If we're just focussing on that function, we can safely leave all 3 of
> the original hunks from patch v1. I agree that some of the sets of NULL
> are not strictly necessary and some are missing where you'd expect
> them. Ideally I'd like to keep the delta between what we have here and
> what we're hoping will go upstream to a minimum, so how does the
> following look as both a patch for Ubuntu and a patch for upstream? 

Ultimately I would prefer that we apply whatever fix ends up upstream,
within reason. There's no patch upstream yet however, so I was giving
some general feedback. If the expectation is that upstream prefers those
two loops to remain the same, then that's the version we want to
consider for Ubuntu.

Seth