Re: [PATCH net v3 5/6] amt: make pre-query report drops visible

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

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] In gateway mode, amt_dev_xmit() (drivers/net/amt.c) counts
  every successfully forwarded IGMP/MLD report as a TX drop.
- [Low] The new netdev_dbg in amt_dev_xmit() (drivers/net/amt.c:1259)
  says "drop %s report: no Membership Query for this family yet"…
- [Low] net: amt: lockless non-atomic increments of dev->stats in tx path

--

amt: make pre-query report drops visible

This adds a family-specific netdev_dbg() to amt_dev_xmit(). It fires when
a gateway drops an IGMP or MLD report because the relay's Membership Query
for that family has not arrived yet. The commit message says the existing
drop path already increments tx_dropped, so each discarded report is
counted once.

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 2feb418e87edb..fec32650cb91f 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1249,9 +1249,17 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		/* Gateway only passes IGMP/MLD packets */
>  		if (!report)
>  			goto free;
> +		/* A validated report can only be forwarded after the relay's
> +		 * family-specific Membership Query supplies the state echoed
> +		 * by the Membership Update. Log this readiness failure before
> +		 * the shared drop path accounts it.
> +		 */
>  		if ((!v6 && !READ_ONCE(amt->ready4)) ||
> -		    (v6 && !READ_ONCE(amt->ready6)))
> +		    (v6 && !READ_ONCE(amt->ready6))) {
> +			netdev_dbg(dev, "drop %s report: no Membership Query for this family yet\n",
> +				   v6 ? "MLD" : "IGMP");

[Severity: Low]
This isn't introduced by this patch, but the commit message relies on
tx_dropped to count each discarded report exactly once. Is that counter
reliable here?

amt_link_setup() sets dev->lltx = true along with IFF_NO_QUEUE. So the
core does not take HARD_TX_LOCK around ndo_start_xmit, and
amt_dev_xmit() can run on several CPUs at once.

Both drop paths in amt_dev_xmit() update the counters with plain
increments:

	dev->stats.tx_errors++;
	...
	dev->stats.tx_dropped++;

These are non-atomic read-modify-write operations on unsigned long.
Two CPUs dropping packets at the same time can lose increments. The
only effect is inaccurate statistics, but it means "accounted exactly
once" doesn't hold under concurrent transmit.

Would it make sense to switch these to DEV_STATS_INC(dev, tx_dropped)
and DEV_STATS_INC(dev, tx_errors)? Those use atomic_long_inc() on the
same fields. Another option is to move the driver to per-CPU stats.
That is probably a separate fix rather than something for this patch.

[Severity: Low]
This isn't a bug, but could the wording of this message be a bit more
precise?

ready4 and ready6 are false before the first Query, and they are also
cleared on every refresh cycle in amt_event_send_request():

	if (!amt->req_cnt) {
		WRITE_ONCE(amt->ready4, false);
		WRITE_ONCE(amt->ready6, false);
		get_random_bytes(&amt->nonce, sizeof(__be32));
	}

They are cleared again on the relay-lost path, when req_cnt exceeds
AMT_MAX_REQ_COUNT.

The nonce is regenerated at the same time, and
amt_membership_query_handler() rejects Queries with a stale nonce. So the
message is correct for the current Request.

Still, an operator who sees it in steady state might read "no Membership
Query for this family yet" as meaning the relay never answered. Would
something like "waiting for Membership Query" be clearer?

> +			goto free;
> +		}
>  		if (amt_send_membership_update(amt, skb,  v6))
>  			goto free;
>  		goto unlock;

[Severity: Medium]
The commit message says:

    That path already increments tx_dropped, so each discarded report
    is accounted exactly once.

The new comment also refers to "the shared drop path". Can tx_dropped
actually be used to count these drops?

When amt_send_membership_update() returns false, it has already handed
the skb to the tunnel:

	udp_tunnel_xmit_skb(rt, sk, skb, ...);
	amt_update_gw_status(amt, AMT_STATUS_SENT_UPDATE, true);
	return false;

The caller then does goto unlock. That label reaches the same increment
that the real drops use:

free:
	dev_kfree_skb(skb);
unlock:
	dev->stats.tx_dropped++;
	return NETDEV_TX_OK;

So in gateway mode, every IGMP/MLD report that is forwarded successfully
is also counted in tx_dropped. That doesn't match the rtnl_link_stats64
definition of tx_dropped ("Number of packets dropped on their way to
transmission").

The success-path increment has been there since commit cbc21dc1cfe9
("amt: add data plane of amt interface"). This patch, though, depends on
tx_dropped to count the pre-Query drops. How would the counter tell those
drops apart from reports that were sent?

Should the goto unlock success path skip the tx_dropped increment?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012125943.558629-1-omar%40blockcast.net