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

[Vikas Gupta]

Since there are valid review comments by Sashiko in this series , I'll
prepare and submit a v5.

Thanks,
Vikas

On Thu, Oct 1, 2026 at 2:44 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature