Re: [PATCH net-next v5 01/14] net: add address list snapshot and reconciliation infrastructure

[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.
---
    net: add address list snapshot and reconciliation infrastructure

This commit introduces address list snapshot and reconciliation routines
to support the async rx_mode path. It provides functions to snapshot the
lists, and to reconcile the hardware sync state back to the original lists
after driver processing.

> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
[ ... ]
> @@ -501,6 +503,114 @@ void __hw_addr_init(struct netdev_hw_addr_list *list)
[ ... ]
> +void __hw_addr_list_reconcile(struct netdev_hw_addr_list *real_list,
> +			      struct netdev_hw_addr_list *work,
> +			      struct netdev_hw_addr_list *ref, int addr_len)
> +{
[ ... ]
> +		real_ha = __hw_addr_lookup(real_list, ref_ha->addr, addr_len,
> +					   ref_ha->type);
> +		if (!real_ha) {
> +			/* The real entry was concurrently removed. If the
> +			 * driver synced this addr to hardware (delta > 0),
> +			 * re-insert it as a stale entry so the next work
> +			 * run unsyncs it from hardware.
> +			 */
> +			if (delta > 0) {
> +				real_ha = __hw_addr_create(ref_ha->addr,
> +							   addr_len,
> +							   ref_ha->type, false,
> +							   false);

Does this code silently leak a hardware MAC filter if the GFP_ATOMIC allocation
fails? If the software loses track of this address, it appears future sync runs
will never unsync it from the hardware.

Could this failure be avoided by unlinking ref_ha from the ref list (which is
about to be unconditionally freed via __hw_addr_flush) and moving it directly
into real_list?

> +				if (real_ha) {
> +					real_ha->sync_cnt = 1;
> +					real_ha->refcount = 1;

Does this code break compatibility with reference-counted sync APIs?

While __hw_addr_sync_dev() increments by 1, drivers processing snapshots with
__hw_addr_ref_sync_dev() might increment sync_cnt by the total number of
references. If such an entry is concurrently removed, recreation with a
hardcoded value of 1 could instruct the next async run to unsync only 1
reference, permanently leaking the remaining hardware references.

Should these be dynamically initialized to real_ha->sync_cnt = delta and
real_ha->refcount = delta?

> +					list_add_tail_rcu(&real_ha->list,
> +							  &real_list->list);
> +					__hw_addr_insert(real_list, real_ha,
> +							 addr_len);
> +					real_list->count++;
> +				}
> +			}
> +			continue;
> +		}
[ ... ]
> diff --git a/net/core/dev_addr_lists_test.c b/net/core/dev_addr_lists_test.c
> --- a/net/core/dev_addr_lists_test.c
> +++ b/net/core/dev_addr_lists_test.c
[ ... ]
> @@ -225,6 +245,339 @@ static void dev_addr_test_add_excl(struct kunit *test)
[ ... ]
> +static void dev_addr_test_snapshot_sync(struct kunit *test)
> +{
[ ... ]
> +	netif_addr_lock_bh(netdev);
> +	__hw_addr_init(&snap);
> +	__hw_addr_init(&ref);
> +	KUNIT_ASSERT_EQ(test, 0,
> +			__hw_addr_list_snapshot(&snap, &netdev->uc, ETH_ALEN));
> +	KUNIT_ASSERT_EQ(test, 0,
> +			__hw_addr_list_snapshot(&ref, &netdev->uc, ETH_ALEN));
> +	netif_addr_unlock_bh(netdev);

Will this cause a system deadlock if the assertion fails?

If KUNIT_ASSERT_EQ triggers (e.g. from an out-of-memory condition in
__hw_addr_list_snapshot), it immediately aborts the test case thread and skips
the rest of the function, leaving the netif_addr_lock_bh spinlock permanently
held and bottom-halves disabled.

Could this be replaced with KUNIT_EXPECT_EQ and explicit goto error handling
to ensure netif_addr_unlock_bh is always reached?

[ ... ]
> +static void dev_addr_test_snapshot_benchmark(struct kunit *test)
> +{
[ ... ]
> +	for (iter = 0; iter < 1000; iter++) {
> +		netif_addr_lock_bh(netdev);
> +		__hw_addr_init(&snap);
> +
> +		start = ktime_get();
> +		KUNIT_ASSERT_EQ(test, 0,
> +				__hw_addr_list_snapshot(&snap, &netdev->uc,
> +							ETH_ALEN));
> +		duration += ktime_to_ns(ktime_sub(ktime_get(), start));
> +
> +		netif_addr_unlock_bh(netdev);

Will this also cause a deadlock if the assertion fails while the lock is held?
-- 
pw-bot: cr