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

[Javi Merino <javi.merino@xxxxxxxxx>]

On Wed, Jan 10, 2024 at 12:25:57PM -0800, Vikram Garhwal wrote:
> Hi Javi,
> Thank you for spotting and fixing this.

Hi Vikram,

> On Tue, Jan 09, 2024 at 03:31:55PM +0000, Julien Grall wrote:
> > 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@xxxxxxxxx>
> > c> ---
> > > CC: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> > > 
> > > 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.

Ok, I'll send a v2 without the printk and add this explanation to the commit 
message.

Thanks!
Javi