Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 4 Apr 2021 02:48:47 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Oleksij Rempel <o.rempel@pengutronix.de>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-mips@vger.kernel.org
Subject: Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding
 database support
Message-ID: <20210403234847.jceg4ubljdq3g7n5@skbuf>
References: <20210403114848.30528-1-o.rempel@pengutronix.de>
 <20210403114848.30528-6-o.rempel@pengutronix.de>
 <YGiI3JtqU7Ezlbxb@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YGiI3JtqU7Ezlbxb@lunn.ch>
Precedence: bulk

On Sat, Apr 03, 2021 at 05:25:16PM +0200, Andrew Lunn wrote:
> > +static int ar9331_sw_port_fdb_rmw(struct ar9331_sw_priv *priv,
> > +				  const unsigned char *mac,
> > +				  u8 port_mask_set,
> > +				  u8 port_mask_clr)
> > +{
> > +	port_mask = FIELD_GET(AR9331_SW_AT_DES_PORT, f2);
> > +	status = FIELD_GET(AR9331_SW_AT_STATUS, f2);
> > +	if (status > 0 && status < AR9331_SW_AT_STATUS_STATIC) {
> > +		dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x\n",
> > +				    __func__, port_mask);
> > +
> > +		if (port_mask_set && port_mask_set != port_mask)
> > +			dev_err_ratelimited(priv->dev, "%s: found existing dynamic entry on %x, replacing it with static on %x\n",
> > +					    __func__, port_mask, port_mask_set);
> > +		port_mask = 0;
> > +	} else if (!status && !port_mask_set) {
> > +		return 0;
> > +	}
> 
> As a generate rule of thumb, use rate limiting where you have no
> control of the number of prints, e.g. it is triggered by packet
> processing, and there is potentially a lot of them, which could DOS
> the box by a remote or unprivileged attacker.
> 
> FDB changes should not happen often. Yes, root my be able to DOS the
> box by doing bridge fdb add commands in a loop, but only root should
> be able to do that.

+1
The way I see it, rate limiting should only be done when the user can't
stop the printing from spamming the console, and they just go "argh,
kill it with fire!!!" and throw the box away. As a side note, most of
the time when I can't stop the kernel from printing in a loop, the rate
limit isn't enough to stop me from wanting to throw the box out the
window, but I digress.

> Plus, i'm not actually sure we should be issuing warnings here. What
> does the bridge code do in this case? Is it silent and just does it,
> or does it issue a warning?

:D

What Oleksij doesn't know, I bet, is that he's using the bridge bypass
commands:

bridge fdb add dev lan0 00:01:02:03:04:05

That is the deprecated way of managing FDB entries, and has several
disadvantages such as:
- the bridge software FDB never gets updated with this entry, so other
  drivers which might be subscribed to DSA's FDB (imagine a non-DSA
  driver having the same logic as our ds->assisted_learning_on_cpu_port)
  will never see these FDB entries
- you have to manage duplicates yourself

The correct way to install a static bridge FDB entry is:

bridge fdb add dev lan0 00:01:02:03:04:05 master static

That will error out on duplicates for you.

From my side I would even go as far as deleting the bridge bypass
operations (.ndo_fdb_add and .ndo_fdb_del). The more integration we add
between DSA and the bridge/switchdev, the worse it will be when users
just keep using the bridge bypass. To start that process, I guess we
should start emitting a deprecation warning and then pull the trigger
after a few kernel release cycles.

> > +
> > +	port_mask_new = port_mask & ~port_mask_clr;
> > +	port_mask_new |= port_mask_set;
> > +
> > +	if (port_mask_new == port_mask &&
> > +	    status == AR9331_SW_AT_STATUS_STATIC) {
> > +		dev_info(priv->dev, "%s: no need to overwrite existing valid entry on %x\n",
> > +				    __func__, port_mask_new);
> 
> This one should probably be dev_dbg().

Or deleted, along with the overwrite logic.