Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support
> > 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
I was actually meaning a pure software bridge, with unaccelerated
interfaces. It has a dynamic MAC address in its tables, and the user
adds a static. Ideally, we want the same behaviour.
And i think the answer is:
static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
const unsigned char *addr, u16 vid)
{
struct net_bridge_fdb_entry *fdb;
if (!is_valid_ether_addr(addr))
return -EINVAL;
fdb = br_fdb_find(br, addr, vid);
if (fdb) {
/* it is okay to have multiple ports with same
* address, just use the first one.
*/
if (test_bit(BR_FDB_LOCAL, &fdb->flags))
return 0;
br_warn(br, "adding interface %s with same address as a
received packet (addr:%pM, vlan:%u)\n",
source ? source->dev->name : br->dev->name, addr, vid);
fdb_delete(br, fdb, true);
}
fdb = fdb_create(br, source, addr, vid,
BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
if (!fdb)
return -ENOMEM;
fdb_add_hw_addr(br, addr);
fdb_notify(br, fdb, RTM_NEWNEIGH, true);
return 0;
}
So it looks like it warns and then replaces the dynamic entry.
So having the DSA driver also warn is maybe O.K. Having said that, i
don't think any other DSA driver does.
Andrew
Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support
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.
Re: [PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support
> +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.
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?
> +
> + 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().
Andrew
[PATCH net-next v1 5/9] net: dsa: qca: ar9331: add forwarding database support
This switch provides simple address resolution table, without VLAN or
multicast specific information.
With this patch we are able now to read, modify unicast and mulicast
addresses.
Signed-off-by: Oleksij Rempel
---
drivers/net/dsa/qca/ar9331.c | 356 +++
1 file changed, 356 insertions(+)
diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index a3de3598fbf5..4a98f14f31f4 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -66,6 +66,47 @@
#define AR9331_SW_REG_GLOBAL_CTRL 0x30
#define AR9331_SW_GLOBAL_CTRL_MFS_MGENMASK(13, 0)
+/* Size of the address resolution table (ARL) */
+#define AR9331_SW_NUM_ARL_RECORDS 1024
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION0 0x50
+#define AR9331_SW_AT_ADDR_BYTES4 GENMASK(31, 24)
+#define AR9331_SW_AT_ADDR_BYTES5 GENMASK(23, 16)
+#define AR9331_SW_AT_FULL_VIO BIT(12)
+#define AR9331_SW_AT_PORT_NUM GENMASK(11, 8)
+#define AR9331_SW_AT_FLUSH_STATIC_EN BIT(4)
+#define AR9331_SW_AT_BUSY BIT(3)
+#define AR9331_SW_AT_FUNC GENMASK(2, 0)
+#define AR9331_SW_AT_FUNC_NOP 0
+#define AR9331_SW_AT_FUNC_FLUSH_ALL1
+#define AR9331_SW_AT_FUNC_LOAD_ENTRY 2
+#define AR9331_SW_AT_FUNC_PURGE_ENTRY 3
+#define AR9331_SW_AT_FUNC_FLUSH_ALL_UNLOCKED 4
+#define AR9331_SW_AT_FUNC_FLUSH_PORT 5
+#define AR9331_SW_AT_FUNC_GET_NEXT 6
+#define AR9331_SW_AT_FUNC_FIND_MAC 7
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION1 0x54
+#define AR9331_SW_AT_ADDR_BYTES0 GENMASK(31, 24)
+#define AR9331_SW_AT_ADDR_BYTES1 GENMASK(23, 16)
+#define AR9331_SW_AT_ADDR_BYTES2 GENMASK(15, 8)
+#define AR9331_SW_AT_ADDR_BYTES3 GENMASK(7, 0)
+
+#define AR9331_SW_REG_ADDR_TABLE_FUNCTION2 0x58
+#define AR9331_SW_AT_COPY_TO_CPU BIT(26)
+#define AR9331_SW_AT_REDIRECT_TOCPUBIT(25)
+#define AR9331_SW_AT_LEAKY_EN BIT(24)
+#define AR9331_SW_AT_STATUSGENMASK(19, 16)
+#define AR9331_SW_AT_STATUS_EMPTY 0
+/* STATUS values from 7 to 1 are different aging levels */
+#define AR9331_SW_AT_STATUS_STATIC 0xf
+
+#define AR9331_SW_AT_SA_DROP_ENBIT(14)
+#define AR9331_SW_AT_MIRROR_EN BIT(13)
+#define AR9331_SW_AT_PRIORITY_EN BIT(12)
+#define AR9331_SW_AT_PRIORITY GENMASK(11, 10)
+#define AR9331_SW_AT_DES_PORT GENMASK(5, 0)
+
#define AR9331_SW_REG_ADDR_TABLE_CTRL 0x5c
#define AR9331_SW_AT_ARP_ENBIT(20)
#define AR9331_SW_AT_LEARN_CHANGE_EN BIT(18)
@@ -266,6 +307,12 @@ struct ar9331_sw_port {
struct spinlock stats_lock;
};
+struct ar9331_sw_fdb {
+ u8 port_mask;
+ u8 aging;
+ u8 mac[ETH_ALEN];
+};
+
struct ar9331_sw_priv {
struct device *dev;
struct dsa_switch ds;
@@ -765,6 +812,309 @@ static void ar9331_get_stats64(struct dsa_switch *ds, int
port,
spin_unlock(&p->stats_lock);
}
+static int ar9331_sw_fdb_wait(struct ar9331_sw_priv *priv, u32 *f0)
+{
+ struct regmap *regmap = priv->regmap;
+
+ return regmap_read_poll_timeout(regmap,
+ AR9331_SW_REG_ADDR_TABLE_FUNCTION0,
+ *f0, !(*f0 & AR9331_SW_AT_BUSY),
+ 10, 2000);
+}
+
+static int ar9331_sw_port_fdb_write(struct ar9331_sw_priv *priv,
+ u32 f0, u32 f1, u32 f2)
+{
+ struct regmap *regmap = priv->regmap;
+ int ret;
+
+ ret = regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION2, f2);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION1, f1);
+ if (ret)
+ return ret;
+
+ return regmap_write(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0, f0);
+}
+
+static int ar9331_sw_fdb_next(struct ar9331_sw_priv *priv,
+ struct ar9331_sw_fdb *fdb, int port)
+{
+ struct regmap *regmap = priv->regmap;
+ unsigned int status, ports;
+ u32 f0, f1, f2;
+ int ret;
+
+ /* Keep AT_ADDR_BYTES4/5 to search next entry after current */
+ ret = regmap_update_bits(regmap, AR9331_SW_REG_ADDR_TABLE_FUNCTION0,
+AR9331_SW_AT_FUNC | AR9331_SW_AT_BUSY,
+AR9331_SW_AT_BUSY |
+FIELD_PREP(AR9331_SW_AT_FUNC,
+ AR9331_SW_AT_FUNC_GET_NEXT));
+ if (ret)
+ return ret;
+
+ ret = ar9331_sw_fdb_wait(priv, &f0);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(regmap, AR9331_SW_REG_ADDR_T
