[PATCH V1 0/4] General qrtr fixes

[PATCH V1 0/4] General qrtr fixes

[Deepak Kumar Singh]

Arun Kumar Neelakantam (1):
  net: qrtr: Check function pointer before calling

Chris Lew (3):
  net: qrtr: Do not send packets before hello negotiation
  net: qrtr: Add socket mode optimization
  net: qrtr: Change port allocation to use cyclic idr

 net/qrtr/qrtr.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 77 insertions(+), 16 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH V1 1/4] net: qrtr: Do not send packets before hello negotiation

[Deepak Kumar Singh]

From: Chris Lew <clew@codeaurora.org>

There is a race where broadcast packets can be sent to a node that has
not sent the hello message to the remote processor. This breaks the
protocol expectation. Add a status variable to track when the hello
packet has been sent.

An alternative solution attempted was to remove the nodes from the
broadcast list until the hello packet is sent. This is not a valid
solution because hello messages are broadcasted if the ns is restarted
or started late. There needs to be a status variable separate from the
broadcast list.
---
 net/qrtr/qrtr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index 90c558f8..d9858a1 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -115,6 +115,7 @@ static DEFINE_MUTEX(qrtr_port_lock);
  * @ep: endpoint
  * @ref: reference count for node
  * @nid: node id
+ * @hello_sent: hello packet sent to endpoint
  * @qrtr_tx_flow: tree of qrtr_tx_flow, keyed by node << 32 | port
  * @qrtr_tx_lock: lock for qrtr_tx_flow inserts
  * @rx_queue: receive queue
