QRTR Multi-endpoint support

PATCH Superseded Mihai Moldovan 4 participants 21 messages Jul 23, 2025 – Aug 4, 2025

Subsystems (5)

Networking Network drivers Arm Qualcomm mailing list Sockets +1 more subsystems

lore

Share & tools

lore

Mihai Moldovan

author

2025-07-23 23:24 UTC #0 lore raw

v3 cover QRTR Multi-endpoint support

I am incredibly thankful for Denis's work on this. To get this back on track and to eventually get it merged, with his permission, I'm resubmitting his patch set with issues in the previous review rounds resolved. This feature is a prerequisite for my work on ath1{1,2}k to allow using multiple devices in one computer.

The original description follows:

The current implementation of QRTR assumes that each entity on the QRTR IPC bus is uniquely identifiable by its node/port combination, with node/port combinations being used to route messages between entities.

However, this assumption of uniqueness is problematic in scenarios where multiple devices with the same node/port combinations are connected to the system. A practical example is a typical consumer PC with multiple PCIe-based devices, such as WiFi cards or 5G modems, where each device could potentially have the same node identifier set. In such cases, the current QRTR protocol implementation does not provide a mechanism to differentiate between these devices, making it impossible to support communication with multiple identical devices.

This patch series addresses this limitation by introducing support for a concept of an 'endpoint.' Multiple devices with conflicting node/port combinations can be supported by assigning a unique endpoint identifier to each one. Such endpoint identifiers can then be used to distinguish between devices while sending and receiving messages over QRTR sockets.

The patch series maintains backward compatibility with existing clients:
the endpoint concept is added using auxiliary data that can be added to recvmsg and sendmsg system calls. The QRTR socket interface is extended as follows:

- Adds QRTR_ENDPOINT auxiliary data element that reports which endpoint generated a particular message. This auxiliary data is only reported if the socket was explicitly opted in using setsockopt, enabling the QRTR_REPORT_ENDPOINT socket option. SOL_QRTR socket level was added to facilitate this. This requires QRTR clients to be updated to use recvmsg instead of the more typical recvfrom() or recv() use.

- Similarly, QRTR_ENDPOINT auxiliary data element can be included in sendmsg() requests. This will allow clients to route QRTR messages to the desired endpoint, even in cases of node/port conflict between multiple endpoints.

- Finally, QRTR_BIND_ENDPOINT socket option is introduced. This allows clients to bind to a particular endpoint (such as a 5G PCIe modem) if they're only interested in receiving or sending messages to this device.

v3:
- rebased against current master
- fix checkpatch.pl warnings
- fix overflow issues with unsigned long radix tree keys by using the upper half of the storage space for one element and the lower half of storage for the other element, making sure that the elements fit into their respective storage space
- Link to v2: https://msgid.link/[email protected]

v2:
- rebased against current master
- fixed most issues found in first review round (see individual commits), minus the 32-bit long
unsafe use
- Link to v1: https://msgid.link/[email protected]

Denis Kenzior (10):
net: qrtr: ns: validate msglen before ctrl_pkt use
net: qrtr: allocate and track endpoint ids
net: qrtr: support identical node ids
net: qrtr: Report sender endpoint in aux data
net: qrtr: Report endpoint for locally generated messages
net: qrtr: Allow sendmsg to target an endpoint
net: qrtr: allow socket endpoint binding
net: qrtr: Drop remote {NEW|DEL}_LOOKUP messages
net: qrtr: ns: support multiple endpoints
net: qrtr: mhi: Report endpoint id in sysfs

Mihai Moldovan (1):
net: qrtr: fit node ID + port number combination into unsigned long

 include/linux/socket.h    |   1 +
 include/uapi/linux/qrtr.h |   7 +
 net/qrtr/af_qrtr.c        | 400 ++++++++++++++++++++++++++++++++------
 net/qrtr/mhi.c            |  14 ++
 net/qrtr/ns.c             | 299 +++++++++++++++++-----------
 net/qrtr/qrtr.h           |   4 +
 6 files changed, 544 insertions(+), 181 deletions(-)

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #1 lore raw

PATCH v3 1/11 net: qrtr: ns: validate msglen before ctrl_pkt use

From: Denis Kenzior <[email protected]>

The qrtr_ctrl_pkt structure is currently accessed without checking if the received payload is large enough to hold the structure's fields. Add a check to ensure the payload length is sufficient.

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

Fixes 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")

---

v3:
- add Fixes: tag
- rebase against current master
- Link to v2: https://msgid.link/866f309e9739d770dce7e8c648b562d37db1d8b5.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- use correct size of packet structure as per review comment
- Link to v1: https://msgid.link/[email protected]

---
 net/qrtr/ns.c | 3 +++
 1 file changed, 3 insertions(+)

net/qrtr/ns.c +3 -0

index 3de9350cbf30..2bcfe539dc3e 100644
@@ -619,6 +619,9 @@ static void qrtr_ns_worker(struct work_struct *work)
 			break;
 		}
 
+		if ((size_t)msglen < sizeof(*pkt))
+			break;
+
 		pkt = recv_buf;
 		cmd = le32_to_cpu(pkt->cmd);
 		if (cmd < ARRAY_SIZE(qrtr_ctrl_pkt_strings) &&

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #2 lore raw

PATCH v3 2/11 net: qrtr: allocate and track endpoint ids

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

Currently, QRTR endpoints are tracked solely by their pointer value, which is sufficient as they are not exposed to user space and it is assumed that each endpoint has a unique set of node identifiers associated with it. However, this assumption does not hold when multiple devices of the same type are connected to the system. For example, multiple PCIe based 5G modems. Such a setup results in multiple endpoints with confliciting node identifiers.

To enable support for such scenarios, introduce the ability to track and assign unique identifiers to QRTR endpoints. These identifiers can then be exposed to user space, allowing for userspace clients to identify which endpoint sent a given message, or to direct a message to a specific endpoint.

A simple allocation strategy is used based on xa_alloc_cyclic. Remote endpoint ids start at 'qrtr_local_nid' + 1. Since qrtr_local_nid is currently always set to 1 and never changed, node identifiers start at '1' for the local endpoint and 2..INT_MAX for remote endpoints.

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- Link to v2: https://msgid.link/86ef12964a23c9331be16961a5f9c6ec857aa56c.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- Link to v1: https://msgid.link/[email protected]

---
 net/qrtr/af_qrtr.c | 24 ++++++++++++++++++++++++
 net/qrtr/qrtr.h    |  1 +
 2 files changed, 25 insertions(+)

net/qrtr/af_qrtr.c +24 -0

index 00c51cf693f3..be275871fb2a 100644
@@ -22,6 +22,7 @@
 #define QRTR_MAX_EPH_SOCKET 0x7fff
 #define QRTR_EPH_PORT_RANGE \
 		XA_LIMIT(QRTR_MIN_EPH_SOCKET, QRTR_MAX_EPH_SOCKET)
+#define QRTR_ENDPOINT_RANGE XA_LIMIT(qrtr_local_nid + 1, INT_MAX)
 
 #define QRTR_PORT_CTRL_LEGACY 0xffff

@@ -109,6 +110,10 @@ static LIST_HEAD(qrtr_all_nodes);
 /* lock for qrtr_all_nodes and node reference */
 static DEFINE_MUTEX(qrtr_node_lock);
 
+/* endpoint id allocation management */
+static DEFINE_XARRAY_ALLOC(qrtr_endpoints);
+static u32 next_endpoint_id;
+
 /* local port allocation management */
 static DEFINE_XARRAY_ALLOC(qrtr_ports);

@@ -585,6 +590,8 @@ static struct sk_buff *qrtr_alloc_ctrl_packet(struct qrtr_ctrl_pkt **pkt,
 int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 {
 	struct qrtr_node *node;
+	u32 endpoint_id;
+	int rc;
 
 	if (!ep || !ep->xmit)
 		return -EINVAL;

@@ -593,6 +600,13 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 	if (!node)
 		return -ENOMEM;
 
+	rc = xa_alloc_cyclic(&qrtr_endpoints, &endpoint_id, NULL,
+			     QRTR_ENDPOINT_RANGE, &next_endpoint_id,
+			     GFP_KERNEL);
+
+	if (rc < 0)
+		goto free_node;
+
 	kref_init(&node->ref);
 	mutex_init(&node->ep_lock);
 	skb_queue_head_init(&node->rx_queue);

@@ -608,8 +622,12 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 	list_add(&node->item, &qrtr_all_nodes);
 	mutex_unlock(&qrtr_node_lock);
 	ep->node = node;
+	ep->id = endpoint_id;
 
 	return 0;
+free_node:
+	kfree(node);
+	return rc;
 }
 EXPORT_SYMBOL_GPL(qrtr_endpoint_register);

@@ -628,8 +646,10 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep)
 	struct sk_buff *skb;
 	unsigned long flags;
 	void __rcu **slot;
+	u32 endpoint_id;
 
 	mutex_lock(&node->ep_lock);
+	endpoint_id = node->ep->id;
 	node->ep = NULL;
 	mutex_unlock(&node->ep_lock);

@@ -656,6 +676,10 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep)
 	mutex_unlock(&node->qrtr_tx_lock);
 
 	qrtr_node_release(node);
