Re: [PATCH net v2] amt: do not store tunnel pointer in skb control block

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new identity-based re-lookup in amt_dev_xmit() can return
  a tunnel object that amt_request_handler() has published to the RCU…

Pre-existing issues:
- [High] amt_dev_stop() mutates amt->tunnel_list without holding
  amt->lock, racing with amt_tunnel_expire() which mutates the same list…
- [Medium] amt_dev_xmit() counts successfully transmitted packets as TX
  drops.

--

amt: do not store tunnel pointer in skb control block

This replaces the raw struct amt_tunnel_list pointer kept in skb->cb with
the tunnel identity (ip4 + source_port), and re-looks-up the tunnel under
RCU in amt_dev_xmit() so that an skb outliving its tunnel in a qdisc no
longer dereferences freed memory.  A few questions about the new lookup and
about surrounding code it now depends on.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index bddc24e1856de..b660cebf2484c 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -791,6 +791,18 @@ static void amt_send_request(struct amt_dev *amt, bool v6)
>  	rcu_read_unlock();
>  }
>  
> +static struct amt_tunnel_list *amt_lookup_tunnel(struct amt_dev *amt,
> +						 __be32 ip4, __be16 source_port)
> +{
> +	struct amt_tunnel_list *tunnel;
> +
> +	list_for_each_entry_rcu(tunnel, &amt->tunnel_list, list)
> +		if (tunnel->ip4 == ip4 && tunnel->source_port == source_port)
> +			return tunnel;
> +
> +	return NULL;
> +}
> +

[Severity: High]
This is a pre-existing issue and not introduced here, but since this new
lockless walk now depends on the integrity of amt->tunnel_list, is the list
actually consistently protected?

amt_tunnel_expire() deletes under amt->lock:

drivers/net/amt.c:amt_tunnel_expire() {
	spin_lock_bh(&amt->lock);
	rcu_read_lock();
	list_del_rcu(&tunnel->list);
	amt->nr_tunnels--;
	amt_clear_groups(tunnel);
	rcu_read_unlock();
	spin_unlock_bh(&amt->lock);
	kfree_rcu(tunnel, rcu);
}

amt_dev_stop() deletes the same entries with no lock at all, and only
cancels the per-tunnel gc work after it has already unlinked and while
other tunnels' gc works can still be running:

drivers/net/amt.c:amt_dev_stop() {
	list_for_each_entry_safe(tunnel, tmp, &amt->tunnel_list, list) {
		list_del_rcu(&tunnel->list);
		amt->nr_tunnels--;
		cancel_delayed_work_sync(&tunnel->gc_wq);
		amt_clear_groups(tunnel);
		kfree_rcu(tunnel, rcu);
	}
}

amt_dev_stop() does not disable the gc_wq of any tunnel before the loop
(only req_wq, discovery_wq, secret_wq and event_wq are cancelled earlier),
so a gc timer firing while the device is being brought down runs
amt_tunnel_expire() concurrently.  Can that give two list_del_rcu() calls
on the same entry, where the second __list_del_entry() writes through
LIST_POISON2, plus a double kfree_rcu() of the same tunnel?  And can the
concurrent unsynchronized list and nr_tunnels updates leave a stale next
pointer for amt_lookup_tunnel() and for the data-forwarding
list_for_each_entry_rcu() walk in amt_dev_xmit() to follow?

Would taking amt->lock around the amt_dev_stop() removals (and stopping the
gc work before unlinking) be the right fix?

[ ... ]

> @@ -1262,15 +1276,17 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		goto unlock;
>  	} else if (amt->mode == AMT_MODE_RELAY) {
>  		if (query) {
> -			tunnel = amt_skb_cb(skb)->tunnel;
> -			if (!tunnel) {
> -				WARN_ON(1);
> -				goto free;
> -			}
> -
> +			rcu_read_lock();
> +			tunnel = amt_lookup_tunnel(amt,
> +						   amt_skb_cb(skb)->tunnel_ip4,
> +						   amt_skb_cb(skb)->tunnel_port);

[Severity: Medium]
Can this lookup return a tunnel that amt_request_handler() has published but
not finished initializing?

amt_request_handler() sets only source_port and ip4 before publishing, and
writes nonce/mac after dropping amt->lock, at the send: label:

drivers/net/amt.c:amt_request_handler() {
	tunnel->source_port = udph->source;
	tunnel->ip4 = iph->saddr;
	...
	list_add_tail_rcu(&tunnel->list, &amt->tunnel_list);
	tunnel->key = amt->key;
	__amt_update_relay_status(tunnel, AMT_STATUS_RECEIVED_REQUEST, true);
	amt->nr_tunnels++;
	mod_delayed_work(amt_wq, &tunnel->gc_wq,
			 msecs_to_jiffies(amt_gmi(amt)));
	spin_unlock_bh(&amt->lock);

send:
	tunnel->nonce = amtrh->nonce;
	...
	tunnel->mac = mac >> 16;
}

amt_lookup_tunnel() matches purely on ip4 + source_port, and both are
already visible at list_add_tail_rcu() time.  So if an older query skb for
an expired tunnel is still sitting in a qdisc (the netem case from the
commit message) and the same gateway re-sends a Request from the same UDP
source port, can the dequeue on another CPU match the new, half-initialized
tunnel and build the query from the kzalloc'ed zeros?

drivers/net/amt.c:amt_send_membership_query() {
	amtmq->nonce	= tunnel->nonce;
	amtmq->response_mac = tunnel->mac;
	...
	amt_update_relay_status(tunnel, AMT_STATUS_SENT_QUERY, true);
}

The gateway then discards such a query:

drivers/net/amt.c:amt_membership_query_handler() {
	if (amtmq->nonce != amt->nonce)
		return true;
}

while the relay has already moved that tunnel to AMT_STATUS_SENT_QUERY.
Would completing the tunnel initialization (key, nonce, mac) before
list_add_tail_rcu(), or carrying the nonce as part of the identity stored in
skb->cb, close this window?

>  			/* Do not forward unexpected query */
> -			if (amt_send_membership_query(amt, skb, tunnel, v6))
> +			if (!tunnel ||
> +			    amt_send_membership_query(amt, skb, tunnel, v6)) {
> +				rcu_read_unlock();
>  				goto free;
> +			}
> +			rcu_read_unlock();
>  			goto unlock;

[Severity: Medium]
This isn't a bug introduced by this patch, but this goto unlock is the
success exit of the path being rewritten here, and it lands on the drop
counter:

drivers/net/amt.c:amt_dev_xmit() {
free:
	dev_kfree_skb(skb);
unlock:
	dev->stats.tx_dropped++;
	return NETDEV_TX_OK;
}

The free: label falls through into the same increment, so a query that was
successfully handed to udp_tunnel_xmit_skb() is counted as tx_dropped, the
same as one that was thrown away.  The gateway report path reaches the same
label after amt_send_membership_update() succeeds.  Since tx_dropped in
struct rtnl_link_stats64 is defined as packets dropped on their way to
transmission, and tx_packets/tx_bytes are never incremented for these
paths, should the success exits use a separate label that skips the
tx_dropped increment?

>  		}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922214150.13970-1-cenzhang%40linux.microsoft.com