Re: [PATCH net-next 1/2] net: bridge: switchdev: add br_switchdev_fdb_find_port()

[netdev-bot+sashiko]

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

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

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] br_switchdev_fdb_find_port() has a kernel-doc that says it
  returns NULL when "switchdev is not notified of" the entry.
- [Low] Concern: br_switchdev_fdb_find_port() is exported, declared in
  include/linux/if_bridge.h and given a stub there, but nothing in the…

--

net: bridge: switchdev: add br_switchdev_fdb_find_port()

This commit adds br_switchdev_fdb_find_port(), an exported RCU lookup
that returns the port of a non-local bridge FDB entry that switchdev is
notified of. The filter in br_switchdev_fdb_notify() moves into a new
helper, br_switchdev_fdb_notifies(), and both functions use it.

> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
> index 75673b8bffcb8..e050b7e698fc9 100644
> --- a/include/linux/if_bridge.h
> +++ b/include/linux/if_bridge.h
> @@ -201,6 +201,19 @@ static inline int br_mst_get_state(const struct net_device *dev, u16 msti,
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_BRIDGE) && IS_ENABLED(CONFIG_NET_SWITCHDEV)
> +struct net_device *br_switchdev_fdb_find_port(const struct net_device *br_dev,
> +					      const unsigned char *addr,
> +					      u16 vid);

[Severity: Low]
This isn't a bug, but nothing in this patch calls the newly exported
br_switchdev_fdb_find_port().

The only user seems to come in the next patch of the series, "net: dsa:
fix host fdb refcount when a bridge fdb entry roams". That patch calls it
from dsa_user_bridge_host_fdb_wanted() in net/dsa/user.c:

    brport = br_switchdev_fdb_find_port(bridge->dev, addr, vid);

Could the commit message name that consumer?

> +#else

[ ... ]

> diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
> index 10f3aea6a1f82..02ca1cbcfede0 100644
> --- a/net/bridge/br_switchdev.c
> +++ b/net/bridge/br_switchdev.c

[ ... ]

> @@ -156,9 +152,58 @@ br_switchdev_fdb_notify(struct net_bridge *br,
>  	 * Drivers don't know how to deal with these, so don't notify them to
>  	 * avoid confusing them.
>  	 */
> -	if (test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) &&
> -	    !test_bit(BR_FDB_STATIC, &fdb->flags) &&
> -	    !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
> +	return !test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags) ||
> +	       test_bit(BR_FDB_STATIC, &fdb->flags) ||
> +	       test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
> +}
> +
> +/**
> + * br_switchdev_fdb_find_port - find the port of an entry switchdev knows of
> + * @br_dev: the bridge device
> + * @addr: the MAC address of the entry
> + * @vid: the VLAN of the entry
> + *
> + * Must be called under RCU; the device is valid until the read-side critical
> + * section ends.
> + *
> + * Return: the port of the non-local entry for @addr and @vid, or NULL if
> + * there is none or switchdev is not notified of it.
> + */
> +struct net_device *br_switchdev_fdb_find_port(const struct net_device *br_dev,
> +					      const unsigned char *addr,
> +					      u16 vid)
> +{
> +	const struct net_bridge_port *p;
> +	struct net_bridge_fdb_entry *f;
> +
> +	RCU_LOCKDEP_WARN(!rcu_read_lock_held(), "suspicious use outside RCU");
> +
> +	if (!netif_is_bridge_master(br_dev))
> +		return NULL;
> +
> +	f = br_fdb_find_rcu(netdev_priv(br_dev), addr, vid);
> +	if (!f || test_bit(BR_FDB_LOCAL, &f->flags) ||
> +	    !br_switchdev_fdb_notifies(f))
> +		return NULL;
> +
> +	/* learned by a switchdev driver, which is not notified back */
> +	if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
> +	    !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
> +		return NULL;

[Severity: Medium]
Does this filter match the kernel-doc above? The kernel-doc says NULL is
returned when "switchdev is not notified of" the entry.

The commit message also says the caller uses it to "ignore the entries
whose deletion it will not be notified of". Several cases seem to
contradict both statements.

The comment "learned by a switchdev driver, which is not notified back"
only holds for the addition. br_switchdev_fdb_notifies() returns true for
BR_FDB_ADDED_BY_EXT_LEARN entries. So a flush, a delete-by-port or
__br_fdb_delete() still sends SWITCHDEV_FDB_DEL_TO_DEVICE for them,
because those paths go through fdb_delete(br, f, true).

A software-learned entry has already had SWITCHDEV_FDB_ADD_TO_DEVICE
sent. Suppose a driver then reports the same addr/vid with
SWITCHDEV_FDB_ADD_TO_BRIDGE. The existing-entry branch of
br_fdb_external_learn_add() does:

    if (test_and_set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {

It leaves BR_FDB_ADDED_BY_USER clear. This helper then returns NULL for
an entry that switchdev already knows about.

Replay also skips the filter. br_switchdev_fdb_replay_one() notifies
every fdb_list entry without calling br_switchdev_fdb_notifies():

    br_switchdev_fdb_populate(br, &item, fdb, ctx);

    err = nb->notifier_call(nb, action, &item);

Locked entries and user "master dynamic" entries are therefore notified
on replay, but this helper reports them as not notified.

The opposite case looks more concerning. A user-added extern_learn entry
(bridge fdb add ... master extern_learn) goes through
__br_fdb_add()->br_fdb_external_learn_add(..., false, true). It gets
BR_FDB_ADDED_BY_USER | BR_FDB_ADDED_BY_EXT_LEARN and passes this filter.

Any switchdev driver can then delete that entry without a DEL_TO_DEVICE
notification:

br_switchdev_event(SWITCHDEV_FDB_DEL_TO_BRIDGE)
  br_fdb_external_learn_del(br, p, addr, vid, false)
    fdb_delete(br, fdb, false)

br_fdb_external_learn_del() only checks BR_FDB_ADDED_BY_EXT_LEARN, not
the port:

    if (fdb && test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
        fdb_delete(br, fdb, swdev_notify);

With the DSA consumer from the next patch, consider a user port that
holds a host address for such an entry on a foreign port. Would it ever
get an event telling it to drop that address? The address seems to stay
on the CPU port until a later event for that addr/vid, or until the port
leaves the bridge.

Should the kernel-doc and commit message describe what the filter really
guarantees, or should the filter change to match the stated contract?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1794134495.git.julius%40bairaktaris.de