| 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 |
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
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
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. > >
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
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
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
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
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
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.