+
+	xa_erase(&qrtr_endpoints, endpoint_id);
+
+	ep->id = 0;
 	ep->node = NULL;
 }
 EXPORT_SYMBOL_GPL(qrtr_endpoint_unregister);

net/qrtr/qrtr.h +1 -0

index 3f2d28696062..11b897af05e6 100644
@@ -21,6 +21,7 @@ struct qrtr_endpoint {
 	int (*xmit)(struct qrtr_endpoint *ep, struct sk_buff *skb);
 	/* private: not for endpoint use */
 	struct qrtr_node *node;
+	u32 id;
 };
 
 int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid);

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #3 lore raw

PATCH v3 3/11 net: qrtr: fit node ID + port number combination into unsigned long

In reply to Mihai Moldovan

The flow control implementation uses a radix tree to store node ID and port number combinations and the key length is hardcoded to unsigned long.

The original implementation shifted the node ID up by 32 bits and added the port number to the lower 64 bits of the unsigned long value to create a key.

Unfortunately, since both node IDs and port numbers are defined as u32, this will overflow on platforms where sizeof(unsigned long) < 8 (which are most 32 bit platforms) and essentially just drop the node ID part.

To fix this, build the key in a generic way, using half of the unsigned long space for the node ID and the other half for the port number.

This will be transparent to platforms where sizeof(unsigned long) >= 8 and fix overflow issues otherwise.

The caveat, of course. is that, for platforms where sizeof(unsigned long) < 8, the supported amount of node IDs and port numbers will be severely limited - to half of sizeof(unsigned long), which typically will be 16 bits. Needless to say, we have to check if both values fit into this limit.

This limitation is probably not going to be an issue in real-world scenarios, but if it turns out to be one after all, we could switch from a radix tree implementation to an XArray implementation.

Signed-off-by Mihai Moldovan <[email protected]>

Fixes 5fdeb0d372ab ("net: qrtr: Implement outgoing flow control")

---

v3:
- introduce commit

---
 net/qrtr/af_qrtr.c | 76 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 65 insertions(+), 11 deletions(-)

net/qrtr/af_qrtr.c +65 ~~-11~~

index be275871fb2a..1cb13242e41b 100644
@@ -117,13 +117,33 @@ static u32 next_endpoint_id;
 /* local port allocation management */
 static DEFINE_XARRAY_ALLOC(qrtr_ports);
 
+/* The radix tree API uses fixed unsigned long keys and we will have to make
+ * do with that.
+ * These keys are often a combination of node IDs (currently u32) and
+ * port numbers (also currently u32).
+ * Using the high 32 bits for the node ID and the low 32 bits for the
+ * port number will work fine to create keys on platforms where unsigned long
+ * is 64 bits wide, but obviously is not be possible on platforms where
+ * unsigned long is smaller.
+ * Virtually split up unsigned long in half and assign the upper bits to
+ * node IDs and the lower bits to the port number, however big that may be.
+ */
+#define QRTR_INDEX_HALF_BITS (RADIX_TREE_INDEX_BITS >> 1)
+
+#define QRTR_INDEX_HALF_UNSIGNED_MAX ((~(unsigned long)(0)) >> QRTR_INDEX_HALF_BITS)
+#define QRTR_INDEX_HALF_UNSIGNED_MIN ((unsigned long)(0))
+
+#define QRTR_INDEX_HALF_SIGNED_MAX ((long)(QRTR_INDEX_HALF_UNSIGNED_MAX) >> 1)
+#define QRTR_INDEX_HALF_SIGNED_MIN ((long)(-1) - QRTR_INDEX_HALF_SIGNED_MAX)
+
 /**
  * struct qrtr_node - endpoint node
  * @ep_lock: lock for endpoint management and callbacks
  * @ep: endpoint
  * @ref: reference count for node
  * @nid: node id
- * @qrtr_tx_flow: tree of qrtr_tx_flow, keyed by node << 32 | port
+ * @qrtr_tx_flow: tree of qrtr_tx_flow, keyed by
+ *                node << QRTR_INDEX_HALF_BITS | port
  * @qrtr_tx_lock: lock for qrtr_tx_flow inserts
  * @rx_queue: receive queue
  * @item: list item for broadcast list

@@ -222,16 +242,23 @@ static void qrtr_node_release(struct qrtr_node *node)
  * qrtr_tx_resume() - reset flow control counter
  * @node:	qrtr_node that the QRTR_TYPE_RESUME_TX packet arrived on
  * @skb:	resume_tx packet
+ *
+ * Return: 0 on success; negative error code on failure
  */
-static void qrtr_tx_resume(struct qrtr_node *node, struct sk_buff *skb)
+static int qrtr_tx_resume(struct qrtr_node *node, struct sk_buff *skb)
 {
 	struct qrtr_ctrl_pkt *pkt = (struct qrtr_ctrl_pkt *)skb->data;
 	u64 remote_node = le32_to_cpu(pkt->client.node);
 	u32 remote_port = le32_to_cpu(pkt->client.port);
 	struct qrtr_tx_flow *flow;
-	unsigned long key;
+	unsigned long key = 0;
 
-	key = remote_node << 32 | remote_port;
+	if (remote_node > QRTR_INDEX_HALF_UNSIGNED_MAX ||
+	    remote_port > QRTR_INDEX_HALF_UNSIGNED_MAX)
+		return -EINVAL;
+
+	key = ((unsigned long)(remote_node) << QRTR_INDEX_HALF_BITS) |
+	      ((unsigned long)(remote_port) & QRTR_INDEX_HALF_UNSIGNED_MAX);
 
 	rcu_read_lock();
 	flow = radix_tree_lookup(&node->qrtr_tx_flow, key);

@@ -244,6 +271,8 @@ static void qrtr_tx_resume(struct qrtr_node *node, struct sk_buff *skb)
 	}
 
 	consume_skb(skb);
+
+	return 0;
 }
 
 /**

@@ -264,11 +293,20 @@ static void qrtr_tx_resume(struct qrtr_node *node, struct sk_buff *skb)
 static int qrtr_tx_wait(struct qrtr_node *node, int dest_node, int dest_port,
 			int type)
 {
-	unsigned long key = (u64)dest_node << 32 | dest_port;
+	unsigned long key = 0;
 	struct qrtr_tx_flow *flow;
 	int confirm_rx = 0;
 	int ret;
 
+	if (dest_node < QRTR_INDEX_HALF_SIGNED_MIN ||
+	    dest_node > QRTR_INDEX_HALF_SIGNED_MAX ||
+	    dest_port < QRTR_INDEX_HALF_SIGNED_MIN ||
+	    dest_port > QRTR_INDEX_HALF_SIGNED_MAX)
+		return -EINVAL;
+
+	key = ((unsigned long)(dest_node) << QRTR_INDEX_HALF_BITS) |
+	      ((unsigned long)(dest_port) & QRTR_INDEX_HALF_UNSIGNED_MAX);
+
 	/* Never set confirm_rx on non-data packets */
 	if (type != QRTR_TYPE_DATA)
 		return 0;

@@ -324,13 +362,24 @@ static int qrtr_tx_wait(struct qrtr_node *node, int dest_node, int dest_port,
  * message associated with the dropped confirm_rx message.
  * Work around this by marking the flow as having a failed transmission and
  * cause the next transmission attempt to be sent with the confirm_rx.
+ *
+ * Return: 0 on success; negative error code on failure
  */
-static void qrtr_tx_flow_failed(struct qrtr_node *node, int dest_node,
-				int dest_port)
+static int qrtr_tx_flow_failed(struct qrtr_node *node, int dest_node,
+			       int dest_port)
 {
-	unsigned long key = (u64)dest_node << 32 | dest_port;
+	unsigned long key = 0;
 	struct qrtr_tx_flow *flow;
 
+	if (dest_node < QRTR_INDEX_HALF_SIGNED_MIN ||
+	    dest_node > QRTR_INDEX_HALF_SIGNED_MAX ||
+	    dest_port < QRTR_INDEX_HALF_SIGNED_MIN ||
+	    dest_port > QRTR_INDEX_HALF_SIGNED_MAX)
+		return -EINVAL;
+
+	key = ((unsigned long)(dest_node) << QRTR_INDEX_HALF_BITS) |
+	      ((unsigned long)(dest_port) & QRTR_INDEX_HALF_UNSIGNED_MAX);
+
 	rcu_read_lock();
 	flow = radix_tree_lookup(&node->qrtr_tx_flow, key);
 	rcu_read_unlock();

@@ -339,6 +388,8 @@ static void qrtr_tx_flow_failed(struct qrtr_node *node, int dest_node,
 		flow->tx_failed = 1;
 		spin_unlock_irq(&flow->resume_tx.lock);
 	}
+
+	return 0;
 }
 
 /* Pass an outgoing packet socket buffer to the endpoint driver. */

@@ -386,7 +437,7 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	/* Need to ensure that a subsequent message carries the otherwise lost
 	 * confirm_rx flag if we dropped this one */
 	if (rc && confirm_rx)
-		qrtr_tx_flow_failed(node, to->sq_node, to->sq_port);
+		rc = qrtr_tx_flow_failed(node, to->sq_node, to->sq_port);
 
 	return rc;
 }

@@ -448,6 +499,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 	size_t size;
 	unsigned int ver;
 	size_t hdrlen;
+	int ret = -EINVAL;
 
 	if (len == 0 || len & 3)
 		return -EINVAL;

@@ -530,7 +582,9 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 	}
 
 	if (cb->type == QRTR_TYPE_RESUME_TX) {
-		qrtr_tx_resume(node, skb);
+		ret = qrtr_tx_resume(node, skb);
+		if (ret)
+			goto err;
 	} else {
 		ipc = qrtr_port_lookup(cb->dst_port);
 		if (!ipc)

@@ -548,7 +602,7 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 
 err:
 	kfree_skb(skb);
-	return -EINVAL;
+	return ret;
 
 }
 EXPORT_SYMBOL_GPL(qrtr_endpoint_post);

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #4 lore raw

PATCH v3 4/11 net: qrtr: support identical node ids

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

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 <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- port usage of [endpoint ID|node ID] key usage to the generic
solution already established for the [node ID|port number] usage
- Link to v2: https://msgid.link/4d0fe1eab4b38fb85e2ec53c07289bc0843611a2.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- no action on review comment regarding integer overflow on 32 bit long platforms (thus far)
- Link to v1: https://msgid.link/[email protected]

---
 net/qrtr/af_qrtr.c | 50 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 13 deletions(-)

net/qrtr/af_qrtr.c +37 ~~-13~~

index 1cb13242e41b..d6efd7f2eddf 100644
@@ -119,14 +119,15 @@ static DEFINE_XARRAY_ALLOC(qrtr_ports);
 
 /* The radix tree API uses fixed unsigned long keys and we will have to make
  * do with that.
- * These keys are often a combination of node IDs (currently u32) and
- * port numbers (also currently u32).
- * Using the high 32 bits for the node ID and the low 32 bits for the
- * port number will work fine to create keys on platforms where unsigned long
- * is 64 bits wide, but obviously is not be possible on platforms where
- * unsigned long is smaller.
+ * These keys are often a combination of node IDs and port numbers or
+ * endpoint IDs and node IDs (all currently u32).
+ * Using the high 32 bits for the node/endpoint ID and the low 32 bits for the
+ * port number/node ID will work fine to create keys on platforms where
+ * unsigned long is 64 bits wide, but obviously is not be possible on
+ * platforms where unsigned long is smaller.
  * Virtually split up unsigned long in half and assign the upper bits to
- * node IDs and the lower bits to the port number, however big that may be.
+ * node/endpoint IDs and the lower bits to the port number/node ID, however
+ * big that may be.
  */
 #define QRTR_INDEX_HALF_BITS (RADIX_TREE_INDEX_BITS >> 1)

