Re: [PATCH net] net: team: stop reusing skb after queue override

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "Lets fix all the bugs in this area in one series"
---
net: team: stop reusing skb after queue override

This patch fixes a use-after-free in team_xmit(). Before it,
team_queue_override_transmit() treated a non-zero dev_queue_xmit() return
as "no handoff" and then reused the skb, which had already been consumed.
The patch reports the handoff separately from transmit success and only
tries the first override port.

> diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
> index beffbe450612b..59dd94271ff19 100644
> --- a/drivers/net/team/team_core.c
> +++ b/drivers/net/team/team_core.c

[ ... ]

> @@ -1864,8 +1865,7 @@ static netdev_tx_t team_xmit(struct sk_buff *skb, struct net_device *dev)
>  	bool tx_success;
>  	unsigned int len = skb->len;
>  
> -	tx_success = team_queue_override_transmit(team, skb);
> -	if (!tx_success)
> +	if (!team_queue_override_transmit(team, skb, &tx_success))
>  		tx_success = READ_ONCE(team->ops.transmit)(team, skb);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. This fix
relies on the transmit op always consuming the skb. Does broadcast mode
break that rule?

In bc_transmit() in drivers/net/team/team_mode_broadcast.c, the original
skb is only handed off if a txable port was found:

	if (last) {
		ret = !team_dev_queue_xmit(team, last, skb);
		if (!sum_ret)
			sum_ret = ret;
	}
	return sum_ret;

If no port passes team_port_txable(), last is NULL and the skb is neither
sent nor freed. team_xmit() then only increments tx_dropped and returns
NETDEV_TX_OK.

Does this leak the skb and its sk_wmem_alloc charge, since sock_wfree()
never runs?

Can this state be reached? team_adjust_ops() picks the mode transmit op
based only on tx_en_port_count:

	if (!team->tx_en_port_count || !team_is_mode_set(team) ||
	    !team->mode->ops->transmit)
		WRITE_ONCE(team->ops.transmit, team_dummy_transmit);
	else
		WRITE_ONCE(team->ops.transmit, team->mode->ops->transmit);

However, team_port_txable() also requires port->linkup.

For example, say port A has link but its tx is disabled through the
tx_enabled option. Port B is oper-down but tx-enabled, which is the default
after team_port_add(). __team_carrier_check() keeps carrier on because port
A has link, and tx_en_port_count == 1 keeps bc_transmit() installed. Every
packet sent through the team device would then leak.

The same could happen if all tx-enabled ports lose link while transmits
are in flight. It could also happen if user_carrier_enabled keeps carrier
up while all ports are down.

The other transmit ops (roundrobin, random, loadbalance, activebackup and
team_dummy_transmit()) all call dev_kfree_skb_any() when they find no port.

Should bc_transmit() also call dev_kfree_skb_any(skb) when last is NULL?

[ ... ]
-- 
pw-bot: cr