This reverts commit 25511676362d8f7d4b8805730a3d29484ceab1ec in the 4.19 stable trees. From what I can tell this commit doesn't do anything to improve the situation, mostly just reordering code to call free_initrd() from one place instead of many. In doing that, it causes free_initrd() to be called even in the case when there isn't an initrd present. That leads to virtual memory bugs that manifest on arm64 devices.
The fix has been merged upstream in commit 5d59aa8f9ce9 ("initramfs: don't free a non-existent initrd"), but backporting that here is more complicated because the patch is stacked upon this patch being reverted along with more patches that rewrites the logic in this area.
Let's just revert the patch from the stable tree instead of trying to backport a collection of fixes to get the final fix from upstream.
Cc: Sasha Levin sashal@kernel.org Cc: Steven Price steven.price@arm.com Cc: Mark Rutland mark.rutland@arm.com Cc: Christoph Hellwig hch@lst.de Signed-off-by: Stephen Boyd swboyd@chromium.org ---
This patch is for 4.19 stable series.
init/initramfs.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/init/initramfs.c b/init/initramfs.c index cd5fb00fcb54..f6f4a1e4cd54 100644 --- a/init/initramfs.c +++ b/init/initramfs.c @@ -612,12 +612,13 @@ static int __init populate_rootfs(void) printk(KERN_INFO "Trying to unpack rootfs image as initramfs...\n"); err = unpack_to_rootfs((char *)initrd_start, initrd_end - initrd_start); - if (!err) + if (!err) { + free_initrd(); goto done; - - clean_rootfs(); - unpack_to_rootfs(__initramfs_start, __initramfs_size); - + } else { + clean_rootfs(); + unpack_to_rootfs(__initramfs_start, __initramfs_size); + } printk(KERN_INFO "rootfs image is not initramfs (%s)" "; looks like an initrd\n", err); fd = ksys_open("/initrd.image", @@ -631,6 +632,7 @@ static int __init populate_rootfs(void) written, initrd_end - initrd_start);
ksys_close(fd); + free_initrd(); } done: /* empty statement */; @@ -640,9 +642,9 @@ static int __init populate_rootfs(void) initrd_end - initrd_start); if (err) printk(KERN_EMERG "Initramfs unpacking failed: %s\n", err); + free_initrd(); #endif } - free_initrd(); flush_delayed_fput(); /* * Try loading default modules from initramfs. This gives
On Tue, Aug 06, 2019 at 10:59:40AM -0700, Stephen Boyd wrote:
This reverts commit 25511676362d8f7d4b8805730a3d29484ceab1ec in the 4.19 stable trees. From what I can tell this commit doesn't do anything to improve the situation, mostly just reordering code to call free_initrd() from one place instead of many. In doing that, it causes free_initrd() to be called even in the case when there isn't an initrd present. That leads to virtual memory bugs that manifest on arm64 devices.
The fix has been merged upstream in commit 5d59aa8f9ce9 ("initramfs: don't free a non-existent initrd"), but backporting that here is more complicated because the patch is stacked upon this patch being reverted along with more patches that rewrites the logic in this area.
Let's just revert the patch from the stable tree instead of trying to backport a collection of fixes to get the final fix from upstream.
The only dependency for taking the fix, 5d59aa8f9ce9, into 4.19 is 23091e28735 ("initramfs: cleanup initrd freeing") which is not too scary.
Is it the case that 25511676362d8 shouldn't have been backported to 4.19 for some reason? If it fixes something on 4.19, I think it's better to take the dependency and the fix instead of reverting.
-- Thanks, Sasha
Quoting Sasha Levin (2019-08-06 13:47:52)
On Tue, Aug 06, 2019 at 10:59:40AM -0700, Stephen Boyd wrote:
This reverts commit 25511676362d8f7d4b8805730a3d29484ceab1ec in the 4.19 stable trees. From what I can tell this commit doesn't do anything to improve the situation, mostly just reordering code to call free_initrd() from one place instead of many. In doing that, it causes free_initrd() to be called even in the case when there isn't an initrd present. That leads to virtual memory bugs that manifest on arm64 devices.
The fix has been merged upstream in commit 5d59aa8f9ce9 ("initramfs: don't free a non-existent initrd"), but backporting that here is more complicated because the patch is stacked upon this patch being reverted along with more patches that rewrites the logic in this area.
Let's just revert the patch from the stable tree instead of trying to backport a collection of fixes to get the final fix from upstream.
The only dependency for taking the fix, 5d59aa8f9ce9, into 4.19 is 23091e28735 ("initramfs: cleanup initrd freeing") which is not too scary.
Is it the case that 25511676362d8 shouldn't have been backported to 4.19 for some reason? If it fixes something on 4.19, I think it's better to take the dependency and the fix instead of reverting.
Ah thanks for taking a second look. I missed that we call free_initrd() in one more case when unpack_to_rootfs() fails and goes into the else statement. I suppose bringing in 23091e28735 ("initramfs: cleanup initrd freeing") in addition to 5d59aa8f9ce9 works just as well, but I'll defer to the persons working in this area.
On 06/08/2019 22:36, Stephen Boyd wrote:
Quoting Sasha Levin (2019-08-06 13:47:52)
On Tue, Aug 06, 2019 at 10:59:40AM -0700, Stephen Boyd wrote:
This reverts commit 25511676362d8f7d4b8805730a3d29484ceab1ec in the 4.19 stable trees. From what I can tell this commit doesn't do anything to improve the situation, mostly just reordering code to call free_initrd() from one place instead of many. In doing that, it causes free_initrd() to be called even in the case when there isn't an initrd present. That leads to virtual memory bugs that manifest on arm64 devices.
The fix has been merged upstream in commit 5d59aa8f9ce9 ("initramfs: don't free a non-existent initrd"), but backporting that here is more complicated because the patch is stacked upon this patch being reverted along with more patches that rewrites the logic in this area.
Let's just revert the patch from the stable tree instead of trying to backport a collection of fixes to get the final fix from upstream.
The only dependency for taking the fix, 5d59aa8f9ce9, into 4.19 is 23091e28735 ("initramfs: cleanup initrd freeing") which is not too scary.
Is it the case that 25511676362d8 shouldn't have been backported to 4.19 for some reason? If it fixes something on 4.19, I think it's better to take the dependency and the fix instead of reverting.
Ah thanks for taking a second look. I missed that we call free_initrd() in one more case when unpack_to_rootfs() fails and goes into the else statement. I suppose bringing in 23091e28735 ("initramfs: cleanup initrd freeing") in addition to 5d59aa8f9ce9 works just as well, but I'll defer to the persons working in this area.
25511676362d 'fixes' a (one-off) memory leak if the initrd setup is misconfigured. So not something that is likely to affect many systems, and a reasonably minor bug. But obviously that commit introduced a more serious regression which my later patch fixed.
As Sasha has pointed out there is only one extra dependency if you want to backport my fix.
So I'm happy either way, the original patch was really more of a cleanup - I'm sorry I missed it when this was being picked up for stable!
Steve
linux-stable-mirror@lists.linaro.org
-
Sasha Levin -
Stephen Boyd -
Steven Price