@@ -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;
+
+	/* 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);
 
-	qrtr_node_assign(node, cb->src_node);
+	ret = qrtr_node_assign(node, cb->src_node);
+	if (ret)
+		goto err;
 
 	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) {

@@ -670,7 +692,9 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 	INIT_RADIX_TREE(&node->qrtr_tx_flow, GFP_KERNEL);
 	mutex_init(&node->qrtr_tx_lock);
 
-	qrtr_node_assign(node, nid);
+	rc = qrtr_node_assign(node, nid);
+	if (rc < 0)
+		goto free_node;
 
 	mutex_lock(&qrtr_node_lock);
 	list_add(&node->item, &qrtr_all_nodes);

Signature · 1 line

2.50.0

Jakub Kicinski

maintainer

2025-07-24 13:05 UTC #5 lore raw

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

net/qrtr/af_qrtr.c static struct qrtr_node *qrtr_node_lookup(unsigned int nid) · line 484

Quoted 6 lines · On Thu, 24 Jul 2025 01:24:01 +0200 Mihai Moldovan wrote:

net/qrtr/af_qrtr.c +4 -1

 	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;

missing unlock?

Signature · 1 line

pw-bot: cr

Simon Horman

reviewer

2025-07-24 13:08 UTC #6 lore raw

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

In reply to Mihai Moldovan

Quoted 16 lines · Mihai Moldovan

From: Denis Kenzior <[email protected]>
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 <[email protected]>
Reviewed-by Marcel Holtmann <[email protected]>
Reviewed-by Andy Gross <[email protected]>
Signed-off-by Mihai Moldovan <[email protected]>

...

Quoted 1 line

net/qrtr/af_qrtr.c +0 -0

...

Quoted 13 lines

net/qrtr/af_qrtr.c +4 -1

@@ -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;

net/qrtr/af_qrtr.c static struct qrtr_node *qrtr_node_lookup(unsigned int nid) · line 484

Quoted 10 lines

net/qrtr/af_qrtr.c +5 -2

 	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.

Quoted 18 lines

net/qrtr/af_qrtr.c +11 -0

+
+	/* 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;
 }
 
 /**

net/qrtr/af_qrtr.c int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len) · line 590

Quoted 2 lines

net/qrtr/af_qrtr.c +0 -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.

net/qrtr/af_qrtr.c int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len) · line 594

Quoted 5 lines

net/qrtr/af_qrtr.c +3 -1

-	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.

net/qrtr/af_qrtr.c int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len) · line 606

Quoted 13 lines

net/qrtr/af_qrtr.c +3 -1

 	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.

...

Mihai Moldovan

author

2025-07-27 13:09 UTC #7 lore raw

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

Quoted 1 line · * On 7/24/25 15:08, Simon Horman wrote:

[...]

Thank you for the reviews, to both you and Jakub.

Quoted 1 line

This will leak holding qrtr_nodes_lock.

It certainly does, will be fixed in v4.

Quoted 1 line

Flagged by Smatch.

I haven't used smatch before, and probably should do so going forward.

Curiously, a simple kchecker net/qrtr/ run did not warn about the locking issue (albeit it being obvious in the patch), while it did warn about the second issue with ret. Am I missing something?

Quoted 3 lines

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.

True. Will also fixed in v4.

Mihai

Simon Horman

reviewer

2025-07-27 14:40 UTC #8 lore raw

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

+ Dan Carpenter

Quoted 11 lines · Mihai Moldovan

Quoted 1 line · * On 7/24/25 15:08, Simon Horman wrote:
[...]
Thank you for the reviews, to both you and Jakub.
Quoted 1 line
This will leak holding qrtr_nodes_lock.
It certainly does, will be fixed in v4.

Quoted 7 lines

Quoted 1 line
Flagged by Smatch.
I haven't used smatch before, and probably should do so going forward.
Curiously, a simple kchecker net/qrtr/ run did not warn about the locking issue (albeit it being obvious in the patch), while it did warn about the second issue with ret. Am I missing something?

TL;DR: No, I seem to have been able to reproduce what you see.

I ran Smatch, compiled from a recent Git commit, like this:

kchecker net/qrtr/af_qrtr.o

The warnings I saw (new to this patch) are:

net/qrtr/af_qrtr.c:498 qrtr_node_assign() warn: inconsistent returns 'global &qrtr_nodes_lock'.
Locked on : 484
Unlocked on: 498
net/qrtr/af_qrtr.c:613 qrtr_endpoint_post() warn: missing error code 'ret'

That was with Smatch compiled from Git [1]
commit e1d933013098 ("return_efault: don't rely on the cross function DB")

I tried again with the latest head,
commit 2fb2b9093c5d ("sleep_info: The synchronize_srcu() sleeps"). And in that case I no longer see the 1st warning, about locking. I think this is what you saw too.

This seems to a regression in Smatch wrt this particular case for this code. I bisected Smatch and it looks like it was introduced in commit d0367cd8a993 ("ranges: use absolute instead implied for possibly_true/false")

I CCed Dan in case he wants to dig into this.

[1] https://repo.or.cz/smatch.git

Quoted 10 lines

Quoted 5 lines
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.
True. Will also fixed in v4.
Mihai

Mihai Moldovan

author

2025-07-27 17:34 UTC #9 lore raw

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

Quoted 4 lines · * On 7/27/25 16:40, Simon Horman wrote:

I tried again with the latest head,
commit 2fb2b9093c5d ("sleep_info: The synchronize_srcu() sleeps"). And in that case I no longer see the 1st warning, about locking. I think this is what you saw too.

Exactly! Together with impossible condition warnings, but those are actually fine/intended.

Quoted 3 lines

This seems to a regression in Smatch wrt this particular case for this code. I bisected Smatch and it looks like it was introduced in commit d0367cd8a993 ("ranges: use absolute instead implied for possibly_true/false")

Oh, thank you very much. I suspected that I'm just missing a special script or option or even addition to Smash (given that Dan seems to have revamped its locking check code in 2020), especially since it seems to be so widely used in kernel development, but not a bug in the software itself.

Mihai

Simon Horman

reviewer

2025-07-28 10:51 UTC #10 lore raw

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

Quoted 8 lines · Mihai Moldovan

Quoted 4 lines · * On 7/27/25 16:40, Simon Horman wrote:
I tried again with the latest head,
commit 2fb2b9093c5d ("sleep_info: The synchronize_srcu() sleeps"). And in that case I no longer see the 1st warning, about locking. I think this is what you saw too.
Exactly! Together with impossible condition warnings, but those are actually fine/intended.

Yeah, I saw them too.
I agree they are not correctness issues.

Quoted 8 lines

Quoted 4 lines
This seems to a regression in Smatch wrt this particular case for this code. I bisected Smatch and it looks like it was introduced in commit d0367cd8a993 ("ranges: use absolute instead implied for possibly_true/false")
Oh, thank you very much. I suspected that I'm just missing a special script or option or even addition to Smash (given that Dan seems to have revamped its locking check code in 2020), especially since it seems to be so widely used in kernel development, but not a bug in the software itself.

Likewise, thanks for pointing out this problem.

Dan Carpenter

2025-08-01 17:26 UTC #11 lore raw

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

In reply to Simon Horman

Quoted 43 lines · Simon Horman

+ Dan Carpenter
Quoted 11 lines · Mihai Moldovan
Quoted 1 line · * On 7/24/25 15:08, Simon Horman wrote:
[...]
Thank you for the reviews, to both you and Jakub.
Quoted 1 line
This will leak holding qrtr_nodes_lock.
It certainly does, will be fixed in v4.
Quoted 7 lines
Quoted 1 line
Flagged by Smatch.
I haven't used smatch before, and probably should do so going forward.
Curiously, a simple kchecker net/qrtr/ run did not warn about the locking issue (albeit it being obvious in the patch), while it did warn about the second issue with ret. Am I missing something?
TL;DR: No, I seem to have been able to reproduce what you see.
I ran Smatch, compiled from a recent Git commit, like this:
kchecker net/qrtr/af_qrtr.o
The warnings I saw (new to this patch) are:
net/qrtr/af_qrtr.c:498 qrtr_node_assign() warn: inconsistent returns 'global &qrtr_nodes_lock'.
Locked on : 484
Unlocked on: 498
net/qrtr/af_qrtr.c:613 qrtr_endpoint_post() warn: missing error code 'ret'
That was with Smatch compiled from Git [1]
commit e1d933013098 ("return_efault: don't rely on the cross function DB")
I tried again with the latest head,
commit 2fb2b9093c5d ("sleep_info: The synchronize_srcu() sleeps"). And in that case I no longer see the 1st warning, about locking. I think this is what you saw too.

Quoted 5 lines

This seems to a regression in Smatch wrt this particular case for this code. I bisected Smatch and it looks like it was introduced in commit d0367cd8a993 ("ranges: use absolute instead implied for possibly_true/false")
I CCed Dan in case he wants to dig into this.

The code looks like this:

	spin_lock_irqsave(&qrtr_nodes_lock, flags);

        if (node->ep->id > QRTR_INDEX_HALF_UNSIGNED_MAX ||
            nid > QRTR_INDEX_HALF_UNSIGNED_MAX)
                return -EINVAL;

The problem is that QRTR_INDEX_HALF_UNSIGNED_MAX is U32_MAX and node->ep->id and nid are both u32 type. The return statement is dead code and I deliberately silenced warnings on impossible paths.

The following patch will enable the warning again and I'll test it tonight to see what happens. If it's not too painful then I'll delete it properly, but if it's generates a bunch of false positives then, in the end, I'm not overly stressed about bugs in dead code.

regards,
dan carpenter

check_inconsistent_locking.c +4 -4

index f3cce559d7a6..e95d9110a1e1 100644
@@ -67,8 +67,8 @@ static void check_lock_bool(const char *name, struct symbol *sym)
 	FOR_EACH_PTR(get_all_return_strees(), stree) {
 		orig = __swap_cur_stree(stree);
 
-		if (is_impossible_path())
-			goto swap_stree;
+//		if (is_impossible_path())
+//			goto swap_stree;
 
 		return_sm = get_sm_state(RETURN_ID, "return_ranges", NULL);
 		if (!return_sm)

@@ -145,8 +145,8 @@ static void check_lock(const char *name, struct symbol *sym)
 	FOR_EACH_PTR(get_all_return_strees(), stree) {
 		orig = __swap_cur_stree(stree);
 
-		if (is_impossible_path())
-			goto swap_stree;
+//		if (is_impossible_path())
+//			goto swap_stree;
 
 		return_sm = get_sm_state(RETURN_ID, "return_ranges", NULL);
 		if (!return_sm)

Simon Horman

reviewer

2025-08-04 09:55 UTC #12 lore raw

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

Quoted 5 lines · Dan Carpenter

Quoted 4 lines · Simon Horman
+ Dan Carpenter
Quoted 1 line · Mihai Moldovan
* On 7/24/25 15:08, Simon Horman wrote:

...

Quoted 14 lines

Quoted 5 lines
This seems to a regression in Smatch wrt this particular case for this code. I bisected Smatch and it looks like it was introduced in commit d0367cd8a993 ("ranges: use absolute instead implied for possibly_true/false")
I CCed Dan in case he wants to dig into this.
The code looks like this:
	spin_lock_irqsave(&qrtr_nodes_lock, flags);

        if (node->ep->id > QRTR_INDEX_HALF_UNSIGNED_MAX ||
            nid > QRTR_INDEX_HALF_UNSIGNED_MAX)
                return -EINVAL;

Quoted 8 lines

The problem is that QRTR_INDEX_HALF_UNSIGNED_MAX is U32_MAX and node->ep->id and nid are both u32 type. The return statement is dead code and I deliberately silenced warnings on impossible paths.
The following patch will enable the warning again and I'll test it tonight to see what happens. If it's not too painful then I'll delete it properly, but if it's generates a bunch of false positives then, in the end, I'm not overly stressed about bugs in dead code.

Thanks Dan,

I think the key point here is that neither Mihai nor I noticed the dead code. Thanks for pointing that out.

...

Dan Carpenter

2025-08-04 10:19 UTC #13 lore raw

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

Quoted 32 lines · Simon Horman

Quoted 5 lines · Dan Carpenter
Quoted 4 lines · Simon Horman
+ Dan Carpenter
Quoted 1 line · Mihai Moldovan
* On 7/24/25 15:08, Simon Horman wrote:
...
Quoted 14 lines
Quoted 5 lines
This seems to a regression in Smatch wrt this particular case for this code. I bisected Smatch and it looks like it was introduced in commit d0367cd8a993 ("ranges: use absolute instead implied for possibly_true/false")
I CCed Dan in case he wants to dig into this.
The code looks like this:
	spin_lock_irqsave(&qrtr_nodes_lock, flags);

        if (node->ep->id > QRTR_INDEX_HALF_UNSIGNED_MAX ||
            nid > QRTR_INDEX_HALF_UNSIGNED_MAX)
                return -EINVAL;
Quoted 8 lines
The problem is that QRTR_INDEX_HALF_UNSIGNED_MAX is U32_MAX and node->ep->id and nid are both u32 type. The return statement is dead code and I deliberately silenced warnings on impossible paths.
The following patch will enable the warning again and I'll test it tonight to see what happens. If it's not too painful then I'll delete it properly, but if it's generates a bunch of false positives then, in the end, I'm not overly stressed about bugs in dead code.

Quoted 5 lines

Thanks Dan,
I think the key point here is that neither Mihai nor I noticed the dead code. Thanks for pointing that out.

I did test this over the weekend, btw. Warning about bugs in dead code does find some "real" bug and they might actually be real depending on config. But it also triggers a bunch of false positives which are hard to solve:

	r = dma_resv_lock(&resv, NULL);
	if (r)
		r;

The dma_resv_lock() can't fail it you pass NULL as a parameter, so Smatch says "every return takes the lock", and then "if we fail, we return without dropping the lock." It's difficult to solve this.

I guess we would have to say "If we're in an impossible return and it was the locking function which failed then we didn't take the lock." That would work, but it's sort of a tricky rule to code.

regards,
dan carpenter

Mihai Moldovan

author

2025-07-23 23:24 UTC #14 lore raw

PATCH v3 5/11 net: qrtr: Report sender endpoint in aux data

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

Introduce support for reporting the remote endpoint that generated a given QRTR message to clients using AF_QIPCRTR family sockets. This is achieved by including QRTR_ENDPOINT auxiliary data, which carries the endpoint identifier of the message sender. To receive this auxiliary data, clients must explicitly opt-in by using setsockopt with the QRTR_REPORT_ENDPOINT option enabled.

Implementation of getsockopt and setsockopt is provided. An additional level 'SOL_QRTR' is added to socket.h for use by AF_QIPCRTR family sockets.

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- fix checkpatch.pl whitespace warning
- Link to v2: https://msgid.link/8dc0c9f99f617db348af5b176bb03b4c11ac9138.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- dropped socket locking in qrtr_setsockopt() and qrtr_getsockopt() as per review comment
- Link to v1: https://msgid.link/[email protected]

---
 include/linux/socket.h    |  1 +
 include/uapi/linux/qrtr.h |  6 +++
 net/qrtr/af_qrtr.c        | 77 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+)

include/linux/socket.h +1 -0

index 3b262487ec06..0698a11bb2e2 100644
@@ -386,6 +386,7 @@ struct ucred {
 #define SOL_MCTP	285
 #define SOL_SMC		286
 #define SOL_VSOCK	287
+#define SOL_QRTR	288
 
 /* IPX options */
 #define IPX_TYPE	1

