* [PATCH net 2/9] idpf: improve locking around idpf_vc_xn_push_free()
From: Tony Nguyen @ 2026-04-06 21:30 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Emil Tantilov, anthony.l.nguyen, bigeasy, clrkwllms, rostedt,
linux-rt-devel, stable, Ray Zhang, Aleksandr Loktionov,
Samuel Salin
In-Reply-To: <20260406213038.444732-1-anthony.l.nguyen@intel.com>
From: Emil Tantilov <emil.s.tantilov@intel.com>
Protect the set_bit() operation for the free_xn bitmask in
idpf_vc_xn_push_free(), to make the locking consistent with rest of the
code and avoid potential races in that logic.
Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
Cc: stable@vger.kernel.org
Reported-by: Ray Zhang <sgzhang@google.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Tested-by: Samuel Salin <Samuel.salin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index 582e0c8e9dc0..fbd5a15b015c 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -402,7 +402,9 @@ static void idpf_vc_xn_push_free(struct idpf_vc_xn_manager *vcxn_mngr,
struct idpf_vc_xn *xn)
{
idpf_vc_xn_release_bufs(xn);
+ spin_lock_bh(&vcxn_mngr->xn_bm_lock);
set_bit(xn->idx, vcxn_mngr->free_xn_bm);
+ spin_unlock_bh(&vcxn_mngr->xn_bm_lock);
}
/**
--
2.47.1
^ permalink raw reply related
* [PATCH net 3/9] idpf: set the payload size before calling the async handler
From: Tony Nguyen @ 2026-04-06 21:30 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Emil Tantilov, anthony.l.nguyen, bigeasy, clrkwllms, rostedt,
linux-rt-devel, stable, Aleksandr Loktionov, Li Li, Samuel Salin
In-Reply-To: <20260406213038.444732-1-anthony.l.nguyen@intel.com>
From: Emil Tantilov <emil.s.tantilov@intel.com>
Set the payload size before forwarding the reply to the async handler.
Without this, xn->reply_sz will be 0 and idpf_mac_filter_async_handler()
will never get past the size check.
Fixes: 34c21fa894a1 ("idpf: implement virtchnl transaction manager")
Cc: stable@vger.kernel.org
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Li Li <boolli@google.com>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Tested-by: Samuel Salin <Samuel.salin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index fbd5a15b015c..be66f9b2e101 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -615,6 +615,10 @@ idpf_vc_xn_forward_reply(struct idpf_adapter *adapter,
err = -ENXIO;
goto out_unlock;
case IDPF_VC_XN_ASYNC:
+ /* Set reply_sz from the actual payload so that async_handler
+ * can evaluate the response.
+ */
+ xn->reply_sz = ctlq_msg->data_len;
err = idpf_vc_xn_forward_async(adapter, xn, ctlq_msg);
idpf_vc_xn_unlock(xn);
return err;
--
2.47.1
^ permalink raw reply related
* [PATCH net 4/9] ice: ptp: don't WARN when controlling PF is unavailable
From: Tony Nguyen @ 2026-04-06 21:30 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Kohei Enju, anthony.l.nguyen, sergey.temerkhanov,
przemyslaw.kitszel, kohei.enju, horms, richardcochran,
Aleksandr Loktionov
In-Reply-To: <20260406213038.444732-1-anthony.l.nguyen@intel.com>
From: Kohei Enju <kohei@enjuk.jp>
In VFIO passthrough setups, it is possible to pass through only a PF
which doesn't own the source timer. In that case the PTP controlling PF
(adapter->ctrl_pf) is never initialized in the VM, so ice_get_ctrl_ptp()
returns NULL and triggers WARN_ON() in ice_ptp_setup_pf().
Since this is an expected behavior in that configuration, replace
WARN_ON() with an informational message and return -EOPNOTSUPP.
Fixes: e800654e85b5 ("ice: Use ice_adapter for PTP shared data instead of auxdev")
Signed-off-by: Kohei Enju <kohei@enjuk.jp>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index 094e96219f45..a3c5f0a5e09b 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -3080,7 +3080,13 @@ static int ice_ptp_setup_pf(struct ice_pf *pf)
struct ice_ptp *ctrl_ptp = ice_get_ctrl_ptp(pf);
struct ice_ptp *ptp = &pf->ptp;
- if (WARN_ON(!ctrl_ptp) || pf->hw.mac_type == ICE_MAC_UNKNOWN)
+ if (!ctrl_ptp) {
+ dev_info(ice_pf_to_dev(pf),
+ "PTP unavailable: no controlling PF\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (pf->hw.mac_type == ICE_MAC_UNKNOWN)
return -ENODEV;
INIT_LIST_HEAD(&ptp->port.list_node);
--
2.47.1
^ permalink raw reply related
* [PATCH net 5/9] ice: fix PTP timestamping broken by SyncE code on E825C
From: Tony Nguyen @ 2026-04-06 21:30 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Petr Oros, anthony.l.nguyen, arkadiusz.kubalewski,
przemyslaw.kitszel, ivecera, richardcochran, Grzegorz Nitka,
Aleksandr Loktionov, Sunitha Mekala
In-Reply-To: <20260406213038.444732-1-anthony.l.nguyen@intel.com>
From: Petr Oros <poros@redhat.com>
The E825C SyncE support added in commit ad1df4f2d591 ("ice: dpll:
Support E825-C SyncE and dynamic pin discovery") introduced a SyncE
reconfiguration block in ice_ptp_link_change() that prevents
ice_ptp_port_phy_restart() from being called in several error paths.
Without the PHY restart, PTP timestamps stop working after any link
change event.
There are three ways the PHY restart gets blocked:
1. When DPLL initialization fails (e.g. missing ACPI firmware node
properties), ICE_FLAG_DPLL is not set and the function returns early
before reaching the PHY restart.
2. When ice_tspll_bypass_mux_active_e825c() fails to read the CGU
register, WARN_ON_ONCE fires and the function returns early.
3. When ice_tspll_cfg_synce_ethdiv_e825c() fails to configure the
clock divider for an active pin, same early return.
SyncE and PTP are independent features. SyncE reconfiguration failures
must not prevent the PTP PHY restart that is essential for timestamp
recovery after link changes.
Fix by making the entire SyncE block conditional on ICE_FLAG_DPLL
without an early return, and replacing the WARN_ON_ONCE + return error
handling inside the loop with dev_err_once + break. The function always
proceeds to ice_ptp_port_phy_restart() regardless of SyncE errors.
Fixes: ad1df4f2d591 ("ice: dpll: Support E825-C SyncE and dynamic pin discovery")
Signed-off-by: Petr Oros <poros@redhat.com>
Reviewed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice_ptp.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c
index a3c5f0a5e09b..6cb0cf7a9891 100644
--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
@@ -1296,12 +1296,10 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
if (pf->hw.reset_ongoing)
return;
- if (hw->mac_type == ICE_MAC_GENERIC_3K_E825) {
+ if (hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
+ test_bit(ICE_FLAG_DPLL, pf->flags)) {
int pin, err;
- if (!test_bit(ICE_FLAG_DPLL, pf->flags))
- return;
-
mutex_lock(&pf->dplls.lock);
for (pin = 0; pin < ICE_SYNCE_CLK_NUM; pin++) {
enum ice_synce_clk clk_pin;
@@ -1314,15 +1312,19 @@ void ice_ptp_link_change(struct ice_pf *pf, bool linkup)
port_num,
&active,
clk_pin);
- if (WARN_ON_ONCE(err)) {
- mutex_unlock(&pf->dplls.lock);
- return;
+ if (err) {
+ dev_err_once(ice_pf_to_dev(pf),
+ "Failed to read SyncE bypass mux for pin %d, err %d\n",
+ pin, err);
+ break;
}
err = ice_tspll_cfg_synce_ethdiv_e825c(hw, clk_pin);
- if (active && WARN_ON_ONCE(err)) {
- mutex_unlock(&pf->dplls.lock);
- return;
+ if (active && err) {
+ dev_err_once(ice_pf_to_dev(pf),
+ "Failed to configure SyncE ETH divider for pin %d, err %d\n",
+ pin, err);
+ break;
}
}
mutex_unlock(&pf->dplls.lock);
--
2.47.1
^ permalink raw reply related
* [PATCH net 6/9] ixgbe: stop re-reading flash on every get_drvinfo for e610
From: Tony Nguyen @ 2026-04-06 21:30 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Aleksandr Loktionov, anthony.l.nguyen, Jedrzej Jagielski,
Simon Horman, Rinitha S
In-Reply-To: <20260406213038.444732-1-anthony.l.nguyen@intel.com>
From: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
ixgbe_get_drvinfo() calls ixgbe_refresh_fw_version() on every ethtool
query for e610 adapters. That ends up in ixgbe_discover_flash_size(),
which bisects the full 16 MB NVM space issuing one ACI command per
step (~20 ms each, ~24 steps total = ~500 ms).
Profiling on an idle E610-XAT2 system with telegraf scraping ethtool
stats every 10 seconds:
kretprobe:ixgbe_get_drvinfo took 527603 us
kretprobe:ixgbe_get_drvinfo took 523978 us
kretprobe:ixgbe_get_drvinfo took 552975 us
kretprobe:ice_get_drvinfo took 3 us
kretprobe:igb_get_drvinfo took 2 us
kretprobe:i40e_get_drvinfo took 5 us
The half-second stall happens under the RTNL lock, causing visible
latency on ip-link and friends.
The FW version can only change after an EMPR reset. All flash data is
already populated at probe time and the cached adapter->eeprom_id is
what get_drvinfo should be returning. The only place that needs to
trigger a re-read is ixgbe_devlink_reload_empr_finish(), right after
the EMPR completes and new firmware is running. Additionally, refresh
the FW version in ixgbe_reinit_locked() so that any PF that undergoes a
reinit after an EMPR (e.g. triggered by another PF's devlink reload)
also picks up the new version in adapter->eeprom_id.
ixgbe_devlink_info_get() keeps its refresh call for explicit
"devlink dev info" queries, which is fine given those are user-initiated.
Fixes: c9e563cae19e ("ixgbe: add support for devlink reload")
Co-developed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ixgbe/devlink/devlink.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 13 +++++++------
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 ++++++++++
4 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
index b0404f37271a..cf8908b82f8a 100644
--- a/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
+++ b/drivers/net/ethernet/intel/ixgbe/devlink/devlink.c
@@ -474,7 +474,7 @@ static int ixgbe_devlink_reload_empr_finish(struct devlink *devlink,
adapter->flags2 &= ~(IXGBE_FLAG2_API_MISMATCH |
IXGBE_FLAG2_FW_ROLLBACK);
- return 0;
+ return ixgbe_refresh_fw_version(adapter);
}
static const struct devlink_ops ixgbe_devlink_ops = {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index dce4936708eb..047f04045585 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -973,7 +973,7 @@ int ixgbe_init_interrupt_scheme(struct ixgbe_adapter *adapter);
bool ixgbe_wol_supported(struct ixgbe_adapter *adapter, u16 device_id,
u16 subdevice_id);
void ixgbe_set_fw_version_e610(struct ixgbe_adapter *adapter);
-void ixgbe_refresh_fw_version(struct ixgbe_adapter *adapter);
+int ixgbe_refresh_fw_version(struct ixgbe_adapter *adapter);
#ifdef CONFIG_PCI_IOV
void ixgbe_full_sync_mac_table(struct ixgbe_adapter *adapter);
#endif
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 56aabaa5caec..ba049b3a9609 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1155,12 +1155,17 @@ static int ixgbe_set_eeprom(struct net_device *netdev,
return ret_val;
}
-void ixgbe_refresh_fw_version(struct ixgbe_adapter *adapter)
+int ixgbe_refresh_fw_version(struct ixgbe_adapter *adapter)
{
struct ixgbe_hw *hw = &adapter->hw;
+ int err;
+
+ err = ixgbe_get_flash_data(hw);
+ if (err)
+ return err;
- ixgbe_get_flash_data(hw);
ixgbe_set_fw_version_e610(adapter);
+ return 0;
}
static void ixgbe_get_drvinfo(struct net_device *netdev,
@@ -1168,10 +1173,6 @@ static void ixgbe_get_drvinfo(struct net_device *netdev,
{
struct ixgbe_adapter *adapter = ixgbe_from_netdev(netdev);
- /* need to refresh info for e610 in case fw reloads in runtime */
- if (adapter->hw.mac.type == ixgbe_mac_e610)
- ixgbe_refresh_fw_version(adapter);
-
strscpy(drvinfo->driver, ixgbe_driver_name, sizeof(drvinfo->driver));
strscpy(drvinfo->fw_version, adapter->eeprom_id,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index e4101c59074d..59801187a839 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -6289,6 +6289,16 @@ void ixgbe_reinit_locked(struct ixgbe_adapter *adapter)
if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
msleep(2000);
ixgbe_up(adapter);
+
+ /* E610 has no FW event to notify all PFs of an EMPR reset, so
+ * refresh the FW version here to pick up any new FW version after
+ * a hardware reset (e.g. EMPR triggered by another PF's devlink
+ * reload). ixgbe_refresh_fw_version() updates both hw->flash and
+ * adapter->eeprom_id so ethtool -i reports the correct string.
+ */
+ if (adapter->hw.mac.type == ixgbe_mac_e610)
+ (void)ixgbe_refresh_fw_version(adapter);
+
clear_bit(__IXGBE_RESETTING, &adapter->state);
}
--
2.47.1
^ permalink raw reply related
* [PATCH net 7/9] ixgbevf: add missing negotiate_features op to Hyper-V ops table
From: Tony Nguyen @ 2026-04-06 21:30 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Michal Schmidt, anthony.l.nguyen, Xiaoqiang Xiong,
Aleksandr Loktionov
In-Reply-To: <20260406213038.444732-1-anthony.l.nguyen@intel.com>
From: Michal Schmidt <mschmidt@redhat.com>
Commit a7075f501bd3 ("ixgbevf: fix mailbox API compatibility by
negotiating supported features") added the .negotiate_features callback
to ixgbe_mac_operations and populated it in ixgbevf_mac_ops, but forgot
to add it to ixgbevf_hv_mac_ops. This leaves the function pointer NULL
on Hyper-V VMs.
During probe, ixgbevf_negotiate_api() calls ixgbevf_set_features(),
which unconditionally dereferences hw->mac.ops.negotiate_features().
On Hyper-V this results in a NULL pointer dereference:
BUG: kernel NULL pointer dereference, address: 0000000000000000
[...]
Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine [...]
Workqueue: events work_for_cpu_fn
RIP: 0010:0x0
[...]
Call Trace:
ixgbevf_negotiate_api+0x66/0x160 [ixgbevf]
ixgbevf_sw_init+0xe4/0x1f0 [ixgbevf]
ixgbevf_probe+0x20f/0x4a0 [ixgbevf]
local_pci_probe+0x50/0xa0
work_for_cpu_fn+0x1a/0x30
[...]
Add ixgbevf_hv_negotiate_features_vf() that returns -EOPNOTSUPP and
wire it into ixgbevf_hv_mac_ops. The caller already handles -EOPNOTSUPP
gracefully.
Fixes: a7075f501bd3 ("ixgbevf: fix mailbox API compatibility by negotiating supported features")
Reported-by: Xiaoqiang Xiong <xxiong@redhat.com>
Closes: https://issues.redhat.com/browse/RHEL-155455
Assisted-by: Claude:claude-4.6-opus-high Cursor
Tested-by: Xiaoqiang Xiong <xxiong@redhat.com>
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ixgbevf/vf.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/ethernet/intel/ixgbevf/vf.c b/drivers/net/ethernet/intel/ixgbevf/vf.c
index b67b580f7f1c..f6df86d124b9 100644
--- a/drivers/net/ethernet/intel/ixgbevf/vf.c
+++ b/drivers/net/ethernet/intel/ixgbevf/vf.c
@@ -709,6 +709,12 @@ static int ixgbevf_negotiate_features_vf(struct ixgbe_hw *hw, u32 *pf_features)
return err;
}
+static int ixgbevf_hv_negotiate_features_vf(struct ixgbe_hw *hw,
+ u32 *pf_features)
+{
+ return -EOPNOTSUPP;
+}
+
/**
* ixgbevf_set_vfta_vf - Set/Unset VLAN filter table address
* @hw: pointer to the HW structure
@@ -1142,6 +1148,7 @@ static const struct ixgbe_mac_operations ixgbevf_hv_mac_ops = {
.setup_link = ixgbevf_setup_mac_link_vf,
.check_link = ixgbevf_hv_check_mac_link_vf,
.negotiate_api_version = ixgbevf_hv_negotiate_api_version_vf,
+ .negotiate_features = ixgbevf_hv_negotiate_features_vf,
.set_rar = ixgbevf_hv_set_rar_vf,
.update_mc_addr_list = ixgbevf_hv_update_mc_addr_list_vf,
.update_xcast_mode = ixgbevf_hv_update_xcast_mode,
--
2.47.1
^ permalink raw reply related
* [PATCH net 9/9] e1000: check return value of e1000_read_eeprom
From: Tony Nguyen @ 2026-04-06 21:30 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Agalakov Daniil, anthony.l.nguyen, lvc-project, dish, rrv,
Aleksandr Loktionov
In-Reply-To: <20260406213038.444732-1-anthony.l.nguyen@intel.com>
From: Agalakov Daniil <ade@amicon.ru>
[Why]
e1000_set_eeprom() performs a read-modify-write operation when the write
range is not word-aligned. This requires reading the first and last words
of the range from the EEPROM to preserve the unmodified bytes.
However, the code does not check the return value of e1000_read_eeprom().
If the read fails, the operation continues using uninitialized data from
eeprom_buff. This results in corrupted data being written back to the
EEPROM for the boundary words.
Add the missing error checks and abort the operation if reading fails.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Co-developed-by: Iskhakov Daniil <dish@amicon.ru>
Signed-off-by: Iskhakov Daniil <dish@amicon.ru>
Signed-off-by: Agalakov Daniil <ade@amicon.ru>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
index ab232b3fbbd0..4dcbeabb3ad2 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
@@ -496,14 +496,19 @@ static int e1000_set_eeprom(struct net_device *netdev,
*/
ret_val = e1000_read_eeprom(hw, first_word, 1,
&eeprom_buff[0]);
+ if (ret_val)
+ goto out;
+
ptr++;
}
- if (((eeprom->offset + eeprom->len) & 1) && (ret_val == 0)) {
+ if ((eeprom->offset + eeprom->len) & 1) {
/* need read/modify/write of last changed EEPROM word
* only the first byte of the word is being modified
*/
ret_val = e1000_read_eeprom(hw, last_word, 1,
&eeprom_buff[last_word - first_word]);
+ if (ret_val)
+ goto out;
}
/* Device's eeprom is always little-endian, word addressable */
@@ -522,6 +527,7 @@ static int e1000_set_eeprom(struct net_device *netdev,
if ((ret_val == 0) && (first_word <= EEPROM_CHECKSUM_REG))
e1000_update_eeprom_checksum(hw);
+out:
kfree(eeprom_buff);
return ret_val;
}
--
2.47.1
^ permalink raw reply related
* [PATCH net 8/9] igb: remove napi_synchronize() in igb_down()
From: Tony Nguyen @ 2026-04-06 21:30 UTC (permalink / raw)
To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev
Cc: Alex Dvoretsky, anthony.l.nguyen, kurt, sriram.yagnaraman,
maciej.fijalkowski, magnus.karlsson, ast, daniel, hawk,
john.fastabend, sdf, bpf, stable, Aleksandr Loktionov,
Patryk Holda
In-Reply-To: <20260406213038.444732-1-anthony.l.nguyen@intel.com>
From: Alex Dvoretsky <advoretsky@gmail.com>
When an AF_XDP zero-copy application terminates abruptly (e.g., kill -9),
the XSK buffer pool is destroyed but NAPI polling continues.
igb_clean_rx_irq_zc() repeatedly returns the full budget, preventing
napi_complete_done() from clearing NAPI_STATE_SCHED.
igb_down() calls napi_synchronize() before napi_disable() for each queue
vector. napi_synchronize() spins waiting for NAPI_STATE_SCHED to clear,
which never happens. igb_down() blocks indefinitely, the TX watchdog
fires, and the TX queue remains permanently stalled.
napi_disable() already handles this correctly: it sets NAPI_STATE_DISABLE.
After a full-budget poll, __napi_poll() checks napi_disable_pending(). If
set, it forces completion and clears NAPI_STATE_SCHED, breaking the loop
that napi_synchronize() cannot.
napi_synchronize() was added in commit 41f149a285da ("igb: Fix possible
panic caused by Rx traffic arrival while interface is down").
napi_disable() provides stronger guarantees: it prevents further
scheduling and waits for any active poll to exit.
Other Intel drivers (ixgbe, ice, i40e) use napi_disable() without a
preceding napi_synchronize() in their down paths.
Remove redundant napi_synchronize() call and reorder napi_disable()
before igb_set_queue_napi() so the queue-to-NAPI mapping is only
cleared after polling has fully stopped.
Fixes: 2c6196013f84 ("igb: Add AF_XDP zero-copy Rx support")
Cc: stable@vger.kernel.org
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Alex Dvoretsky <advoretsky@gmail.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Tested-by: Patryk Holda <patryk.holda@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index ee99fd8fd513..ce91dda00ec0 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2203,9 +2203,8 @@ void igb_down(struct igb_adapter *adapter)
for (i = 0; i < adapter->num_q_vectors; i++) {
if (adapter->q_vector[i]) {
- napi_synchronize(&adapter->q_vector[i]->napi);
- igb_set_queue_napi(adapter, i, NULL);
napi_disable(&adapter->q_vector[i]->napi);
+ igb_set_queue_napi(adapter, i, NULL);
}
}
--
2.47.1
^ permalink raw reply related
* [PATCH net-next 0/3] psp: add crypt-offset and spi-threshold attributes
From: Akhilesh Samineni @ 2026-04-06 22:23 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, andrew+netdev, horms, willemb,
daniel.zahka
Cc: netdev, linux-kernel, jayakrishnan.udayavarma, ajit.khaparde,
kiran.kella, akhilesh.samineni, sachin.suman
This series introduces two new configurable per-device attributes to
the PSP (PSP Security Protocol) subsystem: crypt-offset and spi-threshold.
crypt-offset (Crypt Offset)
----------------------------------
The crypt-offset attribute specifies the byte offset within a packet from
which encryption begins. A non-zero value allows a fixed-size portion of
the packet header to remain in plaintext, which is useful where
intermediate network nodes need to inspect a well-known header before the
encrypted region. The default value is 0, meaning encryption starts
immediately after the PSP header.
spi-threshold (SPI Threshold)
-------------------------------
SPI values are allocated monotonically from a 32-bit per-device space.
The spi-threshold attribute lets userspace configure the SPI value at
which rotation should be triggered before the space is exhausted.
A named constant PSP_SPI_THRESHOLD_DEFAULT (~90% of 0x7FFFFFFF) is
introduced in include/net/psp/types.h as the single authoritative
definition, used by both the PSP core (psp_main.c) and the netdevsim
driver (psp.c). This avoids scattering the magic value across drivers.
A follow-up series will add netlink notification support to alert
subscribed listeners when the configured spi-threshold is reached,
enabling timely SPI rotation.
Akhilesh Samineni (3):
psp: add crypt-offset and spi-threshold get/set attributes
netdevsim: psp: handle the new crypt-offset and spi-threshold get/set operations
selftests: net: psp: add crypt-offset and spi-threshold test cases
Documentation/netlink/specs/psp.yaml | 13 +++++++++
drivers/net/netdevsim/netdevsim.h | 2 ++
drivers/net/netdevsim/psp.c | 6 ++++
include/net/psp/types.h | 7 +++++
include/uapi/linux/psp.h | 2 ++
net/psp/psp-nl-gen.c | 6 ++--
net/psp/psp_main.c | 3 ++
net/psp/psp_nl.c | 27 +++++++++++++++---
tools/testing/selftests/drivers/net/psp.py | 32 +++++++++++++++++++++++
9 files changed, 92 insertions(+), 6 deletions(-)
--
2.45.4
^ permalink raw reply
* [PATCH net-next 1/3] psp: add crypt-offset and spi-threshold get/set attributes
From: Akhilesh Samineni @ 2026-04-06 22:23 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, andrew+netdev, horms, willemb,
daniel.zahka
Cc: netdev, linux-kernel, jayakrishnan.udayavarma, ajit.khaparde,
kiran.kella, akhilesh.samineni, sachin.suman
In-Reply-To: <20260406222305.4111170-1-akhilesh.samineni@broadcom.com>
crypt-offset (Crypt Offset)
----------------------------------
The crypt-offset attribute specifies the byte offset within a packet
from which encryption begins. This is a per-device attribute that
allows a portion of the packet header to remain in plaintext while
the rest of the payload is encrypted. This is useful in scenarios
where intermediate nodes need to inspect or process a fixed-size
header before the encrypted payload.
The default value is 0, meaning encryption starts from the beginning
of the payload following the PSP header.
spi-threshold (SPI Threshold)
------------------------------
The SPI (Security Parameter Index) is a 32-bit per-device identifier
used to distinguish security associations. As SPI values are allocated
monotonically, a threshold is needed to trigger timely SPI rotation
before the space is exhausted.
The spi-threshold attribute allows userspace to configure the value at
which an SPI rotation should be initiated. The default is set to
PSP_SPI_THRESHOLD_DEFAULT (~90% of 0x7FFFFFFF), providing a comfortable
margin to perform rotation without racing to exhaustion.
NOTE: A follow-up series will add notification support to alert
subscribed users when the configured spi-threshold is reached, enabling
timely SPI rotation.
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Reviewed-by: Kiran Kella <kiran.kella@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
Documentation/netlink/specs/psp.yaml | 13 +++++++++++++
include/net/psp/types.h | 7 +++++++
include/uapi/linux/psp.h | 2 ++
net/psp/psp-nl-gen.c | 6 ++++--
net/psp/psp_main.c | 3 +++
net/psp/psp_nl.c | 27 +++++++++++++++++++++++----
6 files changed, 52 insertions(+), 6 deletions(-)
diff --git a/Documentation/netlink/specs/psp.yaml b/Documentation/netlink/specs/psp.yaml
index f3a57782d2cf..b22869be91cf 100644
--- a/Documentation/netlink/specs/psp.yaml
+++ b/Documentation/netlink/specs/psp.yaml
@@ -38,6 +38,15 @@ attribute-sets:
type: u32
enum: version
enum-as-flags: true
+ -
+ name: crypt-offset
+ doc: The offset from the end of the PSP header to the start of the encrypted payload.
+ type: u8
+ -
+ name: spi-threshold
+ doc: Threshold for the SPI to trigger notification to the user for appropriate rotate action.
+ type: u32
+
-
name: assoc
attributes:
@@ -170,6 +179,8 @@ operations:
- ifindex
- psp-versions-cap
- psp-versions-ena
+ - crypt-offset
+ - spi-threshold
pre: psp-device-get-locked
post: psp-device-unlock
dump:
@@ -193,6 +204,8 @@ operations:
attributes:
- id
- psp-versions-ena
+ - crypt-offset
+ - spi-threshold
reply:
attributes: []
pre: psp-device-get-locked
diff --git a/include/net/psp/types.h b/include/net/psp/types.h
index 25a9096d4e7d..875f7822557f 100644
--- a/include/net/psp/types.h
+++ b/include/net/psp/types.h
@@ -25,6 +25,9 @@ struct psphdr {
#define PSP_SPI_KEY_ID GENMASK(30, 0)
#define PSP_SPI_KEY_PHASE BIT(31)
+/* Default SPI threshold: ~90% of max SPI (0x7FFFFFFF) to allow rotation before exhaustion */
+#define PSP_SPI_THRESHOLD_DEFAULT 0x73333333
+
#define PSPHDR_CRYPT_OFFSET GENMASK(5, 0)
#define PSPHDR_VERFL_SAMPLE BIT(7)
@@ -38,9 +41,13 @@ struct psphdr {
/**
* struct psp_dev_config - PSP device configuration
* @versions: PSP versions enabled on the device
+ * @crypt_offset: crypto offset configured on the device
+ * @spi_threshold: SPI threshold value on the device
*/
struct psp_dev_config {
u32 versions;
+ u8 crypt_offset;
+ u32 spi_threshold;
};
/**
diff --git a/include/uapi/linux/psp.h b/include/uapi/linux/psp.h
index a3a336488dc3..bb390159dc72 100644
--- a/include/uapi/linux/psp.h
+++ b/include/uapi/linux/psp.h
@@ -22,6 +22,8 @@ enum {
PSP_A_DEV_IFINDEX,
PSP_A_DEV_PSP_VERSIONS_CAP,
PSP_A_DEV_PSP_VERSIONS_ENA,
+ PSP_A_DEV_CRYPT_OFFSET,
+ PSP_A_DEV_SPI_THRESHOLD,
__PSP_A_DEV_MAX,
PSP_A_DEV_MAX = (__PSP_A_DEV_MAX - 1)
diff --git a/net/psp/psp-nl-gen.c b/net/psp/psp-nl-gen.c
index 22a48d0fa378..e50b8b80955c 100644
--- a/net/psp/psp-nl-gen.c
+++ b/net/psp/psp-nl-gen.c
@@ -23,9 +23,11 @@ static const struct nla_policy psp_dev_get_nl_policy[PSP_A_DEV_ID + 1] = {
};
/* PSP_CMD_DEV_SET - do */
-static const struct nla_policy psp_dev_set_nl_policy[PSP_A_DEV_PSP_VERSIONS_ENA + 1] = {
+static const struct nla_policy psp_dev_set_nl_policy[PSP_A_DEV_SPI_THRESHOLD + 1] = {
[PSP_A_DEV_ID] = NLA_POLICY_MIN(NLA_U32, 1),
[PSP_A_DEV_PSP_VERSIONS_ENA] = NLA_POLICY_MASK(NLA_U32, 0xf),
+ [PSP_A_DEV_CRYPT_OFFSET] = { .type = NLA_U8, },
+ [PSP_A_DEV_SPI_THRESHOLD] = { .type = NLA_U32, },
};
/* PSP_CMD_KEY_ROTATE - do */
@@ -75,7 +77,7 @@ static const struct genl_split_ops psp_nl_ops[] = {
.doit = psp_nl_dev_set_doit,
.post_doit = psp_device_unlock,
.policy = psp_dev_set_nl_policy,
- .maxattr = PSP_A_DEV_PSP_VERSIONS_ENA,
+ .maxattr = PSP_A_DEV_SPI_THRESHOLD,
.flags = GENL_CMD_CAP_DO,
},
{
diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
index 9508b6c38003..536ee44db09d 100644
--- a/net/psp/psp_main.c
+++ b/net/psp/psp_main.c
@@ -79,6 +79,9 @@ psp_dev_create(struct net_device *netdev,
INIT_LIST_HEAD(&psd->stale_assocs);
refcount_set(&psd->refcnt, 1);
+ /* ~90% of 0x7FFFFFFF; allows SPI rotation well before space is exhausted */
+ psd->config.spi_threshold = PSP_SPI_THRESHOLD_DEFAULT;
+
mutex_lock(&psp_devs_lock);
err = xa_alloc_cyclic(&psp_devs, &psd->id, psd, xa_limit_16b,
&last_id, GFP_KERNEL);
diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
index 6afd7707ec12..fbb77460a24b 100644
--- a/net/psp/psp_nl.c
+++ b/net/psp/psp_nl.c
@@ -101,7 +101,9 @@ psp_nl_dev_fill(struct psp_dev *psd, struct sk_buff *rsp,
if (nla_put_u32(rsp, PSP_A_DEV_ID, psd->id) ||
nla_put_u32(rsp, PSP_A_DEV_IFINDEX, psd->main_netdev->ifindex) ||
nla_put_u32(rsp, PSP_A_DEV_PSP_VERSIONS_CAP, psd->caps->versions) ||
- nla_put_u32(rsp, PSP_A_DEV_PSP_VERSIONS_ENA, psd->config.versions))
+ nla_put_u32(rsp, PSP_A_DEV_PSP_VERSIONS_ENA, psd->config.versions) ||
+ nla_put_u8(rsp, PSP_A_DEV_CRYPT_OFFSET, psd->config.crypt_offset) ||
+ nla_put_u32(rsp, PSP_A_DEV_SPI_THRESHOLD, psd->config.spi_threshold))
goto err_cancel_msg;
genlmsg_end(rsp, hdr);
@@ -193,6 +195,13 @@ int psp_nl_dev_set_doit(struct sk_buff *skb, struct genl_info *info)
memcpy(&new_config, &psd->config, sizeof(new_config));
+ if (!info->attrs[PSP_A_DEV_PSP_VERSIONS_ENA] &&
+ !info->attrs[PSP_A_DEV_CRYPT_OFFSET] &&
+ !info->attrs[PSP_A_DEV_SPI_THRESHOLD]) {
+ NL_SET_ERR_MSG(info->extack, "No settings present");
+ return -EINVAL;
+ }
+
if (info->attrs[PSP_A_DEV_PSP_VERSIONS_ENA]) {
new_config.versions =
nla_get_u32(info->attrs[PSP_A_DEV_PSP_VERSIONS_ENA]);
@@ -200,9 +209,19 @@ int psp_nl_dev_set_doit(struct sk_buff *skb, struct genl_info *info)
NL_SET_ERR_MSG(info->extack, "Requested PSP versions not supported by the device");
return -EINVAL;
}
- } else {
- NL_SET_ERR_MSG(info->extack, "No settings present");
- return -EINVAL;
+ }
+
+ if (info->attrs[PSP_A_DEV_CRYPT_OFFSET])
+ new_config.crypt_offset =
+ nla_get_u8(info->attrs[PSP_A_DEV_CRYPT_OFFSET]);
+
+ if (info->attrs[PSP_A_DEV_SPI_THRESHOLD]) {
+ new_config.spi_threshold =
+ nla_get_u32(info->attrs[PSP_A_DEV_SPI_THRESHOLD]);
+ if (new_config.spi_threshold & PSP_SPI_KEY_PHASE) {
+ NL_SET_ERR_MSG(info->extack, "SPI threshold must not have bit 31 set");
+ return -EINVAL;
+ }
}
rsp = psp_nl_reply_new(info);
--
2.45.4
^ permalink raw reply related
* [PATCH net-next 2/3] netdevsim: psp: handle the new crypt-offset and spi-threshold get/set operations
From: Akhilesh Samineni @ 2026-04-06 22:23 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, andrew+netdev, horms, willemb,
daniel.zahka
Cc: netdev, linux-kernel, jayakrishnan.udayavarma, ajit.khaparde,
kiran.kella, akhilesh.samineni, sachin.suman
In-Reply-To: <20260406222305.4111170-1-akhilesh.samineni@broadcom.com>
Implement the crypt-offset and spi-threshold get/set in netdevsim PSP.
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Reviewed-by: Kiran Kella <kiran.kella@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
drivers/net/netdevsim/netdevsim.h | 2 ++
drivers/net/netdevsim/psp.c | 6 ++++++
2 files changed, 8 insertions(+)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index c904e14f6b3f..3ad7d42391c0 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -117,6 +117,8 @@ struct netdevsim {
struct psp_dev *dev;
u32 spi;
u32 assoc_cnt;
+ u8 crypt_offset;
+ u32 spi_threshold;
} psp;
struct nsim_bus_dev *nsim_bus_dev;
diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c
index 0b4d717253b0..9098edf00c5c 100644
--- a/drivers/net/netdevsim/psp.c
+++ b/drivers/net/netdevsim/psp.c
@@ -122,6 +122,11 @@ static int
nsim_psp_set_config(struct psp_dev *psd, struct psp_dev_config *conf,
struct netlink_ext_ack *extack)
{
+ struct netdevsim *ns = psd->drv_priv;
+
+ ns->psp.crypt_offset = conf->crypt_offset;
+ ns->psp.spi_threshold = conf->spi_threshold;
+
return 0;
}
@@ -249,6 +254,7 @@ int nsim_psp_init(struct netdevsim *ns)
if (err)
return err;
+ ns->psp.spi_threshold = PSP_SPI_THRESHOLD_DEFAULT;
debugfs_create_file("psp_rereg", 0200, ddir, ns, &nsim_psp_rereg_fops);
return 0;
}
--
2.45.4
^ permalink raw reply related
* [PATCH net-next 3/3] selftests: net: psp: add crypt-offset and spi-threshold test cases
From: Akhilesh Samineni @ 2026-04-06 22:23 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, andrew+netdev, horms, willemb,
daniel.zahka
Cc: netdev, linux-kernel, jayakrishnan.udayavarma, ajit.khaparde,
kiran.kella, akhilesh.samineni, sachin.suman
In-Reply-To: <20260406222305.4111170-1-akhilesh.samineni@broadcom.com>
Add test cases to set and get crypt-offset and spi-threshold attributes,
verifying both the applied value and the restored prior value.
Signed-off-by: Akhilesh Samineni <akhilesh.samineni@broadcom.com>
Reviewed-by: Kiran Kella <kiran.kella@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
tools/testing/selftests/drivers/net/psp.py | 32 ++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
index 864d9fce1094..9253aab29ded 100755
--- a/tools/testing/selftests/drivers/net/psp.py
+++ b/tools/testing/selftests/drivers/net/psp.py
@@ -171,6 +171,38 @@ def dev_get_device_bad(cfg):
ksft_true(raised)
+def dev_set_crypt_offset(cfg):
+ """ Set and get the crypt-offset """
+ _init_psp_dev(cfg)
+
+ dev = cfg.pspnl.dev_get({'id': cfg.psp_dev_id})
+ orig = dev['crypt-offset']
+ cfg.pspnl.dev_set({"id": cfg.psp_dev_id,
+ "crypt-offset": 5})
+ dev = cfg.pspnl.dev_get({'id': cfg.psp_dev_id})
+ ksft_eq(dev['crypt-offset'], 5)
+ cfg.pspnl.dev_set({"id": cfg.psp_dev_id,
+ "crypt-offset": orig})
+ dev = cfg.pspnl.dev_get({'id': cfg.psp_dev_id})
+ ksft_eq(dev['crypt-offset'], orig)
+
+
+def dev_set_spi_threshold(cfg):
+ """ Set and get the spi-threshold """
+ _init_psp_dev(cfg)
+
+ dev = cfg.pspnl.dev_get({'id': cfg.psp_dev_id})
+ orig = dev['spi-threshold']
+ cfg.pspnl.dev_set({"id": cfg.psp_dev_id,
+ "spi-threshold": 10})
+ dev = cfg.pspnl.dev_get({'id': cfg.psp_dev_id})
+ ksft_eq(dev['spi-threshold'], 10)
+ cfg.pspnl.dev_set({"id": cfg.psp_dev_id,
+ "spi-threshold": orig})
+ dev = cfg.pspnl.dev_get({'id': cfg.psp_dev_id})
+ ksft_eq(dev['spi-threshold'], orig)
+
+
def dev_rotate(cfg):
""" Test key rotation """
_init_psp_dev(cfg)
--
2.45.4
^ permalink raw reply related
* Re: [PATCH net-next v5 09/14] iavf: convert to ndo_set_rx_mode_async (b1dc10d5dff2)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, jiri, jv
In-Reply-To: <20260404000614.2414164-1-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.
> ---
> iavf: convert to ndo_set_rx_mode_async
>
> This commit converts the iavf driver from using ndo_set_rx_mode to
> ndo_set_rx_mode_async. It updates iavf_set_rx_mode to take explicit
> unicast and multicast list parameters and uses __hw_addr_sync_dev on
> the snapshots instead of __dev_uc_sync and __dev_mc_sync.
>
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -1150,14 +1150,18 @@ bool iavf_promiscuous_mode_changed(struct iavf_adapter *adapter)
> > -static void iavf_set_rx_mode(struct net_device *netdev)
> > +static void iavf_set_rx_mode(struct net_device *netdev,
> > + struct netdev_hw_addr_list *uc,
> > + struct netdev_hw_addr_list *mc)
> > {
> > struct iavf_adapter *adapter = netdev_priv(netdev);
> >
> > spin_lock_bh(&adapter->mac_vlan_list_lock);
> > - __dev_uc_sync(netdev, iavf_addr_sync, iavf_addr_unsync);
> > - __dev_mc_sync(netdev, iavf_addr_sync, iavf_addr_unsync);
> > + __hw_addr_sync_dev(uc, netdev, iavf_addr_sync, iavf_addr_unsync);
> > + __hw_addr_sync_dev(mc, netdev, iavf_addr_sync, iavf_addr_unsync);
> > spin_unlock_bh(&adapter->mac_vlan_list_lock);
>
> [ ... ]
>
> > @@ -1210,7 +1214,7 @@ static void iavf_configure(struct iavf_adapter *adapter)
> > struct net_device *netdev = adapter->netdev;
> > int i;
> >
> > - iavf_set_rx_mode(netdev);
> > + iavf_set_rx_mode(netdev, &netdev->uc, &netdev->mc);
>
> Does this code iterate and mutate the live netdev address lists without
> holding the required locks?
>
> Previously, iavf_set_rx_mode() called __dev_uc_sync() and __dev_mc_sync(),
> which internally acquire netif_addr_lock_bh() to protect the live netdev
> address lists.
>
> Now, iavf_configure() passes the live lists (&netdev->uc and &netdev->mc)
> directly to iavf_set_rx_mode(), which calls __hw_addr_sync_dev() on them.
> __hw_addr_sync_dev() iterates and modifies the lists by updating sync_cnt,
> refcount, and removing stale entries.
>
> Since iavf_configure() is called from contexts like iavf_open() and the
> driver's watchdog task, it appears netif_addr_lock_bh() is not held here.
>
> Could this cause list or RB-tree corruption if there are concurrent
> modifications, such as IGMP/MLD timers adding multicast addresses in softirq
> while holding netif_addr_lock_bh()?
Most of this is seems bogus:
- ndo_set_rx_mode_async path is fine because of the snapshot
- iavf_configure is fine because of the ops lock
What I can do is to wrap iavf_set_rx_mode in iavf_configure into
netif_addr_lock_bh to satisfy the bots?
^ permalink raw reply
* Re: [PATCH net-next v5 01/14] net: add address list snapshot and reconciliation infrastructure (123ac7a76378)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, jiri, jv
In-Reply-To: <20260404000602.2413848-1-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?
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.
^ permalink raw reply
* Re: [PATCH net-next v5 07/14] bnxt: convert to ndo_set_rx_mode_async (c1776bbe53ec)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, jiri, jv
In-Reply-To: <20260404000611.2414085-1-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.
> ---
> bnxt: convert to ndo_set_rx_mode_async
>
> This commit converts the bnxt driver to use ndo_set_rx_mode_async, updating
> the rx mode callbacks to take explicit list parameters and iterate over them.
>
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index fe8b886ff82ea..569c7de7c5e44 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>
> [ ... ]
>
> > @@ -13523,17 +13524,17 @@ void bnxt_get_ring_drv_stats(struct bnxt *bp,
> > bnxt_get_one_ring_drv_stats(bp, stats, &bp->bnapi[i]->cp_ring);
> > }
> >
> > -static bool bnxt_mc_list_updated(struct bnxt *bp, u32 *rx_mask)
> > +static bool bnxt_mc_list_updated(struct bnxt *bp, u32 *rx_mask,
> > + const struct netdev_hw_addr_list *mc)
> > {
> > struct bnxt_vnic_info *vnic = &bp->vnic_info[BNXT_VNIC_DEFAULT];
> > - struct net_device *dev = bp->dev;
> > struct netdev_hw_addr *ha;
> > u8 *haddr;
> > int mc_count = 0;
> > bool update = false;
> > int off = 0;
> >
> > - netdev_for_each_mc_addr(ha, dev) {
> > + netdev_hw_addr_list_for_each(ha, mc) {
>
> When iterating over the list, bnxt_mc_list_updated() performs a memcpy()
> into vnic->mc_list.
>
> If this executes asynchronously while bnxt_cfg_rx_mode() is mapping
> vnic->mc_list for DMA to the firmware via bnxt_hwrm_cfa_l2_set_rx_mask(),
> could the NIC hardware read torn or corrupted multicast MAC addresses?
>
> Even if a later patch addresses the explicit list TOCTOU issue, is a
> driver-level spinlock needed here to serialize vnic state access between
> the two concurrent workqueues?
This is not something new I think? The same split was already happening
prior to conversion: bnxt_set_rx_mode updates mc_list and queues
BNXT_RX_MASK_SP_EVENT while bnxt_sp_task does bnxt_cfg_rx_mode.
After the conversion, both paths should be under ops lock, so we should
be in a better place.
>
> [ ... ]
>
> > @@ -15881,7 +15884,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
> > .ndo_start_xmit = bnxt_start_xmit,
> > .ndo_stop = bnxt_close,
> > .ndo_get_stats64 = bnxt_get_stats64,
> > - .ndo_set_rx_mode = bnxt_set_rx_mode,
> > + .ndo_set_rx_mode_async = bnxt_set_rx_mode,
>
> Since this callback now runs asynchronously without the netif_addr_lock_bh()
> that the core networking stack previously held, does this introduce data races
> on the driver state?
>
> With bnxt_set_rx_mode() executing locklessly in the netdev_rx_mode_work
> workqueue, and bnxt_cfg_rx_mode() concurrently modifying vnic->uc_list,
> vnic->uc_filter_count, and vnic->rx_mask in the driver's bnxt_sp_task
> workqueue, can they race when accessing the vnic state?
>
> For example, could lockless assignments to vnic->rx_mask in bnxt_set_rx_mode()
> temporarily drop hardware fallback states (like the PROMISCUOUS bit) set by
> bnxt_cfg_rx_mode()?
Hmm, this is completely bogus? I don't see "lockless assignments". After
async conversion all these paths run fully under ops lock.
^ permalink raw reply
* Re: [PATCH net-next v5 02/14] net: introduce ndo_set_rx_mode_async and netdev_rx_mode_work (61d75e67dcd2)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, jiri, jv
In-Reply-To: <20260404000604.2413879-1-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.
> ---
> Subject: net: introduce ndo_set_rx_mode_async and netdev_rx_mode_work
>
> This commit introduces an async variant of ndo_set_rx_mode that runs from a
> workqueue, allowing drivers to sleep during RX mode changes. It also adds
> netif_rx_mode_sync() to opportunistically flush pending updates before
> returning to userspace.
>
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> [ ... ]
> > @@ -12217,6 +12178,9 @@ void free_netdev(struct net_device *dev)
> >
> > kfree(rcu_dereference_protected(dev->ingress_queue, 1));
> >
> > + if (netif_rx_mode_clean(dev))
> > + dev_put(dev);
> > +
> > /* Flush device addresses */
> > dev_addr_flush(dev);
>
> Is this cleanup reachable during normal shutdown?
>
> When a device is queued to rx_mode_list, netif_rx_mode_queue() elevates its
> reference count via dev_hold(). During unregistration, netdev_wait_allrefs()
> blocks the system until all references are dropped.
>
> Because of this wait, the system shouldn't be able to reach free_netdev()
> until the worker thread has already dequeued the device and called
> dev_put(). It seems the device is guaranteed to not be on the list by the
> time this code executes.
I guess, yeah, we can drop this. netdev_wait_allrefs waits for all refs
to go away, so this is not needed.
>
> > --- a/net/core/dev_addr_lists.c
> > +++ b/net/core/dev_addr_lists.c
> [ ... ]
> > +static void netdev_rx_mode_work(struct work_struct *work)
> > +{
> > + struct net_device *dev;
> > +
> > + rtnl_lock();
> > +
> > + while (true) {
> > + spin_lock_bh(&rx_mode_lock);
> > + if (list_empty(&rx_mode_list)) {
> > + spin_unlock_bh(&rx_mode_lock);
> > + break;
> > + }
> > + dev = list_first_entry(&rx_mode_list, struct net_device,
> > + rx_mode_node);
> > + list_del_init(&dev->rx_mode_node);
> > + spin_unlock_bh(&rx_mode_lock);
> > +
> > + netdev_lock_ops(dev);
>
> Can this while loop starve the system of the rtnl lock?
>
> Since userspace can trigger RX mode updates via ioctls like SIOCADDMULTI
> while holding only netdev_lock_ops() and not the rtnl lock, a continuous
> stream of these operations could repeatedly add devices to rx_mode_list.
>
> Because the loop releases rx_mode_lock while processing but retains the rtnl
> lock, could it run indefinitely if the list is continually replenished?
Idk, this seems overly paranoid, the same work would happen under rtnl
on the main thread if we don't do a async callback.
> Additionally, does the order of list_del_init() and netdev_lock_ops() create
> a race condition with netif_rx_mode_sync()?
>
> If the worker thread dequeues the device and drops the spinlock, it might
> then block waiting for netdev_lock_ops(). Meanwhile, an ioctl thread
> holding netdev_lock_ops() might call netif_rx_mode_sync().
>
> The ioctl thread would call netif_rx_mode_clean(), which checks
> list_empty(). Since the worker already removed the device from the list,
> clean returns false. This causes netif_rx_mode_sync() to act as a no-op,
> allowing the ioctl to return to userspace before the hardware update
> actually completes.
The worst thing that can happen is we re-queue empty work. I don't see
how we can return to userspace without having a sync update.
>
> > --- a/net/core/dev_api.c
> > +++ b/net/core/dev_api.c
> [ ... ]
> > @@ -311,6 +313,7 @@ int dev_set_allmulti(struct net_device *dev, int inc)
> >
> > netdev_lock_ops(dev);
> > ret = netif_set_allmulti(dev, inc, true);
> > + netif_rx_mode_sync(dev);
> > netdev_unlock_ops(dev);
> >
> > return ret;
>
> Does this correctly handle cascaded updates for stacked devices?
>
> When operating on a stacked device like a VLAN, the RX mode update cascades
> to the underlying physical device, queuing the physical device to the
> rx_mode_list via __dev_set_rx_mode().
>
> Since netif_rx_mode_sync() is only called on the stacked device here, and
> the stacked device itself is not on the rx_mode_list, will the physical
> device's update be left on the workqueue? This seems to bypass the
> synchronous update guarantee.
Yes, this assessment is correct in general. Not sure we want some new
netif_rx_mode_deep_sync or (probably better?) add some netif_rx_mode_sync
calls where appropriate. For now, leaving netif_rx_mode_sync in a few
places and planning to add more netif_rx_mode_sync if/when issues with
deep hierarchy syncing arise.
^ permalink raw reply
* Re: [PATCH net-next v5 02/14] net: introduce ndo_set_rx_mode_async and netdev_rx_mode_work (61d75e67dcd2)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: netdev, davem, edumazet, pabeni
In-Reply-To: <20260403172707.28e71a27@kernel.org>
> On Thu, 2 Apr 2026 15:55:23 -0700 Stanislav Fomichev wrote:
> > + if (netif_rx_mode_clean(dev))
> > + dev_put(dev);
>
> let's toss a netdev_tracker into this reference counting
> but overall the work stealing is looking cleaner than I expected,
> nicely done!
Sure, will do!
^ permalink raw reply
* Re: [PATCH net-next v5 03/14] net: move promiscuity handling into netdev_rx_mode_work (ddeab417d841)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, jiri, jv
In-Reply-To: <20260404000606.2413916-1-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: move promiscuity handling into netdev_rx_mode_work
>
> This commit moves unicast promiscuity tracking into netdev_rx_mode_work so
> it runs under netdev_ops_lock instead of the addr_lock spinlock. It also
> changes ASSERT_RTNL() to netdev_ops_assert_locked() in several functions
> called from the work queue.
>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 0efac0c730077..08dac9c759996 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9586,7 +9586,7 @@ int __dev_set_promiscuity(struct net_device *dev, int inc, bool notify)
> > kuid_t uid;
> > kgid_t gid;
> >
> > - ASSERT_RTNL();
> > + netdev_ops_assert_locked(dev);
>
> Can this cause a lockdep assertion failure for unconverted ops-locked devices
> using the legacy RX mode path?
Correct and addressed later in the series in "net: warn ops-locked drivers
still using ndo_set_rx_mode". Don't think there is a better way to split
the patches...
> Replacing ASSERT_RTNL() with netdev_ops_assert_locked(dev) asserts that
> dev->lock is held for devices where netdev_need_ops_lock(dev) is true.
>
> [ ... ]
>
> > diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> > index a05db3ac43ea9..61589537b2d3d 100644
> > --- a/net/core/dev_addr_lists.c
> > +++ b/net/core/dev_addr_lists.c
> [ ... ]
> > @@ -1295,20 +1337,16 @@ void __dev_set_rx_mode(struct net_device *dev)
> > if (!netif_device_present(dev))
> > return;
> >
> > - if (ops->ndo_set_rx_mode_async) {
> > + if (ops->ndo_set_rx_mode_async || ops->ndo_change_rx_flags) {
> > netif_rx_mode_queue(dev);
> > return;
> > }
> >
> > - if (!(dev->priv_flags & IFF_UNICAST_FLT)) {
> > - if (!netdev_uc_empty(dev) && !dev->uc_promisc) {
> > - __dev_set_promiscuity(dev, 1, false);
> > - dev->uc_promisc = true;
> > - } else if (netdev_uc_empty(dev) && dev->uc_promisc) {
> > - __dev_set_promiscuity(dev, -1, false);
> > - dev->uc_promisc = false;
> > - }
> > - }
> > + /* Legacy path for non-ops-locked HW devices. */
> > +
> > + promisc_inc = netif_uc_promisc_update(dev);
> > + if (promisc_inc)
> > + __dev_set_promiscuity(dev, promisc_inc, false);
>
> If an ops-locked device has not yet been converted to implement the new
> ndo_set_rx_mode_async or ndo_change_rx_flags callbacks, this legacy path
> will be executed.
Same here, addressed later in the series in "net: warn ops-locked drivers
still using ndo_set_rx_mode".
>
> When a secondary unicast address is added to such a device (e.g., by
> bringing up a macvlan interface over it) and the device lacks
> IFF_UNICAST_FLT, netif_uc_promisc_update() returns 1. This triggers a
> synchronous inline call to __dev_set_promiscuity(dev, 1, false).
>
> Since __dev_set_rx_mode() is invoked from address list helpers like
> dev_uc_add() which only hold rtnl_lock() and netif_addr_lock_bh(dev),
> dev->lock is not held, which would trigger the new assertion in
> __dev_set_promiscuity().
Same.
^ permalink raw reply
* Re: [PATCH net-next v5 05/14] fbnic: convert to ndo_set_rx_mode_async (1d5e76c60ed0)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, jiri, jv
In-Reply-To: <20260404000608.2414015-1-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.
> ---
> fbnic: convert to ndo_set_rx_mode_async
>
> Convert fbnic from ndo_set_rx_mode to ndo_set_rx_mode_async, updating
> the driver to take explicit uc/mc lists and using __hw_addr_sync_dev()
> on the snapshots instead of __dev_uc_sync() on the netdev directly.
>
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> > index b4b396ca9bce3..c406a3b56b37f 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> [ ... ]
> > @@ -213,10 +215,10 @@ void __fbnic_set_rx_mode(struct fbnic_dev *fbd)
> > }
> >
> > /* Synchronize unicast and multicast address lists */
> > - err = __dev_uc_sync(netdev, fbnic_uc_sync, fbnic_uc_unsync);
> > + err = __hw_addr_sync_dev(uc, netdev, fbnic_uc_sync, fbnic_uc_unsync);
> > if (err == -ENOSPC)
> > uc_promisc = true;
> > - err = __dev_mc_sync(netdev, fbnic_mc_sync, fbnic_mc_unsync);
> > + err = __hw_addr_sync_dev(mc, netdev, fbnic_mc_sync, fbnic_mc_unsync);
>
> Since __fbnic_set_rx_mode() performs read-modify-write operations on the
> internal fbd->mac_addr array and updates the device TCAM, is there a need
> for internal synchronization?
>
> It appears that concurrent out-of-band callers like fbnic_bmc_rpc_check()
> (from an unlocked workqueue), RTNL paths like fbnic_set_mac(), and the async
> RX mode workqueue can race on the internal fbd->mac_addr array and interleave
> writes to the hardware, potentially breaking the RX filtering rules.
This is bogus, both paths are rntl/ops protested.
>
> > if (err == -ENOSPC)
> > mc_promisc = true;
> [ ... ]
> > @@ -257,7 +262,8 @@ static int fbnic_set_mac(struct net_device *netdev, void *p)
> >
> > eth_hw_addr_set(netdev, addr->sa_data);
> >
> > - fbnic_set_rx_mode(netdev);
> > + if (netif_running(netdev))
> > + __fbnic_set_rx_mode(fbn->fbd, &netdev->uc, &netdev->mc);
>
> Is it safe to pass the live &netdev->uc and &netdev->mc address lists to
> __hw_addr_sync_dev() without holding netif_addr_lock_bh() or rtnl_lock()?
>
> This bypasses the core network stack's lock, which could lead to list
> corruption and use-after-free if the network stack concurrently modifies
> the address lists (e.g., via dev_mc_add()).
>
> Additionally, doesn't manually mutating sync_cnt on the live lists bypass
> the core's snapshot reconciliation state machine?
This is the same as before, nothing new added here (and, again, under
rtnl/ops lock).
>
> >
> > return 0;
> > }
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> > index e3aebbe3656d5..6b139cf542567 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> [ ... ]
> > @@ -180,7 +180,7 @@ static int fbnic_fw_config_after_crash(struct fbnic_dev *fbd)
> > }
> >
> > fbnic_rpc_reset_valid_entries(fbd);
> > - __fbnic_set_rx_mode(fbd);
> > + __fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
>
> Similarly here, fbnic_fw_config_after_crash() appears to be executed from the
> unlocked fbnic_service_task workqueue. Could this cause lockless data races on
> the live network stack lists?
Nothing is lockless here, everything is under rtnl at least.
>
> >
> > return 0;
> > }
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > index 42a186db43ea9..fe95b6f696463 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> [ ... ]
> > @@ -244,7 +244,7 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
> >
> > if (fbd->fw_cap.need_bmc_tcam_reinit) {
> > fbnic_bmc_rpc_init(fbd);
> > - __fbnic_set_rx_mode(fbd);
> > + __fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
>
> And here as well, passing live address lists without synchronization.
Same here, nothing technically changed and this runs under rtnl.
^ permalink raw reply
* Re: [PATCH net-next v5 06/14] mlx5: convert to ndo_set_rx_mode_async (3691f90f6593)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, jiri, jv
In-Reply-To: <20260404000609.2414051-1-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.
> ---
> mlx5: convert to ndo_set_rx_mode_async
>
> This commit converts the mlx5 driver to use ndo_set_rx_mode_async. It passes
> the uc and mc snapshots directly to the internal work handlers instead of
> queueing work and manually iterating the address lists.
>
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
> > @@ -609,20 +609,26 @@ static void mlx5e_execute_l2_action(struct mlx5e_flow_steering *fs,
> > }
> >
> > static void mlx5e_sync_netdev_addr(struct mlx5e_flow_steering *fs,
> > - struct net_device *netdev)
> > + struct net_device *netdev,
> > + struct netdev_hw_addr_list *uc,
> > + struct netdev_hw_addr_list *mc)
> > {
> > struct netdev_hw_addr *ha;
> >
> > - netif_addr_lock_bh(netdev);
> > + if (!uc || !mc) {
> > + netif_addr_lock_bh(netdev);
> > + mlx5e_sync_netdev_addr(fs, netdev, &netdev->uc, &netdev->mc);
> > + netif_addr_unlock_bh(netdev);
> > + return;
> > + }
> >
> > mlx5e_add_l2_to_hash(fs->l2.netdev_uc, netdev->dev_addr);
>
> Can this lead to a torn read of the MAC address?
>
> When uc and mc are provided, this path now reads netdev->dev_addr without
> holding netif_addr_lock_bh().
>
> Because ndo_set_rx_mode_async runs asynchronously in a workqueue, this read
> could happen concurrently with a MAC address change in mlx5e_set_mac().
> Since mlx5e_set_mac() updates the address using eth_hw_addr_set() under
> netif_addr_lock_bh(), a concurrent read here might be torn and program a
> corrupted MAC address into the hardware filters.
All existing netif_set_mac_address paths (and, hence, mlx5e_set_mac) run under
rtnl/ops lock.
^ permalink raw reply
* Re: [PATCH net-next v5 08/14] bnxt: use snapshot in bnxt_cfg_rx_mode (74e346419df6)
From: sdf.kernel @ 2026-04-06 22:29 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jakub Kicinski, netdev, davem, edumazet, pabeni, jiri, jv
In-Reply-To: <20260404000612.2414120-1-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.
> ---
> bnxt: use snapshot in bnxt_cfg_rx_mode
>
> With the introduction of ndo_set_rx_mode_async we can call bnxt_cfg_rx_mode
> directly. Convert bnxt_cfg_rx_mode to use uc/mc snapshots and move its
> call in bnxt_sp_task to the section that resets BNXT_STATE_IN_SP_TASK.
>
> Link: https://lore.kernel.org/netdev/CACKFLi=5vj8hPqEUKDd8RTw3au5G+zRgQEqjF+6NZnyoNm90KA@mail.gmail.com/
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> [ ... ]
> > -static int bnxt_cfg_rx_mode(struct bnxt *bp)
> > +static int bnxt_cfg_rx_mode(struct bnxt *bp, struct netdev_hw_addr_list *uc,
> > + struct netdev_hw_addr_list *mc)
>
> This isn't a bug, but is the mc parameter used in this function? The
> multicast configuration appears to be handled synchronously prior to this call
> by bnxt_mc_list_updated().
True, mc is fully consumed in bnxt_set_rx_mode (bnxt_mc_list_updated), will
drop.
>
> > {
> > struct net_device *dev = bp->dev;
> > struct bnxt_vnic_info *vnic = &bp->vnic_info[BNXT_VNIC_DEFAULT];
> > struct netdev_hw_addr *ha;
> > int i, off = 0, rc;
> > - bool uc_update;
> > -
> > - netif_addr_lock_bh(dev);
> > - uc_update = bnxt_uc_list_updated(bp, &dev->uc);
> > - netif_addr_unlock_bh(dev);
> > -
> > - if (!uc_update)
> > - goto skip_uc;
>
> By removing this check, will the driver now unconditionally delete and recreate
> all secondary hardware unicast MAC filters via HWRM commands on any RX mode
> change?
>
> For example, if an unprivileged user joins a multicast group via
> IP_ADD_MEMBERSHIP, does this create a time window where the hardware drops
> valid unicast traffic destined for secondary MACs?
>
> Also, if any of the firmware commands to re-add a filter fail, will the
> function return early and skip programming the RX mask entirely?
I don't think this is problematic, but I will the original uc_update and
bring back skip_uc, that will skip unnecessary uc list transactions.
>
> [ ... ]
> > @@ -14684,6 +14675,12 @@ static void bnxt_sp_task(struct work_struct *work)
> > /* These functions below will clear BNXT_STATE_IN_SP_TASK. They
> > * must be the last functions to be called before exiting.
> > */
> > + if (test_and_clear_bit(BNXT_RX_MASK_SP_EVENT, &bp->sp_event)) {
> > + bnxt_lock_sp(bp);
>
> Could this introduce a race condition with bnxt_close()?
>
> bnxt_lock_sp() temporarily clears the BNXT_STATE_IN_SP_TASK bit before
> blocking on netdev_lock(bp->dev). If bnxt_close() is called concurrently, it
> holds netdev_lock() and loops waiting for BNXT_STATE_IN_SP_TASK to clear.
>
> When bnxt_lock_sp() clears the bit, bnxt_close() could unblock, shut down the
> device, and free memory including bp->vnic_info via bnxt_free_vnics().
>
> After bnxt_close() completes and releases the lock, bnxt_sp_task() would
> acquire it and unconditionally call bnxt_cfg_rx_mode(), which dereferences the
> now-freed bp->vnic_info.
>
> Would it be safer to check test_bit(BNXT_STATE_OPEN, &bp->state) after
> acquiring the lock, similar to what neighboring handlers do?
Will do, this seems similar to handling in bnxt_reset.
^ permalink raw reply
* [PATCH] bpf: add is_locked_tcp_sock guard for sock_ops rtt_min access
From: Werner Kasselman @ 2026-04-06 22:49 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Martin KaFai Lau, John Fastabend, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Lawrence Brakmo, bpf@vger.kernel.org,
netdev@vger.kernel.org, stable@vger.kernel.org
sock_ops_convert_ctx_access() generates BPF instructions to inline
context field accesses for BPF_PROG_TYPE_SOCK_OPS programs. For
tcp_sock-specific fields like snd_cwnd, srtt_us, etc., it uses the
SOCK_OPS_GET_TCP_SOCK_FIELD() macro which checks is_locked_tcp_sock
and returns 0 when the socket is not a locked full TCP socket.
However, the rtt_min field bypasses this guard entirely: it emits a raw
two-instruction load sequence (load sk pointer, then load from
tcp_sock->rtt_min offset) without checking is_locked_tcp_sock first.
This is a problem because bpf_skops_hdr_opt_len() and
bpf_skops_write_hdr_opt() in tcp_output.c set sock_ops.sk to a
tcp_request_sock (cast from request_sock) during SYN-ACK processing,
with is_fullsock=0 and is_locked_tcp_sock=0. If a SOCK_OPS program
with BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG reads ctx->rtt_min in this
callback, the generated code treats the tcp_request_sock pointer as a
tcp_sock and reads at offsetof(struct tcp_sock, rtt_min) -- which is
well past the end of the tcp_request_sock allocation, causing an
out-of-bounds slab read.
The rtt_min field was introduced in the same commit as the other
tcp_sock fields but was given hand-rolled access code because it reads
a sub-field (rtt_min.s[0].v, a minmax_sample) rather than a direct
struct member, making it incompatible with the SOCK_OPS_GET_FIELD()
macro. This hand-rolled code omitted the is_fullsock guard that the
macro provides. The guard was later renamed to is_locked_tcp_sock in
commit fd93eaffb3f9 ("bpf: Prevent unsafe access to the sock fields in the BPF timestamping callback").
Add the is_locked_tcp_sock guard to the rtt_min case, replicating the
exact instruction pattern used by SOCK_OPS_GET_FIELD() including
proper handling of the dst_reg==src_reg case with temp register
save/restore. Use offsetof(struct minmax_sample, v) for the sub-field
offset to match the style in bpf_tcp_sock_convert_ctx_access().
Found via AST-based call-graph analysis using sqry.
Fixes: 44f0e43037d3 ("bpf: Add support for reading sk_state and more")
Cc: stable@vger.kernel.org
Signed-off-by: Werner Kasselman <werner@verivus.com>
---
net/core/filter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 44 insertions(+), 3 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 78b548158fb0..58f0735b18d9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -10830,13 +10830,54 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
BUILD_BUG_ON(sizeof(struct minmax) <
sizeof(struct minmax_sample));
+ /* Unlike other tcp_sock fields that use
+ * SOCK_OPS_GET_TCP_SOCK_FIELD(), rtt_min requires a
+ * custom access pattern because it reads a sub-field
+ * (rtt_min.s[0].v) rather than a direct struct member.
+ * We must still guard the access with is_locked_tcp_sock
+ * to prevent an OOB read when sk points to a
+ * tcp_request_sock (e.g., during SYN-ACK processing via
+ * bpf_skops_hdr_opt_len/bpf_skops_write_hdr_opt).
+ */
+ off = offsetof(struct tcp_sock, rtt_min) +
+ offsetof(struct minmax_sample, v);
+ {
+ int fullsock_reg = si->dst_reg, reg = BPF_REG_9, jmp = 2;
+
+ if (si->dst_reg == reg || si->src_reg == reg)
+ reg--;
+ if (si->dst_reg == reg || si->src_reg == reg)
+ reg--;
+ if (si->dst_reg == si->src_reg) {
+ *insn++ = BPF_STX_MEM(BPF_DW, si->src_reg, reg,
+ offsetof(struct bpf_sock_ops_kern,
+ temp));
+ fullsock_reg = reg;
+ jmp += 2;
+ }
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
+ struct bpf_sock_ops_kern,
+ is_locked_tcp_sock),
+ fullsock_reg, si->src_reg,
+ offsetof(struct bpf_sock_ops_kern,
+ is_locked_tcp_sock));
+ *insn++ = BPF_JMP_IMM(BPF_JEQ, fullsock_reg, 0, jmp);
+ if (si->dst_reg == si->src_reg)
+ *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg,
+ offsetof(struct bpf_sock_ops_kern,
+ temp));
*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
struct bpf_sock_ops_kern, sk),
si->dst_reg, si->src_reg,
offsetof(struct bpf_sock_ops_kern, sk));
- *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
- offsetof(struct tcp_sock, rtt_min) +
- sizeof_field(struct minmax_sample, t));
+ *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg, off);
+ if (si->dst_reg == si->src_reg) {
+ *insn++ = BPF_JMP_A(1);
+ *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg,
+ offsetof(struct bpf_sock_ops_kern,
+ temp));
+ }
+ }
break;
case offsetof(struct bpf_sock_ops, bpf_sock_ops_cb_flags):
--
2.43.0
^ permalink raw reply related
* Re: [PATCH bpf v2 2/2] selftests/bpf: Add tests for sock_ops ctx access with same src/dst register
From: Martin KaFai Lau @ 2026-04-06 23:01 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bpf, Daniel Borkmann, John Fastabend, Stanislav Fomichev,
Alexei Starovoitov, Andrii Nakryiko, Eduard Zingerman,
Kumar Kartikeya Dwivedi, Song Liu, Yonghong Song, Jiri Olsa,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Shuah Khan, linux-kernel, netdev, linux-kselftest
In-Reply-To: <20260406031330.187630-2-jiayuan.chen@linux.dev>
On Mon, Apr 06, 2026 at 11:12:51AM +0800, Jiayuan Chen wrote:
> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c b/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c
> new file mode 100644
> index 0000000000000..e086f7abb197e
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_ops_get_sk.c
> @@ -0,0 +1,77 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "cgroup_helpers.h"
> +#include "network_helpers.h"
> +#include "sock_ops_get_sk.skel.h"
> +
> +/* See progs/sock_ops_get_sk.c for the bug description. */
> +static void run_sock_ops_test(struct sock_ops_get_sk *skel, int cgroup_fd,
skel is not used. sashiko has flagged it also.
pw-bot: cr
> + int prog_fd)
> +{
> + int server_fd, client_fd, err;
> +
> + err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
> + if (!ASSERT_OK(err, "prog_attach"))
> + return;
> +
> + server_fd = start_server(AF_INET, SOCK_STREAM, NULL, 0, 0);
> + if (!ASSERT_GE(server_fd, 0, "start_server"))
> + goto detach;
> +
> + /* Trigger TCP handshake which causes TCP_NEW_SYN_RECV state where
> + * is_fullsock == 0 and is_locked_tcp_sock == 0.
> + */
> + client_fd = connect_to_fd(server_fd, 0);
> + if (!ASSERT_GE(client_fd, 0, "connect_to_fd"))
> + goto close_server;
> +
> + close(client_fd);
> +
> +close_server:
> + close(server_fd);
> +detach:
> + bpf_prog_detach(cgroup_fd, BPF_CGROUP_SOCK_OPS);
> +}
> +
> +void test_sock_ops_get_sk(void)
Add a "ns" in the naming so that test_progs will start a new netns.
Like 'void test_ns_sock_ops_get_sk(void)'.
> +{
> + struct sock_ops_get_sk *skel;
> + int cgroup_fd;
> +
> + cgroup_fd = test__join_cgroup("/sock_ops_get_sk");
> + if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup"))
nit. Use ASSERT_OK_FD check. Same for the other fd checks in this file.
^ permalink raw reply
* Re: [PATCH bpf v2 1/2] bpf: Fix same-register dst/src OOB read and pointer leak in sock_ops
From: Martin KaFai Lau @ 2026-04-06 23:03 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bpf, Quan Sun, Yinhao Hu, Kaiyan Mei, Dongliang Mu,
Emil Tsalapatis, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Eduard Zingerman, Kumar Kartikeya Dwivedi,
Song Liu, Yonghong Song, Jiri Olsa, John Fastabend,
Stanislav Fomichev, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Shuah Khan, linux-kernel, netdev,
linux-kselftest
In-Reply-To: <20260406031330.187630-1-jiayuan.chen@linux.dev>
On Mon, Apr 06, 2026 at 11:12:50AM +0800, Jiayuan Chen wrote:
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 78b548158fb05..53ce06ed4a88e 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -10581,10 +10581,11 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
> si->dst_reg, si->dst_reg, \
> offsetof(OBJ, OBJ_FIELD)); \
> if (si->dst_reg == si->src_reg) { \
> - *insn++ = BPF_JMP_A(1); \
> + *insn++ = BPF_JMP_A(2); \
> *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg, \
> offsetof(struct bpf_sock_ops_kern, \
> temp)); \
> + *insn++ = BPF_MOV64_IMM(si->dst_reg, 0); \
> } \
> } while (0)
>
> @@ -10618,10 +10619,11 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
> si->dst_reg, si->src_reg, \
> offsetof(struct bpf_sock_ops_kern, sk));\
> if (si->dst_reg == si->src_reg) { \
> - *insn++ = BPF_JMP_A(1); \
> + *insn++ = BPF_JMP_A(2); \
> *insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg, \
> offsetof(struct bpf_sock_ops_kern, \
> temp)); \
> + *insn++ = BPF_MOV64_IMM(si->dst_reg, 0); \
Acked-by: Martin KaFai Lau <martin.lau@kernel.org>
^ permalink raw reply
* Re: [PATCH bpf v2 1/2] bpf: Fix same-register dst/src OOB read and pointer leak in sock_ops
From: Martin KaFai Lau @ 2026-04-06 23:09 UTC (permalink / raw)
To: Jiayuan Chen
Cc: bot+bpf-ci, bpf, 2022090917019, dddddd, M202472210, dzm91, emil,
ast, daniel, andrii, eddyz87, memxor, song, yonghong.song, jolsa,
john.fastabend, sdf, davem, edumazet, kuba, pabeni, horms, shuah,
linux-kernel, netdev, linux-kselftest, martin.lau, clm,
ihor.solodrai
In-Reply-To: <739cdd1f-1838-457d-9b71-f47c3eadb0fb@linux.dev>
On Mon, Apr 06, 2026 at 12:54:04PM +0800, Jiayuan Chen wrote:
>
> On 4/6/26 11:47 AM, bot+bpf-ci@kernel.org wrote:
> > This Fixes: tag covers the SOCK_OPS_GET_SK() portion of the fix,
> > since 84f44df664e9 introduced that macro. However, the identical
> > bug in SOCK_OPS_GET_FIELD() was introduced by an earlier commit:
> >
> > fd09af010788 ("bpf: sock_ops ctx access may stomp registers in corner case")
> >
> > That commit added the dst_reg == src_reg handling to
> > SOCK_OPS_GET_FIELD() with the same missing zeroing in the
> > !fullsock path. Should this also carry:
> >
> > Fixes: fd09af010788 ("bpf: sock_ops ctx access may stomp registers in corner case")
> >
> > Without it, kernels that have fd09af010788 but not 84f44df664e9
> > would not receive the SOCK_OPS_GET_FIELD() fix via stable backport.
> >
> >
> > ---
> > AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> > See:https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> >
> > CI run summary:https://github.com/kernel-patches/bpf/actions/runs/24017481706
> Well, it's true. However, fd09af010788 and 84f44df664e9 are from the
> same patchset (same author, same minute), so any stable branch carrying
> one will have both. That's why I only included a single Fixes tag.
>
> But if you prefer carrying both explicitly, I'm happy to add it in next
> version.
Please list both in the Fixes tags so that it is clear both bugs will
be fixed. Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox