[PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"

[PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"

[Stephen Boyd]

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
-- 
Sent by a computer through tubes

Re: [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"

[Sasha Levin]

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

Re: [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"

[Stephen Boyd]

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.

Re: [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"

[Steven Price]

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