include/uapi/linux/qrtr.h +6 -0

index f7e2fb3d752b..6d0911984a05 100644
@@ -46,4 +46,10 @@ struct qrtr_ctrl_pkt {
 	};
 } __packed;
 
+/* setsockopt / getsockopt */
+#define QRTR_REPORT_ENDPOINT 1
+
+/* CMSG */
+#define QRTR_ENDPOINT 1
+
 #endif /* _LINUX_QRTR_H */

net/qrtr/af_qrtr.c +77 -0

125 lines not on this page · raw message

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #15 lore raw

PATCH v3 6/11 net: qrtr: Report endpoint for locally generated messages

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

For messages generated by the local endpoint destined
to the local endpoint, report the local endpoint identifier. Same QRTR_ENDPOINT auxiliary data and QRTR_REPORT_ENDPOINT socket option semantics apply as for messages generated by remote endpoints.

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- fix checkpatch.pl function definition warnings
- Link to v2: https://msgid.link/5b08be230cd2e5faf24297ee04beb7616861fe5b.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- Link to v1: https://msgid.link/[email protected]

---
 net/qrtr/af_qrtr.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

net/qrtr/af_qrtr.c +20 -9

index 0d21acb465ee..68f899b16e95 100644
@@ -184,9 +184,11 @@ struct qrtr_tx_flow {
 #define QRTR_TX_FLOW_LOW	5
 
 static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb,
+			      u32 endpoint_id,
 			      int type, struct sockaddr_qrtr *from,
 			      struct sockaddr_qrtr *to);
 static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
