Re: [PATCH net v7 3/4] iavf: send MAC change request synchronously

[Jose Ignacio Tornos Martinez <jtornosm@redhat.com>]

>>       if (!ether_addr_equal(netdev->dev_addr, addr->sa_data))
>>               return -EACCES;

> [Severity: High]
> Will this cause a persistent state mismatch if a timeout is followed by a
> retry?
>
> If iavf_set_mac_sync() times out, it returns -EAGAIN and bypasses this
> rollback, leaving adapter->hw.mac.addr updated but netdev->dev_addr at the
> old MAC.
>
> If the user then retries before the PF asynchronously completes the original
> request, iavf_add_ether_addrs() returns -EBUSY. Since -EBUSY != -EAGAIN,
> the rollback runs, forcibly reverting adapter->hw.mac.addr to the old MAC.
> 
> When the PF's delayed success response finally arrives,
> iavf_virtchnl_completion() sees the old MAC in both hw.mac.addr and
> netdev->dev_addr, bypassing the netdev MAC update. The PF hardware is now
> programmed with the new MAC, but the kernel retains the old MAC, dropping
> traffic.

This scenario requires an extremely narrow timing window: a 2.5s PF timeout
(rare in normal operation, PF typically responds much faster), followed by an
immediate retry before adminq_task clears current_op (microsecond window).

Even if this race occurs, it's recoverable - the retry fails with -EBUSY
and the user retries again successfully. This is a transient failure, not
permanent state corruption.

>>       if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
>>               /* bail because we already have a command pending */
>>               dev_err(&adapter->pdev->dev, "Cannot add filters, command %d pending\n",
>>                      adapter->current_op);
>> -             return;
>> +             return -EBUSY;
>>       }
>
> [Severity: High]
> Could routine background tasks cause MAC changes to frequently fail?
>
> The watchdog task routinely sets adapter->current_op for background commands
> like VIRTCHNL_OP_GET_STATS. If iavf_set_mac() is called during this window,
> iavf_set_mac_sync() will hit this check and immediately fail the MAC change
> with -EBUSY.
>
> Because there is no retry logic in iavf_set_mac(), this error propagates
> directly to userspace, making MAC address configuration randomly fail based
> on watchdog timing.

Background virtchnl operations (like GET_STATS) complete quickly, typically
within milliseconds. The collision window is very small.

Fail-fast with -EBUSY is semantically correct and allows user-space retry.
If a collision occurs, it's spurious and recoverable immediately on retry.
This is acceptable compared to the complexity and deadlock risks of queuing
or waiting for background operations to complete while holding netdev_lock.

>> -static void iavf_mac_add_reject(struct iavf_adapter *adapter)
>> + */
>> +void iavf_mac_add_reject(struct iavf_adapter *adapter)
>>  {
>>       struct net_device *netdev = adapter->netdev;
>>       struct iavf_mac_filter *f, *ftmp;
>
> [Severity: Medium]
> Does exporting and reusing iavf_mac_add_reject() for local rollback cause
> unintended background MAC changes?
>
> When the user requests a MAC address that is already in the filter list
> (e.g., a secondary MAC), iavf_add_filter() sets f->is_new_mac = false and
> f->add = true, and flags IAVF_FLAG_AQ_ADD_MAC_FILTER.
>
> If the synchronous operation fails locally (like the -EBUSY return above),
> iavf_set_mac() calls this function. However, iavf_mac_add_reject() only
> clears the pending state and removes the filter if f->is_new_mac is true:
>
>    if (f->is_new_mac) {
>        list_del(&f->list);
>        kfree(f);
>    }
>
> Reused filters are ignored, leaving f->add = true and the
> IAVF_FLAG_AQ_ADD_MAC_FILTER flag active. The background watchdog task will
> eventually process this flag and blindly send the MAC configuration to the
> PF, even though the VF already aborted the operation locally.

This scenario only occurs when setting the primary MAC to an address that
already exists as a secondary MAC in the filter list - an extremely rare
configuration.

Even if this occurs and the watchdog later sends the MAC to the PF, it is
harmless: the MAC is already configured on the PF (as a secondary), so the
redundant ADD_ETH_ADDR message has no adverse effect.

The common case - changing primary MAC to a new address - uses is_new_mac =
true and is handled correctly by the rollback logic.



These concerns represent theoretical edge cases that are extremely unlikely
in practice. The synchronous approach fixes a reproducible deadlock affecting
all users in production, allowing the user to retry and complete the
operation. The trade-off is justified.