xen/arm: Check return code from recursive calls to scan_pfdt_node()

[for-4.18][PATCH] xen/arm: Check return code from recursive calls to scan_pfdt_node()

Posted by Michal Orzel 2 years, 3 months ago

At the moment, we do not check a return code from scan_pfdt_node()
called recursively. This means that any issue that may occur while
parsing and copying the passthrough nodes is hidden and Xen continues
to boot a domain despite errors. This may lead to incorrect device tree
generation and various guest issues (e.g. trap on attempt to access MMIO
not mapped in P2M). Fix it.

Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
@Henry:
This is a bug fix, so I think we should have it in 4.18 given the possible
consequences I described in the commit msg. I don't see any risks as this change
only checks the return code for an error.
---
 xen/arch/arm/domain_build.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 24c9019cc43c..49792dd590ee 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
     node_next = fdt_first_subnode(pfdt, nodeoff);
     while ( node_next > 0 )
     {
-        scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
-                       scan_passthrough_prop);
+        rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
+                            scan_passthrough_prop);
+        if ( rc )
+            return rc;
+
         node_next = fdt_next_subnode(pfdt, node_next);
     }
 
-- 
2.25.1

Re: [for-4.18][PATCH] xen/arm: Check return code from recursive calls to scan_pfdt_node()

Posted by Bertrand Marquis 2 years, 3 months ago

Hi Michal,

> On 16 Oct 2023, at 14:45, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> At the moment, we do not check a return code from scan_pfdt_node()
> called recursively. This means that any issue that may occur while
> parsing and copying the passthrough nodes is hidden and Xen continues
> to boot a domain despite errors. This may lead to incorrect device tree
> generation and various guest issues (e.g. trap on attempt to access MMIO
> not mapped in P2M). Fix it.
> 
> Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Good finding :-)

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> @Henry:
> This is a bug fix, so I think we should have it in 4.18 given the possible
> consequences I described in the commit msg. I don't see any risks as this change
> only checks the return code for an error.
> ---
> xen/arch/arm/domain_build.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 24c9019cc43c..49792dd590ee 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
>     node_next = fdt_first_subnode(pfdt, nodeoff);
>     while ( node_next > 0 )
>     {
> -        scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> -                       scan_passthrough_prop);
> +        rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> +                            scan_passthrough_prop);
> +        if ( rc )
> +            return rc;
> +
>         node_next = fdt_next_subnode(pfdt, node_next);
>     }
> 
> -- 
> 2.25.1
>

Re: [for-4.18][PATCH] xen/arm: Check return code from recursive calls to scan_pfdt_node()

Posted by Henry Wang 2 years, 3 months ago

Hi Michal,

> On Oct 16, 2023, at 21:28, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
> 
> Hi Michal,
> 
>> On 16 Oct 2023, at 14:45, Michal Orzel <michal.orzel@amd.com> wrote:
>> 
>> At the moment, we do not check a return code from scan_pfdt_node()
>> called recursively. This means that any issue that may occur while
>> parsing and copying the passthrough nodes is hidden and Xen continues
>> to boot a domain despite errors. This may lead to incorrect device tree
>> generation and various guest issues (e.g. trap on attempt to access MMIO
>> not mapped in P2M). Fix it.
>> 
>> Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb")
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> 
> Good finding :-)
> 
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> 
> Cheers
> Bertrand
> 
>> ---
>> @Henry:
>> This is a bug fix, so I think we should have it in 4.18 given the possible
>> consequences I described in the commit msg. I don't see any risks as this change
>> only checks the return code for an error.
>> ---
>> xen/arch/arm/domain_build.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 24c9019cc43c..49792dd590ee 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
>>    node_next = fdt_first_subnode(pfdt, nodeoff);
>>    while ( node_next > 0 )
>>    {
>> -        scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
>> -                       scan_passthrough_prop);
>> +        rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
>> +                            scan_passthrough_prop);
>> +        if ( rc )
>> +            return rc;
>> +
>>        node_next = fdt_next_subnode(pfdt, node_next);
>>    }
>> 
>> --
>> 2.25.1
>> 
>

Re: [for-4.18][PATCH] xen/arm: Check return code from recursive calls to scan_pfdt_node()

Posted by Luca Fancellu 2 years, 3 months ago


> On 16 Oct 2023, at 13:45, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> At the moment, we do not check a return code from scan_pfdt_node()
> called recursively. This means that any issue that may occur while
> parsing and copying the passthrough nodes is hidden and Xen continues
> to boot a domain despite errors. This may lead to incorrect device tree
> generation and various guest issues (e.g. trap on attempt to access MMIO
> not mapped in P2M). Fix it.
> 
> Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Hi Michal,

Yes makes sense!

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>


> ---
> @Henry:
> This is a bug fix, so I think we should have it in 4.18 given the possible
> consequences I described in the commit msg. I don't see any risks as this change
> only checks the return code for an error.
> ---
> xen/arch/arm/domain_build.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 24c9019cc43c..49792dd590ee 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt,
>     node_next = fdt_first_subnode(pfdt, nodeoff);
>     while ( node_next > 0 )
>     {
> -        scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> -                       scan_passthrough_prop);
> +        rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> +                            scan_passthrough_prop);
> +        if ( rc )
> +            return rc;
> +
>         node_next = fdt_next_subnode(pfdt, node_next);
>     }
> 
> -- 
> 2.25.1
> 
>