linux-kernel - Re: [PATCH RFC net-next 09/10] net: dsa: mv88e6xxx: Enable mc flood for mrouter port

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Wed, 3 Apr 2024 16:49:21 +0100
From: Simon Horman <horms@...nel.org>
To: Joseph Huang <Joseph.Huang@...min.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Roopa Prabhu <roopa@...dia.com>,
	Nikolay Aleksandrov <razor@...ckwall.org>,
	Linus Lüssing <linus.luessing@...3.blue>,
	linux-kernel@...r.kernel.org, bridge@...ts.linux.dev
Subject: Re: [PATCH RFC net-next 09/10] net: dsa: mv88e6xxx: Enable mc flood
 for mrouter port

On Mon, Apr 01, 2024 at 08:11:08PM -0400, Joseph Huang wrote:
> When a port turns into an mrouter port, enable multicast flooding
> on that port even if multicast flooding is disabled by user config. This
> is necessary so that in a distributed system, the multicast packets
> can be forwarded to the Querier when the multicast source is attached
> to a Non-Querier bridge.
> 
> Consider the following scenario:
> 
>                  +--------------------+
>                  |                    |
>                  |      Snooping      |    +------------+
>                  |      Bridge 1      |----| Listener 1 |
>                  |     (Querier)      |    +------------+
>                  |                    |
>                  +--------------------+
>                            |
>                            |
>                  +--------------------+
>                  |    | mrouter |     |
> +-----------+    |    +---------+     |
> | MC Source |----|      Snooping      |
> +-----------|    |      Bridge 2      |
>                  |    (Non-Querier)   |
>                  +--------------------+
> 
> In this scenario, Listener 1 will never receive multicast traffic
> from MC Source if multicast flooding is disabled on the mrouter port on
> Snooping Bridge 2.
> 
> Signed-off-by: Joseph Huang <Joseph.Huang@...min.com>

..

> @@ -6849,11 +6864,28 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
>  
>  	if (flags.mask & BR_MCAST_FLOOD) {
>  		bool multicast = !!(flags.val & BR_MCAST_FLOOD);
> +		struct mv88e6xxx_bridge *mv_bridge;
> +		struct mv88e6xxx_port *p;
> +		bool mrouter;
>  
> -		err = chip->info->ops->port_set_mcast_flood(chip, port,
> -							    multicast);
> -		if (err)
> -			goto out;
> +		mv_bridge = mv88e6xxx_bridge_by_port(chip, port);
> +		if (!mv_bridge)
> +			return -EINVAL;

I think that mv88e6xxx_reg_unlock(chip) is needed here.
So perhaps (completely untested!):

		if (!mv_bridge) {
			err = -EINVAL;
			goto out;
		}

Flagged by Smatch

> +
> +		p = &chip->ports[port];
> +		mrouter = !!(mv_bridge->mrouter_ports & BIT(port));
> +
> +		if (!mrouter) {
> +			err = chip->info->ops->port_set_mcast_flood(chip, port,
> +								    multicast);
> +			if (err)
> +				goto out;
> +		}
> +
> +		if (multicast)
> +			p->flags |= MV88E6XXX_PORT_FLAG_MC_FLOOD;
> +		else
> +			p->flags &= ~MV88E6XXX_PORT_FLAG_MC_FLOOD;
>  	}
>  
>  	if (flags.mask & BR_BCAST_FLOOD) {
> @@ -6883,6 +6915,51 @@ static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
>  	return err;
>  }
>  
> +static int mv88e6xxx_port_mrouter(struct dsa_switch *ds, int port,
> +				  bool mrouter,
> +				  struct netlink_ext_ack *extack)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_bridge *mv_bridge;
> +	struct mv88e6xxx_port *p;
> +	bool old_mrouter;
> +	bool mc_flood;
> +	int err;
> +
> +	if (!chip->info->ops->port_set_mcast_flood)
> +		return -EOPNOTSUPP;
> +
> +	mv_bridge = mv88e6xxx_bridge_by_port(chip, port);
> +	if (!mv_bridge)
> +		return -EINVAL;
> +
> +	old_mrouter = !!(mv_bridge->mrouter_ports & BIT(port));
> +	if (mrouter == old_mrouter)
> +		return 0;
> +
> +	p = &chip->ports[port];
> +	mc_flood = !!(p->flags & MV88E6XXX_PORT_FLAG_MC_FLOOD);
> +
> +	mv88e6xxx_reg_lock(chip);
> +
> +	if (!mc_flood) {
> +		err = chip->info->ops->port_set_mcast_flood(chip, port,
> +							    mrouter);
> +		if (err)
> +			goto out;
> +	}
> +
> +	if (mrouter)
> +		mv_bridge->mrouter_ports |= BIT(port);
> +	else
> +		mv_bridge->mrouter_ports &= ~BIT(port);
> +
> +out:
> +	mv88e6xxx_reg_unlock(chip);

If mc_flood is true then err is uninitialised here.

Flagged by clang-17 W=1 build, and Smatch.

> +
> +	return err;
> +}
> +
>  static bool mv88e6xxx_lag_can_offload(struct dsa_switch *ds,
>  				      struct dsa_lag lag,
>  				      struct netdev_lag_upper_info *info,

..