Re: [PATCH net-next v5 1/3] net: ti: icssm-prueth: Adds helper functions to configure and maintain FDB

[Parvathi Pudi <parvathi@couthit.com>]

Hi,

> On Thu, Nov 13, 2025 at 03:40:21PM +0530, Parvathi Pudi wrote:
> 
> ...
> 
>> +static u16 icssm_prueth_sw_fdb_find_open_slot(struct fdb_tbl *fdb_tbl)
>> +{
>> +	u8 flags;
>> +	u16 i;
>> +
>> +	for (i = 0; i < FDB_MAC_TBL_MAX_ENTRIES; i++) {
>> +		flags = readb(&fdb_tbl->mac_tbl_a->mac_tbl_entry[i].flags);
>> +		if (!(flags & FLAG_ACTIVE))
>> +			break;
> 
> Hi Parvathi, all,
> 
> If the condition above is never met....
> 
>> +	}
>> +
>> +	return i;
> 
> Then FDB_MAC_TBL_MAX_ENTRIES will be returned here.
> Which is written to bkt_info->bucket_idx by
> icssm_prueth_sw_insert_fdb_entry()...
> 
>> +}
>> +
>> +static int
>> +icssm_prueth_sw_find_fdb_insert(struct fdb_tbl *fdb, struct prueth *prueth,
>> +				struct fdb_index_tbl_entry __iomem *bkt_info,
>> +				const u8 *mac, const u8 port)
>> +{
>> +	struct fdb_mac_tbl_array __iomem *mac_tbl = fdb->mac_tbl_a;
>> +	struct fdb_mac_tbl_entry __iomem *e;
>> +	u8 mac_from_hw[ETH_ALEN];
>> +	u16 bucket_entries;
>> +	u16 mac_tbl_idx;
>> +	int i, ret;
>> +	s8 cmp;
>> +
>> +	mac_tbl_idx = readw(&bkt_info->bucket_idx);
> 
> That value will be read here...
> 
>> +	bucket_entries = readw(&bkt_info->bucket_entries);
>> +
>> +	for (i = 0; i < bucket_entries; i++, mac_tbl_idx++) {
>> +		e = &mac_tbl->mac_tbl_entry[mac_tbl_idx];
> 
> And used here, without any bounds checking.
> 
> But if mac_tbl_idx is FDB_MAC_TBL_MAX_ENTRIES then
> the access to mac_tble_entry will overflow, as
> it only has FDB_MAC_TBL_MAX_ENTRIES elements.
> 
> This, and most of my review points for this patch set were
> flagged by Claude Code with https://github.com/masoncl/review-prompts/
> 
> ...

We have verified and this case will never hit since we have a if condition
in the very beginning of the "icssm_prueth_sw_insert_fdb_entry()" function to
return if the table is full as highlighted below and all the entries are active.

  If (fdb->total_entries == FDB_MAC_TBL_MAX_ENTRIES)
        return -ENOMEM;

> 
>> +static int icssm_prueth_sw_insert_fdb_entry(struct prueth_emac *emac,
>> +					    const u8 *mac, u8 is_static)
>> +{
>> +	struct fdb_index_tbl_entry __iomem *bucket_info;
>> +	struct fdb_mac_tbl_entry __iomem *mac_info;
>> +	struct prueth *prueth = emac->prueth;
>> +	struct prueth_emac *other_emac;
>> +	enum prueth_port other_port_id;
>> +	u8 hash_val, mac_tbl_idx;
>> +	struct fdb_tbl *fdb;
>> +	u8 flags;
>> +	u16 val;
>> +	s16 ret;
>> +	int err;
>> +
>> +	fdb = prueth->fdb_tbl;
>> +	other_port_id = (emac->port_id == PRUETH_PORT_MII0) ?
>> +			 PRUETH_PORT_MII1 : PRUETH_PORT_MII0;
>> +
>> +	other_emac = prueth->emac[other_port_id - 1];
>> +
>> +	if (fdb->total_entries == FDB_MAC_TBL_MAX_ENTRIES)
>> +		return -ENOMEM;
> 
> Access to total_entries outside of icssm_prueth_sw_fdb_spin_lock.
> Seems racy here. Likewise for the similar check in
> icssm_prueth_sw_delete_fdb_entry().
> 
> Flagged by Claude Code with https://github.com/masoncl/review-prompts/

We are using icssm_prueth_sw_fdb_spin_lock() to control access between
PRU and CPU. We have kept in and around the sections where PRU writes it. 

Sure, we will review and move the spin lock to protect even the read accesses.

> 
>> +
>> +	if (ether_addr_equal(mac, emac->mac_addr) ||
>> +	    ether_addr_equal(mac, other_emac->mac_addr)) {
>> +		/* Don't insert fdb of own mac addr */
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Get the bucket that the mac belongs to */
>> +	hash_val = icssm_prueth_sw_fdb_hash(mac);
>> +	bucket_info = FDB_IDX_TBL_ENTRY(hash_val);
>> +
>> +	if (!readw(&bucket_info->bucket_entries)) {
>> +		mac_tbl_idx = icssm_prueth_sw_fdb_find_open_slot(fdb);
>> +		writew(mac_tbl_idx, &bucket_info->bucket_idx);
>> +	}
>> +
>> +	ret = icssm_prueth_sw_find_fdb_insert(fdb, prueth, bucket_info, mac,
>> +					      emac->port_id - 1);
>> +	if (ret < 0)
>> +		/* mac is already in fdb table */
>> +		return 0;
>> +
>> +	mac_tbl_idx = ret;
>> +
>> +	err = icssm_prueth_sw_fdb_spin_lock(fdb);
>> +	if (err) {
>> +		dev_err(prueth->dev, "PRU lock timeout %d\n", ret);
>> +		return err;
>> +	}
>> +
>> +	mac_info = icssm_prueth_sw_find_free_mac(prueth, bucket_info,
>> +						 mac_tbl_idx, NULL,
>> +						 mac);
>> +	if (!mac_info) {
>> +		/* Should not happen */
>> +		dev_warn(prueth->dev, "OUT of FDB MEM\n");
> 
> This appears to leak icssm_prueth_sw_fdb_spin_lock.
> 
> Also flagged by Claude Code with https://github.com/masoncl/review-prompts/
> 

We will ensure the spin lock is properly released before returning.
This will be addressed in the next revision.


>> +		return -ENOMEM;
>> +	}
>> +
>> +	memcpy_toio(mac_info->mac, mac, ETH_ALEN);
>> +	writew(0, &mac_info->age);
>> +	writeb(emac->port_id - 1, &mac_info->port);
>> +
>> +	flags = readb(&mac_info->flags);
>> +	if (is_static)
>> +		flags |= FLAG_IS_STATIC;
>> +	else
>> +		flags &= ~FLAG_IS_STATIC;
>> +
>> +	/* bit 1 - active */
>> +	flags |= FLAG_ACTIVE;
>> +	writeb(flags, &mac_info->flags);
>> +
>> +	val = readw(&bucket_info->bucket_entries);
>> +	val++;
>> +	writew(val, &bucket_info->bucket_entries);
>> +
>> +	fdb->total_entries++;
>> +
>> +	icssm_prueth_sw_fdb_spin_unlock(fdb);
>> +
>> +	dev_dbg(prueth->dev, "added fdb: %pM port=%d total_entries=%u\n",
>> +		mac, emac->port_id, fdb->total_entries);
> 
> Less important, but I think the value of total_entries could
> be out of date as it's accessed outside of the lock.
> Perhaps it would be good to stash the value into a local variable while
> the lock is still held?
> 
> Likewise in icssm_prueth_sw_delete_fdb_entry().
> 

Sure, we will review and address this in the next version.


Thanks and Regards,
Parvathi.