From: Emil Tantilov <emil.s.tantilov@intel.com>
Make sure to clear the IDPF_VPORT_UP bit on entry. The idpf_vport_stop()
function is void and once called, the vport teardown is guaranteed to
happen. Previously the bit was cleared at the end of the function, which
opened it up to possible races with all instances in the driver where
operations were conditional on this bit being set. For example, on rmmod
callbacks in the middle of idpf_vport_stop() end up attempting to remove
MAC address filter already removed by the function:
idpf 0000:83:00.0: Received invalid MAC filter payload (op 536) (len 0)
Fixes: 1c325aac10a8 ("idpf: configure resources for TX queues")
Reviewed-by: Joshua Hay <joshua.a.hay@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Chittim Madhu <madhu.chittim@intel.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Tested-by: Samuel Salin <Samuel.salin@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
---
drivers/net/ethernet/intel/idpf/idpf_lib.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 89d30c395533..01ab42fa23f9 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -888,7 +888,7 @@ static void idpf_vport_stop(struct idpf_vport *vport)
{
struct idpf_netdev_priv *np = netdev_priv(vport->netdev);
- if (!test_bit(IDPF_VPORT_UP, np->state))
+ if (!test_and_clear_bit(IDPF_VPORT_UP, np->state))
return;
netif_carrier_off(vport->netdev);
@@ -911,7 +911,6 @@ static void idpf_vport_stop(struct idpf_vport *vport)
idpf_vport_intr_deinit(vport);
idpf_vport_queues_rel(vport);
idpf_vport_intr_rel(vport);
- clear_bit(IDPF_VPORT_UP, np->state);
}
/**
--
2.51.0.rc1.197.g6d975e95c9d7On Wed, 01 Oct 2025 17:14:13 -0700 Jacob Keller wrote: > From: Emil Tantilov <emil.s.tantilov@intel.com> > > Make sure to clear the IDPF_VPORT_UP bit on entry. The idpf_vport_stop() > function is void and once called, the vport teardown is guaranteed to > happen. Previously the bit was cleared at the end of the function, which > opened it up to possible races with all instances in the driver where > operations were conditional on this bit being set. For example, on rmmod > callbacks in the middle of idpf_vport_stop() end up attempting to remove > MAC address filter already removed by the function: > idpf 0000:83:00.0: Received invalid MAC filter payload (op 536) (len 0) Argh, please stop using the flag based state machines. They CANNOT replace locking. If there was proper locking in place it wouldn't have mattered when we clear the flag. I'm guessing false negatives are okay in how you use the UP flag? The commit message doesn't really explain why.
On 10/3/2025 10:43 AM, Jakub Kicinski wrote: > On Wed, 01 Oct 2025 17:14:13 -0700 Jacob Keller wrote: >> From: Emil Tantilov <emil.s.tantilov@intel.com> >> >> Make sure to clear the IDPF_VPORT_UP bit on entry. The idpf_vport_stop() >> function is void and once called, the vport teardown is guaranteed to >> happen. Previously the bit was cleared at the end of the function, which >> opened it up to possible races with all instances in the driver where >> operations were conditional on this bit being set. For example, on rmmod >> callbacks in the middle of idpf_vport_stop() end up attempting to remove >> MAC address filter already removed by the function: >> idpf 0000:83:00.0: Received invalid MAC filter payload (op 536) (len 0) > > Argh, please stop using the flag based state machines. They CANNOT > replace locking. If there was proper locking in place it wouldn't > have mattered when we clear the flag. This patch is resolving a bug in the current logic of how the flag is used (not being atomic and not being cleared properly). I don't think there is an existing lock in place to address this issue, though we are looking to refactor the code over time to remove and/or limit how these flags are used. > > I'm guessing false negatives are okay in how you use the UP flag? > The commit message doesn't really explain why. I am not sure I understand the question completely, if you mean that the flag is cleared before the vport is actually brought down, I did touch on it at the beginning of the description, once we enter idpf_vport_stop() we know for sure that the vport will be brought down and that is how the present checks in the driver for the UP flag are being used. Specifically in the scenario that exposed this issue, the driver will send a message in the middle of idpf_vport_down() after checking the flag. Thanks, Emil
On Mon, 6 Oct 2025 07:49:32 -0700 Tantilov, Emil S wrote:
> > Argh, please stop using the flag based state machines. They CANNOT
> > replace locking. If there was proper locking in place it wouldn't
> > have mattered when we clear the flag.
>
> This patch is resolving a bug in the current logic of how the flag is
> used (not being atomic and not being cleared properly). I don't think
> there is an existing lock in place to address this issue, though we are
> looking to refactor the code over time to remove and/or limit how these
> flags are used.
Can you share more details about the race? If there is no lock in place
there's always the risk that:
CPU 0 CPU 1
idpf_vport_stop() whatever()
if (test_bit(UP))
# sees true
# !< long IRQ arrives
test_and_clear(UP)
...
all the rest
...
# > long IRQ ends
proceed but UP isn't really set any more
On 10/6/2025 10:26 AM, Jakub Kicinski wrote:
> On Mon, 6 Oct 2025 07:49:32 -0700 Tantilov, Emil S wrote:
>>> Argh, please stop using the flag based state machines. They CANNOT
>>> replace locking. If there was proper locking in place it wouldn't
>>> have mattered when we clear the flag.
>>
>> This patch is resolving a bug in the current logic of how the flag is
>> used (not being atomic and not being cleared properly). I don't think
>> there is an existing lock in place to address this issue, though we are
>> looking to refactor the code over time to remove and/or limit how these
>> flags are used.
>
> Can you share more details about the race? If there is no lock in place
> there's always the risk that:
>
> CPU 0 CPU 1
> idpf_vport_stop() whatever()
> if (test_bit(UP))
> # sees true
> # !< long IRQ arrives
> test_and_clear(UP)
> ...
> all the rest
> ...
> # > long IRQ ends
> proceed but UP isn't really set any more
>
The specific case I was targeting with this patch is for when both
idpf_vport_stop() and idpf_addr_unsync(), called via set_rx_mode attempt
to delete the MAC filters. At least in my testing I have not seen a case
where the set_rx_mode callback will happen before idpf_vport_stop(). I
am assuming due to userspace reacting to the removal of the netdevs.
rmmod-6089 [021] ..... 3521.291596: idpf_remove
<-pci_device_remove
rmmod-6089 [021] ..... 3521.292686: idpf_vport_stop
<-idpf_vport_dealloc
systemd-resolve-1633 [022] b..1. 3521.295320: idpf_set_rx_mode
<-dev_mc_del
systemd-resolve-1633 [022] b..1. 3521.295338: idpf_addr_unsync
<-__hw_addr_sync_dev
systemd-resolve-1633 [022] b..1. 3521.295339: idpf_del_mac_filter
<-idpf_addr_unsync
systemd-resolve-1633 [022] b..1. 3521.295450: idpf_set_rx_mode
<-dev_mc_del
systemd-resolve-1633 [022] b..1. 3521.295451: idpf_addr_unsync
<-__hw_addr_sync_dev
systemd-resolve-1633 [022] b..1. 3521.295451: idpf_del_mac_filter
<-idpf_addr_unsync
rmmod-6089 [002] ..... 3521.934980: idpf_vport_stop
<-idpf_vport_dealloc
systemd-resolve-1633 [022] b..1. 3522.297299: idpf_set_rx_mode
<-dev_mc_del
systemd-resolve-1633 [022] b..1. 3522.297316: idpf_addr_unsync
<-__hw_addr_sync_dev
systemd-resolve-1633 [022] b..1. 3522.297317: idpf_del_mac_filter
<-idpf_addr_unsync
kworker/u261:2-3157 [037] ...1. 3522.297931:
idpf_mac_filter_async_handler: Received invalid MAC filter payload (op
536) (len 0)
rmmod-6089 [020] ..... 3522.573251: idpf_vport_stop
<-idpf_vport_dealloc
rmmod-6089 [002] ..... 3523.229936: idpf_vport_stop
<-idpf_vport_dealloc
systemd-resolve-1633 [022] b..1. 3523.311435: idpf_set_rx_mode
<-dev_mc_del
systemd-resolve-1633 [022] b..1. 3523.311452: idpf_addr_unsync
<-__hw_addr_sync_dev
systemd-resolve-1633 [022] b..1. 3523.311453: idpf_del_mac_filter
<-idpf_addr_unsync
On 10/6/2025 5:07 PM, Tantilov, Emil S wrote: > > > On 10/6/2025 10:26 AM, Jakub Kicinski wrote: >> On Mon, 6 Oct 2025 07:49:32 -0700 Tantilov, Emil S wrote: >>>> Argh, please stop using the flag based state machines. They CANNOT >>>> replace locking. If there was proper locking in place it wouldn't >>>> have mattered when we clear the flag. >>> >>> This patch is resolving a bug in the current logic of how the flag is >>> used (not being atomic and not being cleared properly). I don't think >>> there is an existing lock in place to address this issue, though we are >>> looking to refactor the code over time to remove and/or limit how these >>> flags are used. >> >> Can you share more details about the race? If there is no lock in place >> there's always the risk that: >> >> CPU 0 CPU 1 >> idpf_vport_stop() whatever() >> if (test_bit(UP)) >> # sees true >> # !< long IRQ arrives >> test_and_clear(UP) >> ... >> all the rest >> ... >> # > long IRQ ends >> proceed but UP isn't really set any more >> > > The specific case I was targeting with this patch is for when both > idpf_vport_stop() and idpf_addr_unsync(), called via set_rx_mode attempt > to delete the MAC filters. At least in my testing I have not seen a case > where the set_rx_mode callback will happen before idpf_vport_stop(). I > am assuming due to userspace reacting to the removal of the netdevs. > > rmmod-6089 [021] ..... 3521.291596: idpf_remove > <-pci_device_remove > rmmod-6089 [021] ..... 3521.292686: idpf_vport_stop > <-idpf_vport_dealloc > systemd-resolve-1633 [022] b..1. 3521.295320: idpf_set_rx_mode > <-dev_mc_del > systemd-resolve-1633 [022] b..1. 3521.295338: idpf_addr_unsync > <-__hw_addr_sync_dev > systemd-resolve-1633 [022] b..1. 3521.295339: idpf_del_mac_filter > <-idpf_addr_unsync > systemd-resolve-1633 [022] b..1. 3521.295450: idpf_set_rx_mode > <-dev_mc_del > systemd-resolve-1633 [022] b..1. 3521.295451: idpf_addr_unsync > <-__hw_addr_sync_dev > systemd-resolve-1633 [022] b..1. 3521.295451: idpf_del_mac_filter > <-idpf_addr_unsync > rmmod-6089 [002] ..... 3521.934980: idpf_vport_stop > <-idpf_vport_dealloc > systemd-resolve-1633 [022] b..1. 3522.297299: idpf_set_rx_mode > <-dev_mc_del > systemd-resolve-1633 [022] b..1. 3522.297316: idpf_addr_unsync > <-__hw_addr_sync_dev > systemd-resolve-1633 [022] b..1. 3522.297317: idpf_del_mac_filter > <-idpf_addr_unsync > kworker/u261:2-3157 [037] ...1. 3522.297931: > idpf_mac_filter_async_handler: Received invalid MAC filter payload (op > 536) (len 0) > rmmod-6089 [020] ..... 3522.573251: idpf_vport_stop > <-idpf_vport_dealloc > rmmod-6089 [002] ..... 3523.229936: idpf_vport_stop > <-idpf_vport_dealloc > systemd-resolve-1633 [022] b..1. 3523.311435: idpf_set_rx_mode > <-dev_mc_del > systemd-resolve-1633 [022] b..1. 3523.311452: idpf_addr_unsync > <-__hw_addr_sync_dev > systemd-resolve-1633 [022] b..1. 3523.311453: idpf_del_mac_filter > <-idpf_addr_unsync > > Jakub, from this thread it still seems like you won't accept this patch as-is? I'm working on a v3 of this series resolving the other issues Simon pointed out, and want to know if I should continue excluding Emil's patches for now. Thanks, Jake
On Wed, 8 Oct 2025 14:41:40 -0700 Jacob Keller wrote: > Jakub, from this thread it still seems like you won't accept this patch > as-is? I haven't analyzed Emil's response. I hope my concern is clear enough (at least to you). If the code is provably safe I think the patch itself can go in, we just need a much better commit msg.
On 10/13/2025 11:13 AM, Jakub Kicinski wrote: > On Wed, 8 Oct 2025 14:41:40 -0700 Jacob Keller wrote: >> Jakub, from this thread it still seems like you won't accept this patch >> as-is? > > I haven't analyzed Emil's response. I hope my concern is clear enough > (at least to you). If the code is provably safe I think the patch itself > can go in, we just need a much better commit msg. Actually we can drop this patch as the issue it is fixing is no longer reproducible, following the introduction of the RTNL parameter to idpf_vport_stop() by Olek: https://lore.kernel.org/netdev/20250908195748.1707057-5-anthony.l.nguyen@intel.com/ I would still like to keep the first patch in this series: https://lore.kernel.org/netdev/20250822035248.22969-2-emil.s.tantilov@intel.com/ I will rebase it and resend for -next unless there are objections. Thanks, Emil
© 2016 - 2026 Red Hat, Inc.