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

Commit Message

Deepak Kumar Singh Aug. 30, 2020, 2:38 p.m. UTC

From: Chris Lew <[e-mail]>

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

Comments

David Miller Aug. 31, 2020, 2:18 a.m. UTC | #1

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

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;