+			      u32 endpoint_id,
 			      int type, struct sockaddr_qrtr *from,
 			      struct sockaddr_qrtr *to);
 static struct qrtr_sock *qrtr_port_lookup(int port);

@@ -401,6 +403,7 @@ static int qrtr_tx_flow_failed(struct qrtr_node *node, int dest_node,
 
 /* Pass an outgoing packet socket buffer to the endpoint driver. */
 static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
+			     u32 endpoint_id,
 			     int type, struct sockaddr_qrtr *from,
 			     struct sockaddr_qrtr *to)
 {

@@ -748,7 +751,8 @@ void qrtr_endpoint_unregister(struct qrtr_endpoint *ep)
 		skb = qrtr_alloc_ctrl_packet(&pkt, GFP_ATOMIC);
 		if (skb) {
 			pkt->cmd = cpu_to_le32(QRTR_TYPE_BYE);
-			qrtr_local_enqueue(NULL, skb, QRTR_TYPE_BYE, &src, &dst);
+			qrtr_local_enqueue(NULL, skb, endpoint_id,
+					   QRTR_TYPE_BYE, &src, &dst);
 		}
 	}
 	spin_unlock_irqrestore(&qrtr_nodes_lock, flags);

@@ -815,8 +819,8 @@ static void qrtr_port_remove(struct qrtr_sock *ipc)
 		pkt->client.port = cpu_to_le32(ipc->us.sq_port);
 
 		skb_set_owner_w(skb, &ipc->sk);
-		qrtr_bcast_enqueue(NULL, skb, QRTR_TYPE_DEL_CLIENT, &ipc->us,
-				   &to);
+		qrtr_bcast_enqueue(NULL, skb, qrtr_local_nid,
+				   QRTR_TYPE_DEL_CLIENT, &ipc->us, &to);
 	}
 
 	if (port == QRTR_PORT_CTRL)

@@ -956,6 +960,7 @@ static int qrtr_bind(struct socket *sock, struct sockaddr *saddr, int len)
 
 /* Queue packet to local peer socket. */
 static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb,
+			      u32 endpoint_id,
 			      int type, struct sockaddr_qrtr *from,
 			      struct sockaddr_qrtr *to)
 {

@@ -973,6 +978,7 @@ static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	cb = (struct qrtr_cb *)skb->cb;
 	cb->src_node = from->sq_node;
 	cb->src_port = from->sq_port;
+	cb->endpoint_id = endpoint_id;
 
 	if (sock_queue_rcv_skb(&ipc->sk, skb)) {
 		qrtr_port_put(ipc);

@@ -987,6 +993,7 @@ static int qrtr_local_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 
 /* Queue packet for broadcast. */
 static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
+			      u32 endpoint_id,
 			      int type, struct sockaddr_qrtr *from,
 			      struct sockaddr_qrtr *to)
 {

@@ -998,11 +1005,11 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 		if (!skbn)
 			break;
 		skb_set_owner_w(skbn, skb->sk);
-		qrtr_node_enqueue(node, skbn, type, from, to);
+		qrtr_node_enqueue(node, skbn, endpoint_id, type, from, to);
 	}
 	mutex_unlock(&qrtr_node_lock);
 
-	qrtr_local_enqueue(NULL, skb, type, from, to);
+	qrtr_local_enqueue(NULL, skb, endpoint_id, type, from, to);
 
 	return 0;
 }

@@ -1010,12 +1017,15 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 {
 	DECLARE_SOCKADDR(struct sockaddr_qrtr *, addr, msg->msg_name);
-	int (*enqueue_fn)(struct qrtr_node *, struct sk_buff *, int,
-			  struct sockaddr_qrtr *, struct sockaddr_qrtr *);
+	int (*enqueue_fn)(struct qrtr_node *node, struct sk_buff *skb,
+			  u32 endpoint_id, int type,
+			  struct sockaddr_qrtr *from,
+			  struct sockaddr_qrtr *to);
 	__le32 qrtr_type = cpu_to_le32(QRTR_TYPE_DATA);
 	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
 	struct sock *sk = sock->sk;
 	struct qrtr_node *node;
+	u32 endpoint_id = qrtr_local_nid;
 	struct sk_buff *skb;
 	size_t plen;
 	u32 type;

@@ -1099,7 +1109,7 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	}
 
 	type = le32_to_cpu(qrtr_type);
-	rc = enqueue_fn(node, skb, type, &ipc->us, addr);
+	rc = enqueue_fn(node, skb, endpoint_id, type, &ipc->us, addr);
 	if (rc >= 0)
 		rc = len;

@@ -1131,7 +1141,8 @@ static int qrtr_send_resume_tx(struct qrtr_cb *cb)
 	pkt->client.node = cpu_to_le32(cb->dst_node);
 	pkt->client.port = cpu_to_le32(cb->dst_port);
 
-	ret = qrtr_node_enqueue(node, skb, QRTR_TYPE_RESUME_TX, &local, &remote);
+	ret = qrtr_node_enqueue(node, skb, cb->endpoint_id,
+				QRTR_TYPE_RESUME_TX, &local, &remote);
 
 	qrtr_node_release(node);

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #16 lore raw

PATCH v3 7/11 net: qrtr: Allow sendmsg to target an endpoint

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

Allow QIPCRTR family sockets to include QRTR_ENDPOINT auxiliary data as part of the sendmsg system call. By including this parameter, the client can ask the kernel to route the message to a given endpoint, in situations where multiple endpoints with conflicting node identifier sets exist in the system.

For legacy clients, or clients that do not include QRTR_ENDPOINT data, the endpoint is looked up, as before, by only using the node identifier of the destination qrtr socket address.

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- port [endpoint ID|node ID] key usage in qrtr_node_lookup() to the generic
solution already established for the [node ID|port number] usage
- Link to v2: https://msgid.link/fc510e5f0bae7e2d2cc5c0349ee7c166840b9154.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- no action on review comment regarding initializing out_endpoint_id, since that's rightfully already being done
- Link to v1: https://msgid.link/[email protected]

