Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a2c9417b-3a50-bea4-dd39-2f021c196fa1@quicinc.com>
Date:   Thu, 22 Jun 2023 12:51:00 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops
 memory regions
To:     Kees Cook <kees@kernel.org>,
        Isaac Manjarres <isaacmanjarres@google.com>,
        Kees Cook <keescook@chromium.org>
CC:     John Stultz <jstultz@google.com>, Tony Luck <tony.luck@intel.com>,
        "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
        <kernel-team@android.com>, <linux-hardening@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        "Trilok Soni" <quic_tsoni@quicinc.com>,
        Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>,
        Mukesh Ojha <quic_mojha@quicinc.com>
References: <20230622005213.458236-1-isaacmanjarres@google.com>
 <CANDhNCrEhx4LUPaz-FHZZJMG2yX670hk-vGTZs=HxiGR18zm5A@mail.gmail.com>
 <202306212212.5E53607@keescook> <ZJSES98P+zzrhBI5@google.com>
 <3A2CFB4D-27D0-4FEB-93B4-2F888305DE5A@kernel.org>
Content-Language: en-US
From:   Elliot Berman <quic_eberman@quicinc.com>
In-Reply-To: <3A2CFB4D-27D0-4FEB-93B4-2F888305DE5A@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/22/2023 10:58 AM, Kees Cook wrote:
> On June 22, 2023 10:26:35 AM PDT, Isaac Manjarres <isaacmanjarres@google.com> wrote:
>> On Wed, Jun 21, 2023 at 10:15:45PM -0700, Kees Cook wrote:
>>> On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz wrote:
>>>>> The reserved memory region for ramoops is assumed to be at a fixed
>>>>> and known location when read from the devicetree. This is not desirable
>>>>> in environments where it is preferred for the region to be dynamically
>>>>> allocated early during boot (i.e. the memory region is defined with
>>>>> the "alloc-ranges" property instead of the "reg" property).
>>>>>
>>>>
>>>> Thanks for sending this out, Isaac!
>>>>
>>>> Apologies, I've forgotten much of the details around dt bindings here,
>>>> so forgive my questions:
>>>> If the memory is dynamically allocated from a specific range, is it
>>>> guaranteed to be consistently the same address boot to boot?
>>>>
>>>>> Since ramoops regions are part of the reserved-memory devicetree
>>>>> node, they exist in the reserved_mem array. This means that the
>>>>> of_reserved_mem_lookup() function can be used to retrieve the
>>>>> reserved_mem structure for the ramoops region, and that structure
>>>>> contains the base and size of the region, even if it has been
>>>>> dynamically allocated.
>>>>
>>>> I think this is answering my question above, but it's a little opaque,
>>>> so I'm not sure.
>>>
>>> Yeah, I had exactly the same question: will this be the same
>>> boot-to-boot?
>>
>> Hi Kees,
>>
>> Thank you for taking a look at this patch and for your review! When the
>> alloc-ranges property is used to describe a memory region, the memory
>> region will always be allocated within that range, but it's not
>> guaranteed to be allocated at the same base address across reboots.
>>
>> I had proposed re-wording the end of the commit message in my response
>> to John as follows:
>>
>> "...and that structure contains the address of the base of the region
>> that was allocated at boot anywhere within the range specified by the
>> "alloc-ranges" devicetree property."
>>
>> Does that clarify things better?
> 
> I am probably misunderstanding something still, but it it varies from boot to boot, what utility is there for pstore if it changes? I.e. the things written during the last boot would then no longer accessible at the next boot? E.g.:
> 
> Boot 1.
> Get address Foo.
> Crash, write to Foo.
> Boot 2.
> Get address Bar, different from Foo.
> Nothing found at Bar, so nothing populated in pstorefs; crash report from Boot 1 unavailable.
> 
> I feel like there is something I don't understand about the Foo/Bar addresses in my example.
> 

I believe this is being added to support the QCOM SoC minidump feature. 
Mukesh has posted it on the mailing lists here:

https://lore.kernel.org/all/1683133352-10046-1-git-send-email-quic_mojha@quicinc.com/

https://lore.kernel.org/all/1683133352-10046-10-git-send-email-quic_mojha@quicinc.com/

Mukesh, could you comment whether this patch is wanted for us in the 
version you have posted? It looks like maybe not based on the commit 
text in patch #9.

  - Elliot