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.mer...@cloud.com> > > c> --- > > > CC: Vikram Garhwal <vikram.garh...@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. Ok, I'll send a v2 without the printk and add this explanation to the commit message. Thanks! Javi