---
 net/qrtr/af_qrtr.c | 89 ++++++++++++++++++++++++++++++++++------------
 net/qrtr/qrtr.h    |  2 ++
 2 files changed, 68 insertions(+), 23 deletions(-)

net/qrtr/af_qrtr.c +66 ~~-23~~

index 68f899b16e95..cd311ac045c2 100644
@@ -106,6 +106,36 @@ static inline struct qrtr_sock *qrtr_sk(struct sock *sk)
 	return container_of(sk, struct qrtr_sock, sk);
 }
 
+int qrtr_msg_get_endpoint(struct msghdr *msg, u32 *out_endpoint_id)
+{
+	struct cmsghdr *cmsg;
+	u32 endpoint_id = 0;
+
+	for_each_cmsghdr(cmsg, msg) {
+		if (!CMSG_OK(msg, cmsg))
+			return -EINVAL;
+
+		if (cmsg->cmsg_level != SOL_QRTR)
+			continue;
+
+		if (cmsg->cmsg_type != QRTR_ENDPOINT)
+			return -EINVAL;
+
+		if (cmsg->cmsg_len < CMSG_LEN(sizeof(u32)))
+			return -EINVAL;
+
+		/* Endpoint ids start at 1 */
+		endpoint_id = *(u32 *)CMSG_DATA(cmsg);
+		if (!endpoint_id)
+			return -EINVAL;
+	}
+
+	if (out_endpoint_id)
+		*out_endpoint_id = endpoint_id;
+
+	return 0;
+}
+
 static unsigned int qrtr_local_nid = 1;
 
 /* for node ids */

@@ -456,14 +486,23 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
  *
  * callers must release with qrtr_node_release()
  */
