Re: [PATCH] net: prestera: return error on hash table init failure

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 1 · High: 0 · Medium: 0 · Low: 0

- [Critical] Double destroy / double free of rhashtable bucket tables on
  the prestera router teardown path.

--

net: prestera: return error on hash table init failure

prestera_router_hw_init() ran its rhashtable cleanup labels but returned 0,
so a failing rhashtable_init() was reported to prestera_router_init() as
success.  The patch changes the final return in the error path from 0 to err.

Link: https://lore.kernel.org/kernel-janitors/aqT9h61WBcslE0gJ@stanley.mountain

> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
> index ccf6cf98920f2..4785fcb62735d 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
> @@ -93,7 +93,7 @@ int prestera_router_hw_init(struct prestera_switch *sw)
>  err_nexthop_grp_ht_init:
>  	rhashtable_destroy(&sw->router->nh_neigh_ht);
>  err_nh_neigh_ht_init:
> -	return 0;
> +	return err;
>  }
>  
>  void prestera_router_hw_fini(struct prestera_switch *sw)

[Severity: Critical]
The commit message describes the bug being fixed as "dereferencing of freed
memory 'sw->router->nexthop_group_ht.tbl'".  Is a very similar dereference
still left in prestera_router_hw_fini() itself, on every teardown rather
than only on the init error path?

drivers/net/ethernet/marvell/prestera/prestera_router_hw.c:
void prestera_router_hw_fini(struct prestera_switch *sw)
{
	rhashtable_free_and_destroy(&sw->router->fib_ht,
				    prestera_fib_node_destroy_ht_cb, sw);
	WARN_ON(!list_empty(&sw->router->vr_list));
	WARN_ON(!list_empty(&sw->router->rif_entry_list));
	rhashtable_destroy(&sw->router->fib_ht);
	rhashtable_destroy(&sw->router->nexthop_group_ht);
	rhashtable_destroy(&sw->router->nh_neigh_ht);
}

fib_ht is destroyed twice here, and rhashtable_destroy() is only a wrapper
around the same function that already ran:

lib/rhashtable.c:
void rhashtable_destroy(struct rhashtable *ht)
{
	return rhashtable_free_and_destroy(ht, NULL, NULL);
}

rhashtable_free_and_destroy() does not clear ht->tbl after freeing the
bucket table:

lib/rhashtable.c:rhashtable_free_and_destroy() {
	tbl = rcu_dereference_raw(ht->tbl);
restart:
	...
	next_tbl = rcu_dereference_raw(tbl->future_tbl);
	bucket_table_free(tbl);
	if (next_tbl) {
		tbl = next_tbl;
		goto restart;
	}
}

and the free is synchronous, with another dereference of the table inside
it:

lib/rhashtable.c:
static void bucket_table_free(const struct bucket_table *tbl)
{
	if (tbl->nest)
		nested_bucket_table_free(tbl);

	kvfree(tbl);
}

So does the second rhashtable_destroy(&sw->router->fib_ht) read tbl->nest
and tbl->future_tbl out of an already kvfree()d allocation and then kvfree()
it a second time?  If the object has been recycled by then, could the
garbage value read from tbl->future_tbl send the restart loop into freeing
an unrelated pointer?

The destroys of nexthop_group_ht and nh_neigh_ht look fine, since those two
tables were only rhashtable_init()'ed and never passed to
rhashtable_free_and_destroy().

The same duplication appears one level up for kern_fib_cache_ht:

drivers/net/ethernet/marvell/prestera/prestera_router.c:prestera_k_arb_abort() {
	rhashtable_free_and_destroy(&sw->router->kern_fib_cache_ht,
				    __prestera_k_arb_abort_fib_ht_cb,
				    sw);
	...
}

drivers/net/ethernet/marvell/prestera/prestera_router.c:prestera_router_fini() {
	...
	prestera_k_arb_abort(sw);

	kfree(sw->router->nhgrp_hw_state_cache);
	rhashtable_destroy(&sw->router->kern_fib_cache_ht);
	prestera_router_hw_fini(sw);
	...
}

Both of these run unconditionally on device unbind and module removal, and
prestera_router_hw_fini() is also reached from the init error label:

err_kern_fib_cache_ht_init:
	prestera_router_hw_fini(sw);

which is the very path this patch makes reachable with a non-zero return.
Should the redundant rhashtable_destroy() calls for fib_ht and
kern_fib_cache_ht be dropped as part of this fix?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914112026.2822222-1-enachman%40marvell.com