Re: [PATCH for-4.18 v1] xen/common: Don't dereference overlay_node after checking that it is NULL

[Vikram Garhwal <vikram.garhwal@amd.com>]

Hi Javi,
Thank you for spotting and fixing this.
On Tue, Jan 09, 2024 at 03:31:55PM +0000, Julien Grall wrote:
> Hi Javi,
> 
> Title: Any reason this is titled for-4.18? Shouldn't this patch also be
> merged in staging?
> 
> On 09/01/2024 14:19, Javi Merino wrote:
> > In remove_nodes(), overlay_node is dereferenced when printing the
> > error message even though it is known to be NULLL.  Fix the error
> 
> Typo: s/NULLL/NULL/
> 
> This can be fixed on commit if there is nothing else.
> 
> > message to avoid dereferencing a NULL pointer.
> > 
> > The semantic patch that spots this code is available in
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/null/deref_null.cocci?id=1f874787ed9a2d78ed59cb21d0d90ac0178eceb0
> 
> Good catch and glad to see that coccinelle can work on Xen. I am looking
> forward for more work in that area :).
> 
> > 
> > Fixes: 7e5c4a8b86f1 ("xen/arm: Implement device tree node removal functionalities")
> > Signed-off-by: Javi Merino <javi.merino@cloud.com>
> c> ---
> > CC: Vikram Garhwal <vikram.garhwal@amd.com>
> > 
> > Vikram, I didn't know what to put in the error message.  Feel free to
> > suggest something more appropriate than "Device not present in the
> > tree".
> 
> More questions for Vikram, looking at the code, it is not 100% clear in
> which condition overlay_node could be NULL. Is this a programming error? if
> so, maybe this should be an ASSERT_UNREACHABLE() (could be added separately)
> and it would be fine to print nothing.
> 
This can happen with failures in add_nodes() function. add_nodes() failure will
try to call remove_nodes function. Depending on where add_nodes() is failed,
nodes_address may or may not be NULL.

We also added a detailed comment on this:
https://github.com/xen-project/xen/blob/5a3ace21f3d779b291a2d305824b2820d88de7f1/xen/common/dt-overlay.c#L816

For now, we can return from here without printing anything as error message will
be printed by the caller of remove_nodes() anyway.

> Cheers,
> 
> -- 
> Julien Grall