-static struct qrtr_node *qrtr_node_lookup(unsigned int nid)
+static struct qrtr_node *qrtr_node_lookup(unsigned int endpoint_id,
+					  unsigned int nid)
 {
-	struct qrtr_node *node;
+	struct qrtr_node *node = NULL;
 	unsigned long flags;
+	unsigned long key = 0;
+
+	if (endpoint_id > QRTR_INDEX_HALF_UNSIGNED_MAX ||
+	    nid > QRTR_INDEX_HALF_UNSIGNED_MAX)
+		return node;
+
+	key = ((unsigned long)(endpoint_id) << QRTR_INDEX_HALF_BITS) |
+	      ((unsigned long)(nid) & QRTR_INDEX_HALF_UNSIGNED_MAX);
 
 	mutex_lock(&qrtr_node_lock);
 	spin_lock_irqsave(&qrtr_nodes_lock, flags);
-	node = radix_tree_lookup(&qrtr_nodes, nid);
+	node = radix_tree_lookup(&qrtr_nodes, key);
 	node = qrtr_node_acquire(node);
 	spin_unlock_irqrestore(&qrtr_nodes_lock, flags);
 	mutex_unlock(&qrtr_node_lock);

@@ -1025,6 +1064,7 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
 	struct sock *sk = sock->sk;
 	struct qrtr_node *node;
+	u32 msg_endpoint_id;
 	u32 endpoint_id = qrtr_local_nid;
 	struct sk_buff *skb;
 	size_t plen;

@@ -1037,46 +1077,48 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	if (len > 65535)
 		return -EMSGSIZE;
 
+	rc = qrtr_msg_get_endpoint(msg, &msg_endpoint_id);
+	if (rc < 0)
+		return rc;
+
 	lock_sock(sk);
 
 	if (addr) {
-		if (msg->msg_namelen < sizeof(*addr)) {
-			release_sock(sk);
-			return -EINVAL;
-		}
+		rc = -EINVAL;
 
-		if (addr->sq_family != AF_QIPCRTR) {
-			release_sock(sk);
-			return -EINVAL;
-		}
+		if (msg->msg_namelen < sizeof(*addr))
+			goto release_sock;
+
+		if (addr->sq_family != AF_QIPCRTR)
+			goto release_sock;
 
 		rc = qrtr_autobind(sock);
-		if (rc) {
-			release_sock(sk);
-			return rc;
-		}
+		if (rc)
+			goto release_sock;
 	} else if (sk->sk_state == TCP_ESTABLISHED) {
 		addr = &ipc->peer;
 	} else {
-		release_sock(sk);
-		return -ENOTCONN;
+		rc = -ENOTCONN;
+		goto release_sock;
 	}
 
 	node = NULL;
 	if (addr->sq_node == QRTR_NODE_BCAST) {
 		if (addr->sq_port != QRTR_PORT_CTRL &&
 		    qrtr_local_nid != QRTR_NODE_BCAST) {
-			release_sock(sk);
-			return -ENOTCONN;
+			rc = -ENOTCONN;
+			goto release_sock;
 		}
 		enqueue_fn = qrtr_bcast_enqueue;
 	} else if (addr->sq_node == ipc->us.sq_node) {
 		enqueue_fn = qrtr_local_enqueue;
 	} else {
-		node = qrtr_node_lookup(addr->sq_node);
+		endpoint_id = msg_endpoint_id;
+
+		node = qrtr_node_lookup(endpoint_id, addr->sq_node);
 		if (!node) {
-			release_sock(sk);
-			return -ECONNRESET;
+			rc = endpoint_id ? -ENXIO : -ECONNRESET;
+			goto release_sock;
 		}
 		enqueue_fn = qrtr_node_enqueue;
 	}

@@ -1115,6 +1157,7 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 
 out_node:
 	qrtr_node_release(node);
+release_sock:
 	release_sock(sk);
 
 	return rc;

@@ -1129,7 +1172,7 @@ static int qrtr_send_resume_tx(struct qrtr_cb *cb)
 	struct sk_buff *skb;
 	int ret;
 
-	node = qrtr_node_lookup(remote.sq_node);
+	node = qrtr_node_lookup(cb->endpoint_id, remote.sq_node);
 	if (!node)
 		return -EINVAL;

net/qrtr/qrtr.h +2 -0

index 11b897af05e6..22fcecbf8de2 100644
@@ -34,4 +34,6 @@ int qrtr_ns_init(void);
 
 void qrtr_ns_remove(void);
 
+int qrtr_msg_get_endpoint(struct msghdr *msg, u32 *out_endpoint_id);
+
 #endif

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #17 lore raw

PATCH v3 8/11 net: qrtr: allow socket endpoint binding

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

Introduce the ability to bind a QIPCRTR family socket to a specific endpoint. When a socket is bound, only messages from the bound endpoint can be received, and any messages sent from the socket are by default directed to the bound endpoint. Clients can bind a socket by using the setsockopt system call with the QRTR_BIND_ENDPOINT option set to the desired endpoint binding.

A previously set binding can be reset by setting QRTR_BIND_ENDPOINT option to zero. This behavior matches that of SO_BINDTOIFINDEX.

This functionality is useful for clients that need to communicate with a specific device (i.e. endpoint), such as a PCIe-based 5G modem, and are not interested in messages from other endpoints / nodes.

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- Link to v2: https://msgid.link/c914eae5bd8d4a3924cc3c00c1dd5810024678f5.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- use WRITE_ONCE() to write value in qrtr_setsockopt() and READ_ONCE() to read it in qrtr_getsockopt() as per review comment
- Link to v1: https://msgid.link/[email protected]

---
 include/uapi/linux/qrtr.h |  1 +
 net/qrtr/af_qrtr.c        | 54 ++++++++++++++++++++++++++++-----------
 2 files changed, 40 insertions(+), 15 deletions(-)

include/uapi/linux/qrtr.h +1 -0

index 6d0911984a05..0a8667b049c3 100644
@@ -48,6 +48,7 @@ struct qrtr_ctrl_pkt {
 
 /* setsockopt / getsockopt */
 #define QRTR_REPORT_ENDPOINT 1
+#define QRTR_BIND_ENDPOINT 2
 
 /* CMSG */
 #define QRTR_ENDPOINT 1

net/qrtr/af_qrtr.c +39 ~~-15~~

102 lines not on this page · raw message

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #18 lore raw

PATCH v3 9/11 net: qrtr: Drop remote {NEW|DEL}_LOOKUP messages

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

These messages are explicitly filtered out by the in-kernel name service (ns.c). Filter them out even earlier to save some CPU cycles.

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- Link to v2: https://msgid.link/ad089e97706b095063777f1eefe04d75cbb917f1.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- Link to v1: https://msgid.link/[email protected]

---
 net/qrtr/af_qrtr.c | 5 +++++
 1 file changed, 5 insertions(+)

net/qrtr/af_qrtr.c +5 -0

index fa22a27ec5d2..e0123bbe0bb5 100644
@@ -629,6 +629,11 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 	if (!size || len != ALIGN(size, 4) + hdrlen)
 		goto err;
 
+	/* Don't allow remote lookups */
+	if (cb->type == QRTR_TYPE_NEW_LOOKUP ||
+	    cb->type == QRTR_TYPE_DEL_LOOKUP)
+		goto err;
+
 	if ((cb->type == QRTR_TYPE_NEW_SERVER ||
 	     cb->type == QRTR_TYPE_RESUME_TX) &&
 	    size < sizeof(struct qrtr_ctrl_pkt))

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #19 lore raw

PATCH v3 10/11 net: qrtr: ns: support multiple endpoints

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

Extend the qrtr name service with the concept of an endpoint. Endpoints can be remote or local, and are represented by a unique endpoint id. This allows the qrtr name service to support multiple devices (endpoints) that might have overlapping node / port combinations.

The socket used by the name service is subscribed to receive endpoint information via a mechanism similar to the SOL_QRTR QRTR_REPORT_ENDPOINT socket option. Internal data structures are then extended to track endpoint information in addition to nodes and ports.

The name service directs packets to the endpoint originating the request. For NEW_SERVER and DEL_SERVER messages triggered using lookups or due to remote endpoints sending the corresponding message, qrtr name service generates a NEW_SERVER and DEL_SERVER messages to all local sockets registered to receive such notifications. The messages are made to look as if they're coming from the remote endpoint using a special extension to AF_QRTR sendmsg operation. This extension only works for the local socket that owns the QRTR_PORT_CTRL port (name service).

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- Link to v2: https://msgid.link/0781b3d2293d05616f18b03439a08ae8612b2dbb.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- drop socket locking from qrtr_sock_set_report_endpoint() as per review comment
- Link to v1: https://msgid.link/[email protected]

---
 net/qrtr/af_qrtr.c |  12 ++
 net/qrtr/ns.c      | 296 +++++++++++++++++++++++++++------------------
 net/qrtr/qrtr.h    |   1 +
 3 files changed, 191 insertions(+), 118 deletions(-)

net/qrtr/af_qrtr.c +12 -0

index e0123bbe0bb5..4f6dbc7420aa 100644
@@ -107,6 +107,13 @@ static inline struct qrtr_sock *qrtr_sk(struct sock *sk)
 	return container_of(sk, struct qrtr_sock, sk);
 }
 
+void qrtr_sock_set_report_endpoint(struct sock *sk)
+{
+	struct qrtr_sock *ipc = qrtr_sk(sk);
+
+	assign_bit(QRTR_F_REPORT_ENDPOINT, &ipc->flags, 1);
+}
+
 int qrtr_msg_get_endpoint(struct msghdr *msg, u32 *out_endpoint_id)
 {
 	struct cmsghdr *cmsg;

@@ -1171,6 +1178,11 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 
 		/* control messages already require the type as 'command' */
 		skb_copy_bits(skb, 0, &qrtr_type, 4);
+		/*
+		 * Allow local name service to make packets appear as if
+		 * they originated remotely
+		 */
+		endpoint_id = msg_endpoint_id;
 	}
 
 	type = le32_to_cpu(qrtr_type);

net/qrtr/ns.c +178 ~~-118~~

632 lines not on this page · raw message

net/qrtr/qrtr.h +1 -0

index 22fcecbf8de2..b4f50336ae75 100644
@@ -35,5 +35,6 @@ int qrtr_ns_init(void);
 void qrtr_ns_remove(void);
 
 int qrtr_msg_get_endpoint(struct msghdr *msg, u32 *out_endpoint_id);
+void qrtr_sock_set_report_endpoint(struct sock *sk);
 
 #endif

Signature · 1 line

2.50.0

Mihai Moldovan

author

2025-07-23 23:24 UTC #20 lore raw

PATCH v3 11/11 net: qrtr: mhi: Report endpoint id in sysfs

In reply to Mihai Moldovan

From: Denis Kenzior <[email protected]>

Add a read-only 'endpoint' sysfs entry that contains the qrtr endpoint identifier assigned to this mhi device. Can be used to direct / receive qrtr traffic only from a particular MHI device.

Signed-off-by Denis Kenzior <[email protected]>

Reviewed-by Marcel Holtmann <[email protected]>

Reviewed-by Andy Gross <[email protected]>

Signed-off-by Mihai Moldovan <[email protected]>

---

v3:
- rebase against current master
- Link to v2: https://msgid.link/1a49dec96d5c2c5258c9df935d8c9381793d4ddd.1752947108.git.ionic@ionic.de

v2:
- rebase against current master
- use %u formatter instead of %d when printing endpoint id (u32) as per review comment
- Link to v1: https://msgid.link/[email protected]

---
 net/qrtr/mhi.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

net/qrtr/mhi.c +14 -0

index 69f53625a049..9a23c888e234 100644
@@ -72,6 +72,16 @@ static int qcom_mhi_qrtr_send(struct qrtr_endpoint *ep, struct sk_buff *skb)
 	return rc;
 }
 
+static ssize_t endpoint_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct qrtr_mhi_dev *qdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", qdev->ep.id);
+}
+
+static DEVICE_ATTR_RO(endpoint);
+
 static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
 			       const struct mhi_device_id *id)
 {

@@ -91,6 +101,9 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev,
 	if (rc)
 		return rc;
 
+	if (device_create_file(&mhi_dev->dev, &dev_attr_endpoint) < 0)
+		dev_err(qdev->dev, "Failed to create endpoint attribute\n");
+
 	/* start channels */
 	rc = mhi_prepare_for_transfer_autoqueue(mhi_dev);
 	if (rc) {

@@ -107,6 +120,7 @@ static void qcom_mhi_qrtr_remove(struct mhi_device *mhi_dev)
 {
 	struct qrtr_mhi_dev *qdev = dev_get_drvdata(&mhi_dev->dev);
 
+	device_remove_file(&mhi_dev->dev, &dev_attr_endpoint);
 	qrtr_endpoint_unregister(&qdev->ep);
 	mhi_unprepare_from_transfer(mhi_dev);
 	dev_set_drvdata(&mhi_dev->dev, NULL);

Signature · 1 line

2.50.0