Re: [net-next,v5,2/2] net: dsa: mv88e6xxx: Add partial support for TCAM entries

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: mv88e6xxx: Add partial support for TCAM entries

This patch adds partial Ternary Content Addressable Memory (TCAM) for
the mv88e6390 and mv88e6393 family of switches. TCAM entries allow the
switch to match the first 48 or 96 bytes of a frame and take actions on
matched frames. The patch introduces a subset of the available TCAM
functionality for matching on ip addresses/protocol and trapping to the
cpu.

> diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
> index dd961081d631..b0b08c6f159c 100644
> --- a/drivers/net/dsa/mv88e6xxx/Makefile
> +++ b/drivers/net/dsa/mv88e6xxx/Makefile
> @@ -21,6 +21,8 @@ mv88e6xxx-objs += serdes.o
>  mv88e6xxx-objs += smi.o
>  mv88e6xxx-objs += switchdev.o
>  mv88e6xxx-objs += trace.o
> +mv88e6xxx-objs += tcflower.o
> +mv88e6xxx-objs += tcam.o

[ ... ]

> diff --git a/drivers/net/dsa/mv88e6xxx/tcam.c b/drivers/net/dsa/mv88e6xxx/tcam.c
> new file mode 100644
> index 000000000000..f56cc7e0c00f
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/tcam.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Marvell 88E6xxx Switch TCAM support
> + *
> + * Copyright (c) 2026 Luminex Network Intelligence
> + */

[ ... ]

> +/* TCAM keys register 1 */
> +#define MV88E6XXX_TCAM_KEYS1			0x02
> +#define MV88E6XXX_TCAM_KEYS1_FT_MASK		0xC000
> +#define MV88E6XXX_TCAM_KEYS1_SPV_MASK		0x0007
> +
> +/* TCAM keys register 1 */
> +#define MV88E6XXX_TCAM_KEYS2			0x03

This isn't a bug, but there's a copy-paste error in the comment. The
comment says "/* TCAM keys register 1 */" but the define is KEYS2 for
register 0x03.

> +#define MV88E6XXX_TCAM_KEYS2_SPV_MASK		0x00ff

[ ... ]

> +/* insert tcam entry in ordered list and move existing entries if necessary */
> +static int mv88e6xxx_tcam_insert_entry(struct mv88e6xxx_chip *chip,
> +				       struct mv88e6xxx_tcam_entry *entry)
> +{
> +	struct mv88e6xxx_tcam_entry *elem;
> +	struct list_head *hpos;
> +	int err;
> +
> +	list_for_each_prev(hpos, &chip->tcam.entries) {
> +		u8 move_idx;
> +
> +		elem = list_entry(hpos, struct mv88e6xxx_tcam_entry, list);
> +		if (entry->prio >= elem->prio)
> +			break;
> +
> +		move_idx = elem->hw_idx + 1;
> +
> +		err = mv88e6xxx_tcam_flush_entry(chip, move_idx);
> +		if (err)
> +			return err;
> +
> +		err = chip->info->ops->tcam_ops->entry_add(chip, elem,
> +							   move_idx);
> +		if (err)
> +			return err;
> +
> +		elem->hw_idx = move_idx;
> +	}

Can hw_idx overflow when the TCAM is full? The hw_idx field in
struct mv88e6xxx_tcam_entry is u8, and the code computes move_idx as
elem->hw_idx + 1 without checking if it will wrap from 255 to 0.

When a user repeatedly adds tc filters beyond the hardware TCAM capacity
(typically 256 entries for mv88e6390/mv88e6393), the index could wrap and
cause mv88e6xxx_tcam_flush_entry() and entry_add() to overwrite existing
entries at index 0.

Looking at the add path, there's no entry count limit:

mv88e6xxx_cls_flower_add() in tcflower.c
  -> mv88e6xxx_tcam_entry_add() here in tcam.c
    -> mv88e6xxx_tcam_insert_entry()

The struct mv88e6xxx_info has no num_tcam_entries field, and no -ENOSPC
is returned when the table is full. This is reachable from userspace via
repeated 'tc filter add' commands.

> +
> +	if (list_is_head(hpos, &chip->tcam.entries)) {
> +		entry->hw_idx = 0;
> +	} else {
> +		elem = list_entry(hpos, struct mv88e6xxx_tcam_entry, list);
> +		entry->hw_idx = elem->hw_idx + 1;
> +	}
> +	list_add(&entry->list, hpos);
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/net/dsa/mv88e6xxx/tcflower.c b/drivers/net/dsa/mv88e6xxx/tcflower.c
> new file mode 100644
> index 000000000000..dcbfe0beb5cb
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/tcflower.c
> @@ -0,0 +1,159 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Marvell 88E6xxx Switch flower support
> + *
> + * Copyright (c) 2026 Luminex Network Intelligence
> + */

[ ... ]

> +int mv88e6xxx_cls_flower_del(struct dsa_switch *ds, int port,
> +			     struct flow_cls_offload *cls, bool ingress)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_tcam_entry *entry;
> +	int err = 0;
> +
> +	mv88e6xxx_reg_lock(chip);
> +	entry = mv88e6xxx_tcam_entry_find(chip, cls->cookie);
> +
> +	if (entry) {
> +		err = mv88e6xxx_tcam_entry_del(chip, entry);
> +		kfree(entry);
> +	}

Does this create a use-after-free when mv88e6xxx_tcam_entry_del() fails?

Looking at mv88e6xxx_tcam_entry_del() in tcam.c, it only calls
list_del(&entry->list) at the END of the function. If the initial
mv88e6xxx_tcam_flush_entry() call fails, the function returns early
WITHOUT removing the entry from chip->tcam.entries:

drivers/net/dsa/mv88e6xxx/tcam.c:mv88e6xxx_tcam_entry_del() {
	...
	err = mv88e6xxx_tcam_flush_entry(chip, entry->hw_idx);
	if (err)
		return err;  // early return without list_del

	/* move entries that come after the deleted entry forward */
	list_for_each_entry_continue(elem, &chip->tcam.entries, list) {
		...
	}

	list_del(&entry->list);
	return err;
}

The caller then unconditionally calls kfree(entry), leaving a dangling
pointer in the linked list. Any subsequent traversal of chip->tcam.entries
(e.g., via mv88e6xxx_tcam_entry_find or mv88e6xxx_tcam_insert_entry) will
dereference freed memory.

The fix should either not kfree the entry when mv88e6xxx_tcam_entry_del
fails, or ensure mv88e6xxx_tcam_entry_del always calls list_del before
returning, even on error.

> +	mv88e6xxx_reg_unlock(chip);
> +	return  err;
> +}
-- 
pw-bot: cr