mbox series

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

Message ID 20211019103307.27519-1-andrea.righi@canonical.com
Headers show
Series Drop "UBUNTU: SAUCE: cachefiles: Page leaking in cachefiles_read_backing_file while vmscan is active" | expand

Message

Andrea Righi Oct. 19, 2021, 10:33 a.m. UTC
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.

Comments

Thadeu Lima de Souza Cascardo Oct. 19, 2021, 12:02 p.m. UTC | #1
On Tue, Oct 19, 2021 at 12:33:07PM +0200, 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.
> 

So, I see bionic has the SAUCE patch, but does not have the upstream one. We
should revert the SAUCE patch on bionic as well and apply the upstream patch
there.

Also, the upstream commit has a test case, are you able to use it?

Thanks.
Cascardo.

> [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.
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Andrea Righi Oct. 19, 2021, 12:11 p.m. UTC | #2
On Tue, Oct 19, 2021 at 09:02:44AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Oct 19, 2021 at 12:33:07PM +0200, 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.
> > 
> 
> So, I see bionic has the SAUCE patch, but does not have the upstream one. We
> should revert the SAUCE patch on bionic as well and apply the upstream patch
> there.

Agreed. I can send another SRU email to include bionic as well.

> 
> Also, the upstream commit has a test case, are you able to use it?
> 
> Thanks.
> Cascardo.

I haven't tried the test case yet, because it requires to setup an NFS
server, I'll do some tests later today to see if I can trigger any bug.

Thanks,
-Andrea
Tim Gardner Oct. 19, 2021, 12:41 p.m. UTC | #3
Acked-by: Tim Gardner <tim.gardner@canonical.com>

Looks good assuming the reproducer results also look good. I assume 
Bionic will be a separate patch ?

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.
> 
>
Andrea Righi Oct. 19, 2021, 1:14 p.m. UTC | #4
On Tue, Oct 19, 2021 at 06:41:05AM -0600, Tim Gardner wrote:
> Acked-by: Tim Gardner <tim.gardner@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
Andrea Righi Oct. 19, 2021, 3:05 p.m. UTC | #5
On Tue, Oct 19, 2021 at 09:02:44AM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Oct 19, 2021 at 12:33:07PM +0200, 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.
> > 
> 
> So, I see bionic has the SAUCE patch, but does not have the upstream one. We
> should revert the SAUCE patch on bionic as well and apply the upstream patch
> there.
> 
> Also, the upstream commit has a test case, are you able to use it?

Alright, I've been running the upstream commit test case for a while
now, but I wasn't able to trigger any bug, so it's either a bad test
case to trigger the bug that I see or my analysis about the potential
NULL pointer dereference is incorrect. In any case I think it'd be still
better to drop the SAUCE patch, because it's claiming to fix something
that is already fixed by another upstream commit. Opinions?

Thanks,
-Andrea
Kamal Mostafa Oct. 19, 2021, 3:55 p.m. UTC | #6
LGTM.

Acked-by: Kamal Mostafa <kamal@canonical.com>

 -Kamal

On Tue, Oct 19, 2021 at 12:33:07PM +0200, 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.
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Thadeu Lima de Souza Cascardo Oct. 19, 2021, 4:32 p.m. UTC | #7
On Tue, Oct 19, 2021 at 05:05:39PM +0200, Andrea Righi wrote:
> On Tue, Oct 19, 2021 at 09:02:44AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > On Tue, Oct 19, 2021 at 12:33:07PM +0200, 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.
> > > 
> > 
> > So, I see bionic has the SAUCE patch, but does not have the upstream one. We
> > should revert the SAUCE patch on bionic as well and apply the upstream patch
> > there.
> > 
> > Also, the upstream commit has a test case, are you able to use it?
> 
> Alright, I've been running the upstream commit test case for a while
> now, but I wasn't able to trigger any bug, so it's either a bad test
> case to trigger the bug that I see or my analysis about the potential
> NULL pointer dereference is incorrect. In any case I think it'd be still
> better to drop the SAUCE patch, because it's claiming to fix something
> that is already fixed by another upstream commit. Opinions?
> 

Yeah, though it would be great to be able to reproduce the potential failure,
the test also helps to exercise the code, which avoids surprising regressions.

Thanks, Andrea.
Cascardo.

Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>

> Thanks,
> -Andrea
Kleber Sacilotto de Souza Nov. 4, 2021, 3:58 p.m. UTC | #8
On 19.10.21 12:33, 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.
> 
> 

Applied to [focal/hirsute/impish]:linux.

Thanks,
Kleber