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

[sdf.kernel@gmail.com]

> 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?

Will unlink the ref_ha from ref list and relink, that should work.

> 
> > +				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?

Idk, I did convince myself that the delta can be only 1 here and felt
like it's easier to follow, but will switch to sync_cnt=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?

Will do.

> 
> [ ... ]
> > +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?

Same here, will do.