Re: [net-next, v4 07/10] bnge: add NTUPLE filter infrastructure

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Readers and writers access the __le64 filter_id of an RCU-
  published L2 filter concurrently, with plain loads and stores and no…

--

bnge: add NTUPLE filter infrastructure

This commit adds the NTUPLE filter data structures, the hash table and
bitmap management, and HWRM wrappers that allocate and free ntuple filters.
It also adds an RCU lookup helper for L2 filter IDs, which later patches
use for kernel flow steering.

> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> index 3b26939b04578..7fbb67021b5d9 100644
> --- a/drivers/net/ethernet/broadcom/bnge/bnge_filter.c
> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_filter.c

[ ... ]

> @@ -49,12 +177,38 @@ static struct bnge_l2_filter *bnge_lookup_l2_filter(struct bnge_net *bn,
>  	return NULL;
>  }
>  
> +__le64 bnge_lookup_l2_filter_rcu(struct bnge_net *bn,
> +				 struct bnge_l2_key *key,
> +				 u32 idx)
> +{
> +	__le64 id = BNGE_FLTR_ID_INVALID;
> +	struct bnge_l2_filter *fltr;
> +	struct hlist_head *head;
> +
> +	rcu_read_lock();
> +
> +	head = &bn->l2_fltr_hash_tbl[idx];
> +	hlist_for_each_entry_rcu(fltr, head, base.hlist) {
> +		struct bnge_l2_key *l2_key = &fltr->l2_key;
> +
> +		if (ether_addr_equal(l2_key->dst_mac_addr, key->dst_mac_addr) &&
> +		    l2_key->vlan == key->vlan) {
> +			id = fltr->base.filter_id;
                                          ^^^^^^^^^

[Severity: Low]

Can this plain load race with the store in bnge_hwrm_l2_filter_alloc()?

bnge_init_l2_filter() publishes the filter with hlist_add_head_rcu() while
filter_id is still BNGE_FLTR_ID_INVALID. The real ID is written later,
after the HWRM round trip (which can sleep). That write is a plain store,
and only the netdev instance lock is held:

bnge_hwrm_l2_filter_alloc() {
    ...
	if (!rc) {
		fltr->base.filter_id = resp->l2_filter_id;
		set_bit(BNGE_FLTR_VALID, &fltr->base.state);
	}
    ...
}

In the later patch "bnge: add aRFS flow steering ndo support", this helper
is called from bnge_rx_flow_steer(). That function runs in RX softirq and
holds neither the netdev lock nor ntp_fltr_lock.

The writer can run at the same time. bnge_cfg_rx_mode() calls
bnge_hwrm_set_vnic_filter() for secondary unicast MACs while the device
is open.

KCSAN should report this as a data race. BNGE can also be built for
32-bit, where the 64-bit load can tear. A value that is half INVALID and
half the real ID would pass this check in bnge_rx_flow_steer():

	filter_id = bnge_lookup_l2_filter_from_key(bn, &key);
	if (filter_id == BNGE_FLTR_ID_INVALID) {

The torn value would then be sent to firmware as req->l2_filter_id.

The aRFS commit message already says that a stale ID is either rejected
by firmware or aged out through rps_may_expire_flow(). A torn value would
end up in one of those two outcomes, so the effect is limited.

Would it make sense to use READ_ONCE() here, paired with WRITE_ONCE() in
bnge_hwrm_l2_filter_alloc()? The series already pairs them this way for
ntp_fltr_count.

> +			break;
> +		}
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928061307.1172344-1-vikas.gupta%40broadcom.com