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: <240e0299-96ca-2703-1339-1c52c4c1582d@quicinc.com>
Date:   Fri, 13 Jan 2023 17:32:46 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH] pstore/ram: Rework logic for detecting ramoops
Content-Language: en-US
To:     Kees Cook <keescook@chromium.org>
CC:     <tony.luck@intel.com>, <gpiccoli@igalia.com>,
        <linux-hardening@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <1673428065-22356-1-git-send-email-quic_mojha@quicinc.com>
 <202301121337.B8CCCB6@keescook>
From:   Mukesh Ojha <quic_mojha@quicinc.com>
In-Reply-To: <202301121337.B8CCCB6@keescook>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Kees,

Thanks for your comments.

On 1/13/2023 3:09 AM, Kees Cook wrote:
> On Wed, Jan 11, 2023 at 02:37:45PM +0530, Mukesh Ojha 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 the region to be dynamically
>> allocated at runtime, as opposed to being fixed at compile time.
>>
>> Also, Some of the platforms might be still expecting dedicated
>> memory region for ramoops node where the region is known
>> beforehand and platform_get_resource() is used in that case.
>>
>> So, Add logic to detect the start and size of the ramoops memory
>> region by looking up reserved memory region with
>> of_reserved_mem_lookup() when platform_get_resource() failed.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> Thanks for the patch! Notes below...
> 
>> ---
>>   fs/pstore/ram.c | 19 ++++++++++++++-----
>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index ade66db..e4bbba1 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -20,6 +20,7 @@
>>   #include <linux/compiler.h>
>>   #include <linux/of.h>
>>   #include <linux/of_address.h>
>> +#include <linux/of_reserved_mem.h>
>>   
>>   #include "internal.h"
>>   #include "ram_internal.h"
>> @@ -643,6 +644,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>>   {
>>   	struct device_node *of_node = pdev->dev.of_node;
>>   	struct device_node *parent_node;
>> +	struct reserved_mem *rmem;
>>   	struct resource *res;
>>   	u32 value;
>>   	int ret;
>> @@ -651,13 +653,20 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>>   
>>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>   	if (!res) {
>> -		dev_err(&pdev->dev,
>> -			"failed to locate DT /reserved-memory resource\n");
>> -		return -EINVAL;
>> +		rmem = of_reserved_mem_lookup(of_node);
>> +		if (rmem) {
>> +			pdata->mem_size = rmem->size;
>> +			pdata->mem_address = rmem->base;
>> +		} else {
>> +			dev_err(&pdev->dev,
>> +				"failed to locate DT /reserved-memory resource\n");
>> +			return -EINVAL;
>> +		}
> 
> Since the "else" case returns, the traditional code pattern is to leave
> the other case "inline" (an indented), like so:
> 
> 		if (!rmem) {
> 			dev_err(&pdev->dev,
> 				"failed to locate DT /reserved-memory resource\n");
> 			return -EINVAL;
> 		}
> 		pdata->mem_size = rmem->size;
> 		pdata->mem_address = rmem->base;
> 

Fixed it in v2.

> 
>> +	} else {
>> +		pdata->mem_size = resource_size(res);
>> +		pdata->mem_address = res->start;
>>   	}
> 
> Since this change the potential interface with DT, can you also update
> the documentation in:
> 
> Documentation/devicetree/bindings/reserved-memory/ramoops.yaml
> 
> Or maybe my understanding of DT parsing is lacking and this change is
> doing something slightly different?
>

Have updated the docs in v2;

-Mukesh