linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"
@ 2019-08-06 17:59 Stephen Boyd
  2019-08-06 20:47 ` Sasha Levin
  0 siblings, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2019-08-06 17:59 UTC (permalink / raw)
  To: stable
  Cc: linux-kernel, Sasha Levin, Steven Price, Mark Rutland, Christoph Hellwig

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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"
  2019-08-06 17:59 [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails" Stephen Boyd
@ 2019-08-06 20:47 ` Sasha Levin
  2019-08-06 21:36   ` Stephen Boyd
  0 siblings, 1 reply; 4+ messages in thread
From: Sasha Levin @ 2019-08-06 20:47 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: stable, linux-kernel, Steven Price, Mark Rutland, Christoph Hellwig

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"
  2019-08-06 20:47 ` Sasha Levin
@ 2019-08-06 21:36   ` Stephen Boyd
  2019-08-07 11:53     ` Steven Price
  0 siblings, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2019-08-06 21:36 UTC (permalink / raw)
  To: Sasha Levin
  Cc: stable, linux-kernel, Steven Price, Mark Rutland, Christoph Hellwig

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.


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails"
  2019-08-06 21:36   ` Stephen Boyd
@ 2019-08-07 11:53     ` Steven Price
  0 siblings, 0 replies; 4+ messages in thread
From: Steven Price @ 2019-08-07 11:53 UTC (permalink / raw)
  To: Stephen Boyd, Sasha Levin
  Cc: stable, linux-kernel, Mark Rutland, Christoph Hellwig

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-08-07 11:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 17:59 [PATCH 4.19] Revert "initramfs: free initrd memory if opening /initrd.image fails" Stephen Boyd
2019-08-06 20:47 ` Sasha Levin
2019-08-06 21:36   ` Stephen Boyd
2019-08-07 11:53     ` Steven Price

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).