Re: [PATCH v3 03/11] net: qrtr: fit node ID + port number combination into unsigned long

Re: [PATCH v3 03/11] net: qrtr: fit node ID + port number combination into unsigned long

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <c60cc5f238873f72ef6f49582fb87ae7122853d5.1753312999.git.ionic@ionic.de>
References: <c60cc5f238873f72ef6f49582fb87ae7122853d5.1753312999.git.ionic@ionic.de>
TO: Mihai Moldovan <ionic@ionic.de>
TO: linux-arm-msm@vger.kernel.org
TO: Manivannan Sadhasivam <mani@kernel.org>
CC: Eric Dumazet <edumazet@google.com>
CC: Kuniyuki Iwashima <kuniyu@google.com>
CC: Paolo Abeni <pabeni@redhat.com>
CC: Willem de Bruijn <willemb@google.com>
CC: "David S . Miller" <davem@davemloft.net>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Simon Horman <horms@kernel.org>
CC: linux-kernel@vger.kernel.org
CC: netdev@vger.kernel.org

Hi Mihai,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mani-mhi/mhi-next]
[also build test WARNING on net-next/main net/main linus/master v6.16-rc7 next-20250725]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mihai-Moldovan/net-qrtr-ns-validate-msglen-before-ctrl_pkt-use/20250724-073030
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git mhi-next
patch link:    https://lore.kernel.org/r/c60cc5f238873f72ef6f49582fb87ae7122853d5.1753312999.git.ionic%40ionic.de
patch subject: [PATCH v3 03/11] net: qrtr: fit node ID + port number combination into unsigned long
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: x86_64-randconfig-161-20250726 (https://download.01.org/0day-ci/archive/20250727/202507270040.z4mru6Go-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202507270040.z4mru6Go-lkp@intel.com/

smatch warnings:
net/qrtr/af_qrtr.c:301 qrtr_tx_wait() warn: impossible condition '(dest_node < ((-1) - ((((~(0)) >> ((8 * 8) >> 1))) >> 1))) => (s32min-s32max < s32min)'
net/qrtr/af_qrtr.c:303 qrtr_tx_wait() warn: impossible condition '(dest_port < ((-1) - ((((~(0)) >> ((8 * 8) >> 1))) >> 1))) => (s32min-s32max < s32min)'
net/qrtr/af_qrtr.c:374 qrtr_tx_flow_failed() warn: impossible condition '(dest_node < ((-1) - ((((~(0)) >> ((8 * 8) >> 1))) >> 1))) => (s32min-s32max < s32min)'
net/qrtr/af_qrtr.c:376 qrtr_tx_flow_failed() warn: impossible condition '(dest_port < ((-1) - ((((~(0)) >> ((8 * 8) >> 1))) >> 1))) => (s32min-s32max < s32min)'

vim +301 net/qrtr/af_qrtr.c

5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  277  
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  278  /**
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  279   * qrtr_tx_wait() - flow control for outgoing packets
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  280   * @node:	qrtr_node that the packet is to be send to
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  281   * @dest_node:	node id of the destination
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  282   * @dest_port:	port number of the destination
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  283   * @type:	type of message
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  284   *
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  285   * The flow control scheme is based around the low and high "watermarks". When
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  286   * the low watermark is passed the confirm_rx flag is set on the outgoing
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  287   * message, which will trigger the remote to send a control message of the type
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  288   * QRTR_TYPE_RESUME_TX to reset the counter. If the high watermark is hit
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  289   * further transmision should be paused.
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  290   *
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  291   * Return: 1 if confirm_rx should be set, 0 otherwise or errno failure
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  292   */
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  293  static int qrtr_tx_wait(struct qrtr_node *node, int dest_node, int dest_port,
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  294  			int type)
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  295  {
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  296  	unsigned long key = 0;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  297  	struct qrtr_tx_flow *flow;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  298  	int confirm_rx = 0;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  299  	int ret;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  300  
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24 @301  	if (dest_node < QRTR_INDEX_HALF_SIGNED_MIN ||
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  302  	    dest_node > QRTR_INDEX_HALF_SIGNED_MAX ||
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24 @303  	    dest_port < QRTR_INDEX_HALF_SIGNED_MIN ||
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  304  	    dest_port > QRTR_INDEX_HALF_SIGNED_MAX)
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  305  		return -EINVAL;
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  306  
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  307  	key = ((unsigned long)(dest_node) << QRTR_INDEX_HALF_BITS) |
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  308  	      ((unsigned long)(dest_port) & QRTR_INDEX_HALF_UNSIGNED_MAX);
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  309  
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  310  	/* Never set confirm_rx on non-data packets */
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  311  	if (type != QRTR_TYPE_DATA)
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  312  		return 0;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  313  
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  314  	mutex_lock(&node->qrtr_tx_lock);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  315  	flow = radix_tree_lookup(&node->qrtr_tx_flow, key);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  316  	if (!flow) {
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  317  		flow = kzalloc(sizeof(*flow), GFP_KERNEL);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  318  		if (flow) {
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  319  			init_waitqueue_head(&flow->resume_tx);
8a03dd925786bd net/qrtr/qrtr.c    Loic Poulain    2021-03-30  320  			if (radix_tree_insert(&node->qrtr_tx_flow, key, flow)) {
8a03dd925786bd net/qrtr/qrtr.c    Loic Poulain    2021-03-30  321  				kfree(flow);
8a03dd925786bd net/qrtr/qrtr.c    Loic Poulain    2021-03-30  322  				flow = NULL;
8a03dd925786bd net/qrtr/qrtr.c    Loic Poulain    2021-03-30  323  			}
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  324  		}
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  325  	}
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  326  	mutex_unlock(&node->qrtr_tx_lock);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  327  
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  328  	/* Set confirm_rx if we where unable to find and allocate a flow */
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  329  	if (!flow)
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  330  		return 1;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  331  
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  332  	spin_lock_irq(&flow->resume_tx.lock);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  333  	ret = wait_event_interruptible_locked_irq(flow->resume_tx,
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  334  						  flow->pending < QRTR_TX_FLOW_HIGH ||
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  335  						  flow->tx_failed ||
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  336  						  !node->ep);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  337  	if (ret < 0) {
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  338  		confirm_rx = ret;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  339  	} else if (!node->ep) {
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  340  		confirm_rx = -EPIPE;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  341  	} else if (flow->tx_failed) {
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  342  		flow->tx_failed = 0;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  343  		confirm_rx = 1;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  344  	} else {
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  345  		flow->pending++;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  346  		confirm_rx = flow->pending == QRTR_TX_FLOW_LOW;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  347  	}
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  348  	spin_unlock_irq(&flow->resume_tx.lock);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  349  
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  350  	return confirm_rx;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  351  }
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  352  
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  353  /**
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  354   * qrtr_tx_flow_failed() - flag that tx of confirm_rx flagged messages failed
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  355   * @node:	qrtr_node that the packet is to be send to
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  356   * @dest_node:	node id of the destination
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  357   * @dest_port:	port number of the destination
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  358   *
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  359   * Signal that the transmission of a message with confirm_rx flag failed. The
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  360   * flow's "pending" counter will keep incrementing towards QRTR_TX_FLOW_HIGH,
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  361   * at which point transmission would stall forever waiting for the resume TX
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  362   * message associated with the dropped confirm_rx message.
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  363   * Work around this by marking the flow as having a failed transmission and
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  364   * cause the next transmission attempt to be sent with the confirm_rx.
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  365   *
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  366   * Return: 0 on success; negative error code on failure
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  367   */
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  368  static int qrtr_tx_flow_failed(struct qrtr_node *node, int dest_node,
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  369  			       int dest_port)
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  370  {
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  371  	unsigned long key = 0;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  372  	struct qrtr_tx_flow *flow;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  373  
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24 @374  	if (dest_node < QRTR_INDEX_HALF_SIGNED_MIN ||
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  375  	    dest_node > QRTR_INDEX_HALF_SIGNED_MAX ||
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24 @376  	    dest_port < QRTR_INDEX_HALF_SIGNED_MIN ||
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  377  	    dest_port > QRTR_INDEX_HALF_SIGNED_MAX)
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  378  		return -EINVAL;
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  379  
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  380  	key = ((unsigned long)(dest_node) << QRTR_INDEX_HALF_BITS) |
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  381  	      ((unsigned long)(dest_port) & QRTR_INDEX_HALF_UNSIGNED_MAX);
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  382  
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  383  	rcu_read_lock();
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  384  	flow = radix_tree_lookup(&node->qrtr_tx_flow, key);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  385  	rcu_read_unlock();
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  386  	if (flow) {
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  387  		spin_lock_irq(&flow->resume_tx.lock);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  388  		flow->tx_failed = 1;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  389  		spin_unlock_irq(&flow->resume_tx.lock);
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  390  	}
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  391  
e92239648a9190 net/qrtr/af_qrtr.c Mihai Moldovan  2025-07-24  392  	return 0;
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  393  }
5fdeb0d372ab33 net/qrtr/qrtr.c    Bjorn Andersson 2020-01-13  394  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 00/11] QRTR Multi-endpoint support

[Mihai Moldovan]

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/cover.1752947108.git.ionic@ionic.de

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/20241018181842.1368394-1-denkenz@gmail.com

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(-)

-- 
2.50.0

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

[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 <ionic@ionic.de>
Fixes: 5fdeb0d372ab ("net: qrtr: Implement outgoing flow control")

---

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

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index be275871fb2a..1cb13242e41b 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -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);
-- 
2.50.0