Re: [PATCH v3 04/11] net: qrtr: support identical node ids

[Simon Horman <horms@kernel.org>]

On Thu, Jul 24, 2025 at 01:24:01AM +0200, Mihai Moldovan wrote:
> From: Denis Kenzior <denkenz@gmail.com>
> 
> Add support for tracking multiple endpoints that may have conflicting
> node identifiers. This is achieved by using both the node and endpoint
> identifiers as the key inside the radix_tree data structure.
> 
> For backward compatibility with existing clients, the previous key
> schema (node identifier only) is preserved. However, this schema will
> only support the first endpoint/node combination.  This is acceptable
> for legacy clients as support for multiple endpoints with conflicting
> node identifiers was not previously possible.
> 
> Signed-off-by: Denis Kenzior <denkenz@gmail.com>
> Reviewed-by: Marcel Holtmann <marcel@holtmann.org>
> Reviewed-by: Andy Gross <agross@kernel.org>
> Signed-off-by: Mihai Moldovan <ionic@ionic.de>

...

> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c

...

> @@ -465,19 +466,36 @@ static struct qrtr_node *qrtr_node_lookup(unsigned int nid)
>   *
>   * This is mostly useful for automatic node id assignment, based on
>   * the source id in the incoming packet.
> + *
> + * Return: 0 on success; negative error code on failure
>   */
> -static void qrtr_node_assign(struct qrtr_node *node, unsigned int nid)
> +static int qrtr_node_assign(struct qrtr_node *node, unsigned int nid)
>  {
>  	unsigned long flags;
> +	unsigned long key;
>  
>  	if (nid == QRTR_EP_NID_AUTO)
> -		return;
> +		return 0;
>  
>  	spin_lock_irqsave(&qrtr_nodes_lock, flags);
> -	radix_tree_insert(&qrtr_nodes, nid, node);
> +
> +	if (node->ep->id > QRTR_INDEX_HALF_UNSIGNED_MAX ||
> +	    nid > QRTR_INDEX_HALF_UNSIGNED_MAX)
> +		return -EINVAL;

Hi Mihai, Denis, all,

This will leak holding qrtr_nodes_lock.

Flagged by Smatch.

> +
> +	/* Always insert with the endpoint_id + node_id */
> +	key = ((unsigned long)(node->ep->id) << QRTR_INDEX_HALF_BITS) |
> +	      ((unsigned long)(nid) & QRTR_INDEX_HALF_UNSIGNED_MAX);
> +	radix_tree_insert(&qrtr_nodes, key, node);
> +
> +	if (!radix_tree_lookup(&qrtr_nodes, nid))
> +		radix_tree_insert(&qrtr_nodes, nid, node);
> +
>  	if (node->nid == QRTR_EP_NID_AUTO)
>  		node->nid = nid;
>  	spin_unlock_irqrestore(&qrtr_nodes_lock, flags);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -571,14 +589,18 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>  
>  	skb_put_data(skb, data + hdrlen, size);

When declared, ret is assigned the value -EINVAL.
And that is still the value of ret if this line is reached.

>  
> -	qrtr_node_assign(node, cb->src_node);
> +	ret = qrtr_node_assign(node, cb->src_node);
> +	if (ret)
> +		goto err;

With this patch, if we get to this line, ret is 0.
Whereas before this patch it was -EINVAL.

>  
>  	if (cb->type == QRTR_TYPE_NEW_SERVER) {
>  		/* Remote node endpoint can bridge other distant nodes */
>  		const struct qrtr_ctrl_pkt *pkt;
>  
>  		pkt = data + hdrlen;
> -		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
> +		ret = qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
> +		if (ret)
> +			goto err;
>  	}
>  
>  	if (cb->type == QRTR_TYPE_RESUME_TX) {

The next portion of this function looks like this:

		ret = qrtr_tx_resume(node, skb);
		if (ret)
			goto err;
	} else {
		ipc = qrtr_port_lookup(cb->dst_port);
		if (!ipc)
			goto err;

If we get to the line above, then the function will jump to err,
free skb, and return ret.

But ret is now 0, whereas before this patch it was -EINVAL.
This seems both to be an unintentional side effect of this patch,
and incorrect.

Also flagged by Smatch.

...