Re: [PATCH for-4.19(?)] xen/arm: bootfdt: Fix device tree memory node probing

[Michal Orzel <michal.orzel@xxxxxxx>]

On 27/06/2024 12:32, Julien Grall wrote:
> 
> 
> On 27/06/2024 08:40, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 26/06/2024 22:13, Julien Grall wrote:
>>>
>>>
>>> Hi Michal,
>>>
>>> On 26/06/2024 09:04, Michal Orzel wrote:
>>>> Memory node probing is done as part of early_scan_node() that is called
>>>> for each node with depth >= 1 (root node is at depth 0). According to
>>>> Devicetree Specification v0.4, chapter 3.4, /memory node can only exists
>>>> as a top level node. However, Xen incorrectly considers all the nodes with
>>>> unit node name "memory" as RAM. This buggy behavior can result in a
>>>> failure if there are other nodes in the device tree (at depth >= 2) with
>>>> "memory" as unit node name. An example can be a "memory@xxx" node under
>>>> /reserved-memory. Fix it by introducing device_tree_is_memory_node() to
>>>> perform all the required checks to assess if a node is a proper /memory
>>>> node.
>>>>
>>>> Fixes: 3e99c95ba1c8 ("arm, device tree: parse the DTB for RAM location and 
>>>> size")
>>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>>> ---
>>>> 4.19: This patch is fixing a possible early boot Xen failure (before main
>>>> console is initialized). In my case it results in a warning "Shattering
>>>> superpage is not supported" and panic "Unable to setup the directmap 
>>>> mappings".
>>>>
>>>> If this is too late for this patch to go in, we can backport it after the 
>>>> tree
>>>> re-opens.
>>>
>>> The code looks correct to me, but I am not sure about merging it to 4.19.
>>>
>>> This is not a new bug (in fact has been there since pretty much Xen on
>>> Arm was created) and we haven't seen any report until today. So in some
>>> way it would be best to merge it after 4.19 so it can get more testing.
>>>
>>> In the other hand, I guess this will block you. Is this a new platform?
>>> Is it available?
>> Stefano answered this question. Also, IMO this change is quite 
>> straightforward
>> and does not introduce any engineering doubt, so I'm not really sure if it 
>> needs
>> more testing.
> 
> At this stage of the release we should think whether the bug is critical
> enough (rather than the risk is low enough) to be merged. IMHO, it is
> not because this has been there for the past 12 years...
> 
> But if we focus on the "riskness". We are rightly changing an interface
> which possibly someone may have (bogusly) relied on. So there is a
> lowish risk that you may end up to break use-case late in the release
> (we are a couple of weeks away) for use-case that never worked in the
> past 12 years.
> 
> We should also think what the worse that can happen if there is a bug in
> your series. The worse is we would not be able to boot on already
> supported platform. This would be quite a bad regression this late in
> the release. For Device-Tree parsing, I don't think it is enough to just
> test on just a handful of platforms this late in the release.
> 
> Which is why to me the answer to "It should be in 4.19" is not
> straightforward. If we merge post 4.19, then we give the chance to
> people to test, update & adjust their setup if needed.
> 
> Anyway, ultimately this is Oleksii's decision as the release manager.
Ok, I agree with your reasoning.

> 
> [...]
> 
>>>> +/*
>>>> + * Check if a node is a proper /memory node according to Devicetree
>>>> + * Specification v0.4, chapter 3.4.
>>>> + */
>>>> +static bool __init device_tree_is_memory_node(const void *fdt, int node,
>>>> +                                              int depth)
>>>> +{
>>>> +    const char *type;
>>>> +    int len;
>>>> +
>>>> +    if ( depth != 1 )
>>>> +        return false;
>>>> +
>>>> +    if ( !device_tree_node_matches(fdt, node, "memory") )
>>>> +        return false;
>>>> +
>>>> +    type = fdt_getprop(fdt, node, "device_type", &len);
>>>> +    if ( !type )
>>>> +        return false;
>>>> +
>>>> +    if ( (len <= 0) || strcmp(type, "memory") )
>>>
>>> I would consider to use strncmp() to avoid relying on the property to be
>>> well-formed (i.e. nul-terminated).
>> Are you sure? AFAIR, libfdt returns NULL and -FDT_ERR_TRUNCATED as len if a 
>> string is non null terminated.
> 
> I can't find such code in path. Do you have any pointer?
I checked and I cannot find such code either. I made the wrong assumption 
thinking that fdt_getprop can only work with strings.
Therefore, I'm ok with changing s/strcmp/strncmp/ for hardening. Shall I respin 
the patch marking it as for-4.20?


~Michal