netdev - Re: [PATCH net 1/2] net: switchdev: Add helper to check if an object event is pending

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240131133406.v6zk33j43wy2j7fa@skbuf>
Date: Wed, 31 Jan 2024 15:34:06 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, roopa@...dia.com,
	razor@...ckwall.org, bridge@...ts.linux.dev, netdev@...r.kernel.org,
	jiri@...nulli.us, ivecera@...hat.com
Subject: Re: [PATCH net 1/2] net: switchdev: Add helper to check if an object
 event is pending

On Wed, Jan 31, 2024 at 01:35:43PM +0100, Tobias Waldekranz wrote:
> When adding/removing a port to/from a bridge, the port must be brought
> up to speed with the current state of the bridge. This is done by
> replaying all relevant events, directly to the port in question.
> 
> In some situations, specifically when replaying the MDB, this process
> may race against new events that are generated concurrently.
> 
> So the bridge must ensure that the event is not already pending on the
> deferred queue. switchdev_port_obj_is_deferred answers this question.
> 
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>

I don't see great value in splitting this patch in (1) unused helpers
(2) actual fix that uses them. Especially since it creates confusion -
it is nowhere made clear in this commit message that it is just
preparatory work.

> ---
>  include/net/switchdev.h   |  3 ++
>  net/switchdev/switchdev.c | 61 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index a43062d4c734..538851a93d9e 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -308,6 +308,9 @@ void switchdev_deferred_process(void);
>  int switchdev_port_attr_set(struct net_device *dev,
>  			    const struct switchdev_attr *attr,
>  			    struct netlink_ext_ack *extack);
> +bool switchdev_port_obj_is_deferred(struct net_device *dev,
> +				    enum switchdev_notifier_type nt,
> +				    const struct switchdev_obj *obj);

I think this is missing a shim definition for when CONFIG_NET_SWITCHDEV
is disabled.

>  int switchdev_port_obj_add(struct net_device *dev,
>  			   const struct switchdev_obj *obj,
>  			   struct netlink_ext_ack *extack);
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 5b045284849e..40bb17c7fdbf 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -19,6 +19,35 @@
>  #include <linux/rtnetlink.h>
>  #include <net/switchdev.h>
>  
> +static bool switchdev_obj_eq(const struct switchdev_obj *a,
> +			     const struct switchdev_obj *b)
> +{
> +	const struct switchdev_obj_port_vlan *va, *vb;
> +	const struct switchdev_obj_port_mdb *ma, *mb;
> +
> +	if (a->id != b->id || a->orig_dev != b->orig_dev)
> +		return false;
> +
> +	switch (a->id) {
> +	case SWITCHDEV_OBJ_ID_PORT_VLAN:
> +		va = SWITCHDEV_OBJ_PORT_VLAN(a);
> +		vb = SWITCHDEV_OBJ_PORT_VLAN(b);
> +		return va->flags == vb->flags &&
> +			va->vid == vb->vid &&
> +			va->changed == vb->changed;
> +	case SWITCHDEV_OBJ_ID_PORT_MDB:
> +	case SWITCHDEV_OBJ_ID_HOST_MDB:
> +		ma = SWITCHDEV_OBJ_PORT_MDB(a);
> +		mb = SWITCHDEV_OBJ_PORT_MDB(b);
> +		return ma->vid == mb->vid &&
> +			!memcmp(ma->addr, mb->addr, sizeof(ma->addr));

ether_addr_equal().

> +	default:
> +		break;

Does C allow you to not return anything here?

> +	}
> +
> +	BUG();
> +}
> +
>  static LIST_HEAD(deferred);
>  static DEFINE_SPINLOCK(deferred_lock);
>  
> @@ -307,6 +336,38 @@ int switchdev_port_obj_del(struct net_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(switchdev_port_obj_del);
>  
> +bool switchdev_port_obj_is_deferred(struct net_device *dev,
> +				    enum switchdev_notifier_type nt,
> +				    const struct switchdev_obj *obj)

A kernel-doc comment would be great. It looks like it's not returning
whether the port object is deferred, but whether the _action_ given by
@nt on the @obj is deferred. This further distinguishes between deferred
additions and deferred removals.

> +{
> +	struct switchdev_deferred_item *dfitem;
> +	bool found = false;
> +
> +	ASSERT_RTNL();

Why does rtnl_lock() have to be held? To fully allow switchdev_deferred_process()
to run to completion, aka its dfitem->func() as well?

> +
> +	spin_lock_bh(&deferred_lock);
> +
> +	list_for_each_entry(dfitem, &deferred, list) {
> +		if (dfitem->dev != dev)
> +			continue;
> +
> +		if ((dfitem->func == switchdev_port_obj_add_deferred &&
> +		     nt == SWITCHDEV_PORT_OBJ_ADD) ||
> +		    (dfitem->func == switchdev_port_obj_del_deferred &&
> +		     nt == SWITCHDEV_PORT_OBJ_DEL)) {
> +			if (switchdev_obj_eq((const void *)dfitem->data, obj)) {
> +				found = true;
> +				break;
> +			}
> +		}
> +	}
> +
> +	spin_unlock_bh(&deferred_lock);
> +
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(switchdev_port_obj_is_deferred);
> +
>  static ATOMIC_NOTIFIER_HEAD(switchdev_notif_chain);
>  static BLOCKING_NOTIFIER_HEAD(switchdev_blocking_notif_chain);
>  
> -- 
> 2.34.1
>