[PATCH net 2/4] amt: send the relay General Query directly instead of via dev_queue_xmit

[Omar Ramadan <omar@blockcast.net>]

amt_send_igmp_gq() and amt_send_mld_gq() build the relay's General Query
with an L2 header, stash the destination tunnel in
amt_skb_cb(skb)->tunnel, and dev_queue_xmit() the skb so it loops back
through amt_dev_xmit(), which recovers the tunnel from skb->cb and calls
amt_send_membership_query().

skb->cb is not guaranteed to survive the transmit path -- qdisc, tc and
GRO may write into it. When the control block is clobbered between the
queue and the amt_dev_xmit() re-entry, amt_dev_xmit() reads back a
foreign tunnel and sends the Query to the wrong endpoint (in practice
the relay's own address with UDP source port 0). For a gateway that
shares the relay's L2 segment the mis-routed packet loops back locally
instead of failing, so the gateway never sees the Query and its
handshake stalls until the tunnel is garbage-collected.

The relay already holds the correct amt_tunnel_list when it builds the
Query, so the dev_queue_xmit() round-trip is both unnecessary and
fragile. Strip the L2 header and call amt_send_membership_query()
directly -- exactly what amt_dev_xmit() does for the query path --
freeing the skb on the sender's error return.

That leaves amt_skb_cb(skb)->tunnel with no writer, so delete the
relay's query branch in amt_dev_xmit() together with struct amt_skb_cb
and amt_skb_cb(). A query that still reaches amt_dev_xmit() is now
dropped like any other non-data packet instead of reading an unset
control block (and hitting WARN_ON(1) when it is NULL).

Fixes: cbc21dc1cfe9 ("amt: add data plane of amt interface")
Signed-off-by: Omar Ramadan <omar@blockcast.net>
---
 drivers/net/amt.c | 53 ++++++++++++++++++-----------------------------
 include/net/amt.h |  4 ----
 2 files changed, 20 insertions(+), 37 deletions(-)

diff --git a/drivers/net/amt.c b/drivers/net/amt.c
index a652c8c79..17dceeaa1 100644
--- a/drivers/net/amt.c
+++ b/drivers/net/amt.c
@@ -80,15 +80,6 @@ static struct in6_addr mld2_all_node = MLD2_ALL_NODE_INIT;
 static struct mld2_grec mldv2_zero_grec;
 #endif
 
-static struct amt_skb_cb *amt_skb_cb(struct sk_buff *skb)
-{
-	BUILD_BUG_ON(sizeof(struct amt_skb_cb) + sizeof(struct tc_skb_cb) >
-		     sizeof_field(struct sk_buff, cb));
-
-	return (struct amt_skb_cb *)((void *)skb->cb +
-		sizeof(struct tc_skb_cb));
-}
-
 static void __amt_source_gc_work(void)
 {
 	struct amt_source_node *snode;
@@ -789,6 +780,19 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
 	rcu_read_unlock();
 }
 
+static bool amt_send_membership_query(struct amt_dev *amt,
+				      struct sk_buff *skb,
+				      struct amt_tunnel_list *tunnel,
+				      bool v6);
+
+/* Send the relay's General Query directly to the requesting gateway's tunnel.
+ *
+ * The query used to go through dev_queue_xmit() with the target tunnel stashed
+ * in skb->cb for amt_dev_xmit() to recover, but the control block does not
+ * survive every transmit path. We already hold the tunnel here, so strip the
+ * L2 header amt_build_igmp_gq() adds and call the membership-query sender
+ * directly. The sender returns true on error without consuming the skb.
+ */
 static void amt_send_igmp_gq(struct amt_dev *amt,
 			     struct amt_tunnel_list *tunnel)
 {
@@ -798,8 +802,9 @@ static void amt_send_igmp_gq(struct amt_dev *amt,
 	if (!skb)
 		return;
 
-	amt_skb_cb(skb)->tunnel = tunnel;
-	dev_queue_xmit(skb);
+	skb_pull(skb, sizeof(struct ethhdr));
+	if (amt_send_membership_query(amt, skb, tunnel, false))
+		kfree_skb(skb);
 }
 
 #if IS_ENABLED(CONFIG_IPV6)
@@ -883,8 +888,10 @@ static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
 	if (!skb)
 		return;
 
-	amt_skb_cb(skb)->tunnel = tunnel;
-	dev_queue_xmit(skb);
+	/* Direct send -- see amt_send_igmp_gq(). */
+	skb_pull(skb, sizeof(struct ethhdr));
+	if (amt_send_membership_query(amt, skb, tunnel, true))
+		kfree_skb(skb);
 }
 #else
 static void amt_send_mld_gq(struct amt_dev *amt, struct amt_tunnel_list *tunnel)
@@ -1183,7 +1190,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 #endif
 	bool report = false;
 	struct igmphdr *ih;
-	bool query = false;
 	struct iphdr *iph;
 	bool data = false;
 	bool v6 = false;
@@ -1201,9 +1207,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			case IGMP_HOST_MEMBERSHIP_REPORT:
 				report = true;
 				break;
-			case IGMP_HOST_MEMBERSHIP_QUERY:
-				query = true;
-				break;
 			default:
 				goto free;
 			}
@@ -1225,9 +1228,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			case ICMPV6_MLD2_REPORT:
 				report = true;
 				break;
-			case ICMPV6_MGM_QUERY:
-				query = true;
-				break;
 			default:
 				goto free;
 			}
@@ -1258,19 +1258,6 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 			goto free;
 		goto unlock;
 	} else if (amt->mode == AMT_MODE_RELAY) {
-		if (query) {
-			tunnel = amt_skb_cb(skb)->tunnel;
-			if (!tunnel) {
-				WARN_ON(1);
-				goto free;
-			}
-
-			/* Do not forward unexpected query */
-			if (amt_send_membership_query(amt, skb, tunnel, v6))
-				goto free;
-			goto unlock;
-		}
-
 		if (!data)
 			goto free;
 		list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list) {
diff --git a/include/net/amt.h b/include/net/amt.h
index c881bc8b6..ad844d65a 100644
--- a/include/net/amt.h
+++ b/include/net/amt.h
@@ -231,10 +231,6 @@ struct amt_relay_headers {
 	};
 } __packed;
 
-struct amt_skb_cb {
-	struct amt_tunnel_list *tunnel;
-};
-
 struct amt_tunnel_list {
 	struct list_head	list;
 	/* Protect All resources under an amt_tunne_list */
-- 
2.43.0