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