ACK/Cmnt: [SRU][J/I/H/F][PATCH 0/1] Drop "UBUNTU: SAUCE: cachefiles: Page leaking in cachefiles_read_backing_file while vmscan is active"

[Andrea Righi]

On Tue, Oct 19, 2021 at 06:41:05AM -0600, Tim Gardner wrote:
> Acked-by: Tim Gardner <tim.gardner at canonical.com>
> 
> Looks good assuming the reproducer results also look good. I assume Bionic
> will be a separate patch ?

Yes, I'll send a separate patch for bionic.

Thanks for the review!
-Andrea

> 
> On 10/19/21 4:33 AM, Andrea Righi wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1947709
> > 
> > [Impact]
> > 
> > "UBUNTU: SAUCE: cachefiles: Page leaking in cachefiles_read_backing_file while
> > vmscan is active" has been applied to fix a page leaking issue.
> > 
> > However a slightly different fix has been applied upstream:
> > 
> >   9a24ce5b66f9 ("cachefiles: Fix page leak in cachefiles_read_backing_file while vmscan is active")
> > 
> > Basically we are fixing the same issue in two different ways at the same time,
> > but even worse our patch an introduce a potential NULL pointer dereference: we
> > do a put_page(newpage) and set newpage = NULL in the main for() loop and then
> > we may do additional put_page(newpage) after the main for loop if
> > ret == -EEXIST, that would trigger the NULL pointer dereference.
> > 
> > [Test case]
> > 
> > No test case or reproducer is available at the moment, this issue has been
> > found simply by reviewing the code.
> > 
> > [Fix]
> > 
> > Drop the SAUCE patch and rely on the upstream fix.
> > 
> > [Regression potential]
> > 
> > If the analysis is not correct we may re-introduce a page leak in cachefiles
> > (NFS for example), but it seems unlikely to happen, since the upstream fix is
> > addressing the page leaking already.
> > 
> > 
> 
> -- 
> -----------
> Tim Gardner
> Canonical, Inc