Re: [net PATCH 07/13] fbnic: Let the multicast sync reuse an entry waiting for deletion

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 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).

- [High] The patch fixes two defects in __fbnic_mc_sync().
- [Medium] A full-region multicast swap can still drop the device to all-
  multi with room to spare when the removed host address is also tagged…

--

fbnic: Let the multicast sync reuse an entry waiting for deletion

This patch lets __fbnic_mc_sync() treat MACDA entries in the DELETE state
as free. This lets a full multicast region swap one address for another
in a single rx mode pass. It also resets a matched DELETE entry, clearing
its tags, so fbnic_write_macda() no longer wipes the tag the caller has
just set.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index d69c91066508f..ee781a3e15285 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> @@ -520,12 +520,16 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
>  	     --i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX;) {
>  		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[i];
>  
> -		if (mac_addr->state == FBNIC_TCAM_S_DISABLED) {
> -			avail_addr = mac_addr;
> -		} else if (ether_addr_equal(mac_addr->value.addr8, addr)) {
> +		if (mac_addr->state != FBNIC_TCAM_S_DISABLED &&
> +		    ether_addr_equal(mac_addr->value.addr8, addr)) {
>  			avail_addr = mac_addr;
>  			break;
>  		}
> +
> +		/* An entry waiting to be deleted is as good as a free one */
> +		if (mac_addr->state == FBNIC_TCAM_S_DISABLED ||
> +		    mac_addr->state == FBNIC_TCAM_S_DELETE)
> +			avail_addr = mac_addr;
>  	}

[Severity: Medium]
Can a full-region swap still end in all-multi with a slot free, when the
address being removed is also tagged for the BMC?

Take this setup:

- every shared multicast slot is in use
- one of those slots holds address A with both the MULTICAST and BMC tags
- one of the BMC-only slots below it is DISABLED

Now the host leaves A and joins B in one rx mode pass:

__hw_addr_sync_dev()
  fbnic_mc_unsync(A)
    __fbnic_mc_unsync()
      clears the MULTICAST bit. The BMC bit stays set, so
      __fbnic_xc_unsync() leaves the entry VALID, not DELETE.
  fbnic_mc_sync(B)
    __fbnic_mc_sync()
      finds no match and no DISABLED or DELETE slot, and B is not
      in the BMC region, so it returns NULL
    returns -ENOSPC

__fbnic_set_rx_mode() then turns on all-multi:

	err = __hw_addr_sync_dev(mc, netdev, fbnic_mc_sync, fbnic_mc_unsync);
	if (err == -ENOSPC)
		mc_promisc = true;

Later in the same pass, fbnic_sift_macda() moves the now BMC-only A into
the free BMC slot and marks its shared slot DELETE. fbnic_write_macda()
then frees that slot.

Nothing retries the sync of B, so its sync_cnt stays at zero. Wouldn't the
device then stay in all-multi with a free slot until the address lists
next change? That is the symptom the commit message describes as "the
device can sit in all-multi with room to spare until then".

[ ... ]

> @@ -551,9 +555,12 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
>  		break;
>  	}
>  
> -	if (avail_addr && avail_addr->state == FBNIC_TCAM_S_DISABLED) {
> +	if (avail_addr && (avail_addr->state == FBNIC_TCAM_S_DISABLED ||
> +			   avail_addr->state == FBNIC_TCAM_S_DELETE)) {
>  		ether_addr_copy(avail_addr->value.addr8, addr);
>  		eth_zero_addr(avail_addr->mask.addr8);
> +		bitmap_zero(avail_addr->act_tcam,
> +			    FBNIC_RPC_TCAM_ACT_NUM_ENTRIES);
>  		avail_addr->state = FBNIC_TCAM_S_ADD;
>  	}

[Severity: High]
Does __fbnic_uc_sync() need the same fix? It has both of the patterns this
patch fixes for multicast, and it is left unchanged:

drivers/net/ethernet/meta/fbnic/fbnic_rpc.c:__fbnic_uc_sync() {
	...
		if (mac_addr->state == FBNIC_TCAM_S_DISABLED) {
			avail_addr = mac_addr;
		} else if (ether_addr_equal(mac_addr->value.addr8, addr)) {
			avail_addr = mac_addr;
			break;
		}
	}

	if (avail_addr && avail_addr->state == FBNIC_TCAM_S_DISABLED) {
	...
}

The first pattern is the swap. Suppose the unicast region is full and one
secondary unicast address (a macvlan, for example) is swapped for another
in a single rx mode pass. That hits the same unsync-before-sync ordering:

- fbnic_uc_unsync() only marks the old entry DELETE
- __fbnic_uc_sync() returns NULL
- fbnic_uc_sync() returns -ENOSPC

In __fbnic_set_rx_mode() that sets uc_promisc, which also forces
mc_promisc:

	if (err == -ENOSPC)
		uc_promisc = true;
	...
	mc_promisc |= !!(netdev->flags & IFF_ALLMULTI) || uc_promisc;

The new address keeps a sync_cnt of zero. Won't the device then stay fully
promiscuous, with a free slot, until the address lists change again?

The second pattern is the matched DELETE entry. The else-if branch can
match a DELETE entry by address and return it still in DELETE. The caller
then sets its tag, and fbnic_write_macda() wipes the entry, tag included:

		if (mac_addr->state == FBNIC_TCAM_S_DELETE) {
			/* Invalidate entry and clear addr state info */
			fbnic_clear_macda_entry(fbd, idx);
			memset(mac_addr, 0, sizeof(*mac_addr));

There seem to be two ways to reach this for unicast.

In the first, the interface is down and an ethtool ETHER_FLOW rule for
unicast DA X is deleted:

- fbnic_clear_nfc_macda() skips fbnic_write_macda() when !netif_running(),
  so the entry stays in DELETE.
- A new rule for X then goes through
  fbnic_set_cls_rule_ins()->__fbnic_uc_sync(), gets the DELETE entry, and
  sets the idx bit.
- On ifup, fbnic_write_macda() wipes the entry.

The act TCAM rule then points at an empty MACDA index. Can that index
later be handed out for a different address, so the rule steers the wrong
traffic?

In the second, host secondary unicast X shares an entry with an NFC rule:

- On ifdown, fbnic_clear_rx_mode() and fbnic_clear_macda() leave the
  entry NFC-only.
- Deleting the NFC rule while the interface is down moves it to DELETE.
- On ifup, fbnic_uc_sync(X) matches the DELETE entry and sets
  FBNIC_MAC_ADDR_T_UNICAST.
- fbnic_write_macda() then wipes the entry.

Wouldn't traffic to X be dropped, while the core counts X as synced and
never retries it?

fbnic_set_cls_rule_ins() picks __fbnic_mc_sync() or __fbnic_uc_sync() by
address type and handles both results the same way. After this patch, only
the multicast branch is sure not to return an entry still in DELETE.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179322311834.40667.5383039334126572422.stgit%40ahduyck-xeon-server.home.arpa