Re: [PATCH 1/4] bootconfig: return 0 from xbc_snprint_cmdline() for a leaf root

[Breno Leitao <leitao@debian.org>]

On Wed, May 27, 2026 at 05:05:21PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The early return incorrectly assumes `xbc_node_is_leaf(root)` means there are no descendant key/value pairs, causing it to silently skip rendering valid subkeys if the root node has both a value and subkeys.
> --
> 
> commit 7fe40e4bc583f53fe486de6d90af68f475369ee3
> Author: Breno Leitao <leitao@debian.org>
> 
> bootconfig: return 0 from xbc_snprint_cmdline() for a leaf root
> 
> This commit changes xbc_snprint_cmdline() to return 0 instead of an error
> when the root node is considered a leaf, preventing a build failure for
> empty subtrees in the bootconfig tools.
> 
> > diff --git a/lib/bootconfig.c b/lib/bootconfig.c
> > index f445b7703fdd9..3a102c9122f71 100644
> > --- a/lib/bootconfig.c
> > +++ b/lib/bootconfig.c
> > @@ -431,6 +431,16 @@ int __init xbc_snprint_cmdline(char *buf, size_t size, struct xbc_node *root)
> >  	const char *val, *q;
> >  	int ret;
> >  
> > +	/*
> > +	 * A leaf @root (e.g. an empty "kernel {}" subtree, or a key whose
> > +	 * only child is a value node) has no descendant key/value pairs to
> > +	 * render. The leaf-finding iterator below would otherwise return
> > +	 * @root itself, which xbc_node_compose_key_after() rejects with
> > +	 * -EINVAL.
> > +	 */
> > +	if (root && xbc_node_is_leaf(root))
> > +		return 0;
> 
> [Severity: High]
> This is a pre-existing issue, but does this early return unintentionally
> skip valid descendant subkeys?
> 
> Looking at include/linux/bootconfig.h, xbc_node_is_leaf() evaluates to true
> if a node has no children or its first child is a value node:
> 
> static inline __init bool xbc_node_is_leaf(struct xbc_node *node)
> {
> 	return xbc_node_is_key(node) &&
> 		(!node->child || xbc_node_is_value(xbc_node_get_child(node)));
> }
> 
> If a bootconfig contains a root node with both a value and subkeys
> (for example, "kernel = val" and "kernel.param = 1"), xbc_node_is_leaf()
> will evaluate to true for the root node.
> 
> Since this patch changes the behavior to return 0 instead of propagating
> the -EINVAL error, will it silently skip rendering these descendant subkeys
> and cause missing kernel command line parameters?
> 
> Would it be safer to drop this early return and instead add a check like
> "if (knode == root) continue;" inside the xbc_node_for_each_key_value() loop
> to skip the root's value while continuing to process its subkeys?


Good catch, you're right. If @root carries both a value and child keys
(e.g. "kernel = val" together with "kernel.foo = bar"),
xbc_node_is_leaf(root) is true -- root's first child is a value node --
so the early return drops kernel.foo=bar, which the pre-patch code did
render.

For v2 I'll drop the early return and skip @root itself inside the loop,
as you suggest:

	xbc_node_for_each_key_value(root, knode, val) {
		/*
			* An empty or value-only @root yields @root itself here;
			* skip it so we still render any real descendant keys
			* (and return 0 rather than -EINVAL for an empty subtree).
			*/
		if (knode == root)
			continue;
		...
	}