@@ -125,6 +126,7 @@ struct qrtr_node {
 	struct qrtr_endpoint *ep;
 	struct kref ref;
 	unsigned int nid;
+	atomic_t hello_sent;
 
 	struct radix_tree_root qrtr_tx_flow;
 	struct mutex qrtr_tx_lock; /* for qrtr_tx_flow */
@@ -335,6 +337,11 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	int rc = -ENODEV;
 	int confirm_rx;
 
+	if (!atomic_read(&node->hello_sent) && type != QRTR_TYPE_HELLO) {
+		kfree_skb(skb);
+		return rc;
+	}
+
 	confirm_rx = qrtr_tx_wait(node, to->sq_node, to->sq_port, type);
 	if (confirm_rx < 0) {
 		kfree_skb(skb);
@@ -370,6 +377,8 @@ static int qrtr_node_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 	 * confirm_rx flag if we dropped this one */
 	if (rc && confirm_rx)
 		qrtr_tx_flow_failed(node, to->sq_node, to->sq_port);
+	if (!rc && type == QRTR_TYPE_HELLO)
+		atomic_inc(&node->hello_sent);
 
 	return rc;
 }
@@ -563,6 +572,7 @@ int qrtr_endpoint_register(struct qrtr_endpoint *ep, unsigned int nid)
 	skb_queue_head_init(&node->rx_queue);
 	node->nid = QRTR_EP_NID_AUTO;
 	node->ep = ep;
+	atomic_set(&node->hello_sent, 0);
 
 	INIT_RADIX_TREE(&node->qrtr_tx_flow, GFP_KERNEL);
 	mutex_init(&node->qrtr_tx_lock);
@@ -854,6 +864,8 @@ static int qrtr_bcast_enqueue(struct qrtr_node *node, struct sk_buff *skb,
 
 	mutex_lock(&qrtr_node_lock);
 	list_for_each_entry(node, &qrtr_all_nodes, item) {
+		if (node->nid == QRTR_EP_NID_AUTO)
+			continue;
 		skbn = skb_clone(skb, GFP_KERNEL);
 		if (!skbn)
 			break;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 1/4] net: qrtr: Do not send packets before hello negotiation

[David Miller]

A proper patch series must provide a header "[PATCH 0/N ..." posting which
explains what the patch series does, at a high level, how it does it, and
why it does it that way.

You must also explicitly state the target GIT tree your changes are for
in the subject line, f.e. "[PATCH net-next M/N] ..."

I'm sorry I have to be strict about this but it is very important for
reviewers, and anyone looking at these changes in the future, to have
this summary information.

Thank you.

[PATCH V1 2/4] net: qrtr: Add socket mode optimization

[Deepak Kumar Singh]

From: Chris Lew <clew@codeaurora.org>

A remote endpoint should not need to know when a client socket is freed
if the socket never established commnication with the endpoint. Add a
mode to keep track of which endpoints a socket communicates with.

There are three modes a socket can be in:
        INIT   - Socket has not sent anything or only local messages,
                 only send client close to local services.

        SINGLE - Socket has sent messages to a single ept, send event
                 to this single ept.

        MULTI  - Socket has sent messages to multiple epts, broadcast
                 release of this socket.

Server state changes should be broadcast throughout the system. Change
the ipc state of a port when it sends a NEW SERVER control packet. This
ensures the DEL CLIENT control packet is propagated correctly for
servers.
---
 net/qrtr/qrtr.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 13 deletions(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index d9858a1..4496b75 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -21,6 +21,10 @@
 #define QRTR_MIN_EPH_SOCKET 0x4000
 #define QRTR_MAX_EPH_SOCKET 0x7fff
 
+/* qrtr socket states */
+#define QRTR_STATE_MULTI	-2
+#define QRTR_STATE_INIT		-1
+
 /**
  * struct qrtr_hdr_v1 - (I|R)PCrouter packet header version 1
  * @version: protocol version
@@ -87,6 +91,8 @@ struct qrtr_sock {
 	struct sock sk;
 	struct sockaddr_qrtr us;
 	struct sockaddr_qrtr peer;
+
+	int state;
 };
 
 static inline struct qrtr_sock *qrtr_sk(struct sock *sk)
@@ -653,29 +659,59 @@ static void qrtr_port_put(struct qrtr_sock *ipc)
 	sock_put(&ipc->sk);
 }
 
-/* Remove port assignment. */
-static void qrtr_port_remove(struct qrtr_sock *ipc)
+static void qrtr_send_del_client(struct qrtr_sock *ipc)
 {
 	struct qrtr_ctrl_pkt *pkt;
-	struct sk_buff *skb;
-	int port = ipc->us.sq_port;
 	struct sockaddr_qrtr to;
+	struct qrtr_node *node;
+	struct sk_buff *skbn;
+	struct sk_buff *skb;
+	int type = QRTR_TYPE_DEL_CLIENT;
+
+	skb = qrtr_alloc_ctrl_packet(&pkt);
+	if (!skb)
+		return;
 
 	to.sq_family = AF_QIPCRTR;
 	to.sq_node = QRTR_NODE_BCAST;
 	to.sq_port = QRTR_PORT_CTRL;
 
-	skb = qrtr_alloc_ctrl_packet(&pkt);
-	if (skb) {
-		pkt->cmd = cpu_to_le32(QRTR_TYPE_DEL_CLIENT);
-		pkt->client.node = cpu_to_le32(ipc->us.sq_node);
-		pkt->client.port = cpu_to_le32(ipc->us.sq_port);
+	pkt->cmd = cpu_to_le32(QRTR_TYPE_DEL_CLIENT);
+	pkt->client.node = cpu_to_le32(ipc->us.sq_node);
+	pkt->client.port = cpu_to_le32(ipc->us.sq_port);
+
+	skb_set_owner_w(skb, &ipc->sk);
 
-		skb_set_owner_w(skb, &ipc->sk);
-		qrtr_bcast_enqueue(NULL, skb, QRTR_TYPE_DEL_CLIENT, &ipc->us,
-				   &to);
+	if (ipc->state == QRTR_STATE_MULTI) {
+		qrtr_bcast_enqueue(NULL, skb, type, &ipc->us, &to);
+		return;
+	}
+
+	if (ipc->state > QRTR_STATE_INIT) {
+		node = qrtr_node_lookup(ipc->state);
+		if (!node)
+			goto exit;
+
+		skbn = skb_clone(skb, GFP_KERNEL);
+		if (!skbn) {
+			qrtr_node_release(node);
+			goto exit;
+		}
+
+		skb_set_owner_w(skbn, &ipc->sk);
+		qrtr_node_enqueue(node, skbn, type, &ipc->us, &to);
+		qrtr_node_release(node);
 	}
+exit:
+	qrtr_local_enqueue(NULL, skb, type, &ipc->us, &to);
+}
 
+/* Remove port assignment. */
+static void qrtr_port_remove(struct qrtr_sock *ipc)
+{
+	int port = ipc->us.sq_port;
+
+	qrtr_send_del_client(ipc);
 	if (port == QRTR_PORT_CTRL)
 		port = 0;
 
@@ -941,6 +977,11 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 			return -ECONNRESET;
 		}
 		enqueue_fn = qrtr_node_enqueue;
+
+		if (ipc->state > QRTR_STATE_INIT && ipc->state != node->nid)
+			ipc->state = QRTR_STATE_MULTI;
+		else if (ipc->state == QRTR_STATE_INIT)
+			ipc->state = node->nid;
 	}
 
 	plen = (len + 3) & ~3;
@@ -957,7 +998,8 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 		goto out_node;
 	}
 
-	if (ipc->us.sq_port == QRTR_PORT_CTRL) {
+	if (ipc->us.sq_port == QRTR_PORT_CTRL ||
+	    addr->sq_port == QRTR_PORT_CTRL) {
 		if (len < 4) {
 			rc = -EINVAL;
 			kfree_skb(skb);
@@ -969,6 +1011,9 @@ static int qrtr_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 	}
 
 	type = le32_to_cpu(qrtr_type);
+	if (addr->sq_port == QRTR_PORT_CTRL && type == QRTR_TYPE_NEW_SERVER)
+		ipc->state = QRTR_STATE_MULTI;
+
 	rc = enqueue_fn(node, skb, type, &ipc->us, addr);
 	if (rc >= 0)
 		rc = len;
@@ -1256,6 +1301,7 @@ static int qrtr_create(struct net *net, struct socket *sock,
 	ipc->us.sq_family = AF_QIPCRTR;
 	ipc->us.sq_node = qrtr_local_nid;
 	ipc->us.sq_port = 0;
+	ipc->state = QRTR_STATE_INIT;
 
 	return 0;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH V1 3/4] net: qrtr: Change port allocation to use cyclic idr

[Deepak Kumar Singh]

From: Chris Lew <clew@codeaurora.org>

There is a race for clients that open sockets before the control port
is bound. If a client gets an idr that was allocated before the control
port is bound, there is a chance the previous address owner sent lookup
packets to the control port. The new address owner will get residual
responses to this the lookup packets.

Change the idr_alloc to idr_alloc_cyclic so new idr's are allocated
instead of trying to reuse the freed idrs.
---
 net/qrtr/qrtr.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index 4496b75..e2dd38e 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -744,7 +744,8 @@ static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
 	mutex_lock(&qrtr_port_lock);
 	if (!*port) {
 		min_port = QRTR_MIN_EPH_SOCKET;
-		rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, QRTR_MAX_EPH_SOCKET, GFP_ATOMIC);
+		rc = idr_alloc_cyclic(&qrtr_ports, ipc, &min_port,
+				      QRTR_MAX_EPH_SOCKET, GFP_ATOMIC);
 		if (!rc)
 			*port = min_port;
 	} else if (*port < QRTR_MIN_EPH_SOCKET && !capable(CAP_NET_ADMIN)) {
@@ -754,7 +755,8 @@ static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
 		rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, 0, GFP_ATOMIC);
 	} else {
 		min_port = *port;
-		rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, *port, GFP_ATOMIC);
+		rc = idr_alloc_cyclic(&qrtr_ports, ipc, &min_port,
+				      *port, GFP_ATOMIC);
 		if (!rc)
 			*port = min_port;
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 3/4] net: qrtr: Change port allocation to use cyclic idr

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4277 bytes --]

Hi Deepak,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.9-rc2 next-20200828]
[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]

url:    https://github.com/0day-ci/linux/commits/Deepak-Kumar-Singh/General-qrtr-fixes/20200830-224348
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1127b219ce9481c84edad9711626d856127d5e51
config: x86_64-randconfig-a014-20200830 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c10e63677f5d20f18010f8f68c631ddc97546f7d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> net/qrtr/qrtr.c:747:43: warning: incompatible pointer to integer conversion passing 'u32 *' (aka 'unsigned int *') to parameter of type 'int' [-Wint-conversion]
                   rc = idr_alloc_cyclic(&qrtr_ports, ipc, &min_port,
                                                           ^~~~~~~~~
   include/linux/idr.h:117:51: note: passing argument to parameter 'start' here
   int idr_alloc_cyclic(struct idr *, void *ptr, int start, int end, gfp_t);
                                                     ^
   net/qrtr/qrtr.c:758:43: warning: incompatible pointer to integer conversion passing 'u32 *' (aka 'unsigned int *') to parameter of type 'int' [-Wint-conversion]
                   rc = idr_alloc_cyclic(&qrtr_ports, ipc, &min_port,
                                                           ^~~~~~~~~
   include/linux/idr.h:117:51: note: passing argument to parameter 'start' here
   int idr_alloc_cyclic(struct idr *, void *ptr, int start, int end, gfp_t);
                                                     ^
   2 warnings generated.

# https://github.com/0day-ci/linux/commit/66a6331807301aa59aa1355f7e21546f88cb0b2d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Deepak-Kumar-Singh/General-qrtr-fixes/20200830-224348
git checkout 66a6331807301aa59aa1355f7e21546f88cb0b2d
vim +747 net/qrtr/qrtr.c

   728	
   729	/* Assign port number to socket.
   730	 *
   731	 * Specify port in the integer pointed to by port, and it will be adjusted
   732	 * on return as necesssary.
   733	 *
   734	 * Port may be:
   735	 *   0: Assign ephemeral port in [QRTR_MIN_EPH_SOCKET, QRTR_MAX_EPH_SOCKET]
   736	 *   <QRTR_MIN_EPH_SOCKET: Specified; requires CAP_NET_ADMIN
   737	 *   >QRTR_MIN_EPH_SOCKET: Specified; available to all
   738	 */
   739	static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
   740	{
   741		u32 min_port;
   742		int rc;
   743	
   744		mutex_lock(&qrtr_port_lock);
   745		if (!*port) {
   746			min_port = QRTR_MIN_EPH_SOCKET;
 > 747			rc = idr_alloc_cyclic(&qrtr_ports, ipc, &min_port,
   748					      QRTR_MAX_EPH_SOCKET, GFP_ATOMIC);
   749			if (!rc)
   750				*port = min_port;
   751		} else if (*port < QRTR_MIN_EPH_SOCKET && !capable(CAP_NET_ADMIN)) {
   752			rc = -EACCES;
   753		} else if (*port == QRTR_PORT_CTRL) {
   754			min_port = 0;
   755			rc = idr_alloc_u32(&qrtr_ports, ipc, &min_port, 0, GFP_ATOMIC);
   756		} else {
   757			min_port = *port;
   758			rc = idr_alloc_cyclic(&qrtr_ports, ipc, &min_port,
   759					      *port, GFP_ATOMIC);
   760			if (!rc)
   761				*port = min_port;
   762		}
   763		mutex_unlock(&qrtr_port_lock);
   764	
   765		if (rc == -ENOSPC)
   766			return -EADDRINUSE;
   767		else if (rc < 0)
   768			return rc;
   769	
   770		sock_hold(&ipc->sk);
   771	
   772		return 0;
   773	}
   774	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34367 bytes --]

[PATCH V1 4/4] net: qrtr: Check function pointer before calling

[Deepak Kumar Singh]

From: Arun Kumar Neelakantam <aneela@codeaurora.org>

sk_error_report callback function called without validating cause the NULL
pointer dereference.

Validate function pointer before using for error report.
---
 net/qrtr/qrtr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/qrtr/qrtr.c b/net/qrtr/qrtr.c
index e2dd38e..01cabd3 100644
--- a/net/qrtr/qrtr.c
+++ b/net/qrtr/qrtr.c
@@ -786,7 +786,8 @@ static void qrtr_reset_ports(void)
 
 		sock_hold(&ipc->sk);
 		ipc->sk.sk_err = ENETRESET;
-		ipc->sk.sk_error_report(&ipc->sk);
+		if (ipc->sk.sk_error_report)
+			ipc->sk.sk_error_report(&ipc->sk);
 		sock_put(&ipc->sk);
 	}
 	mutex_unlock(&qrtr_port_lock);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project