| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bebd0334-4d4b-4162-a935-1a2125400cea@intel.com>
Date: Fri, 29 Aug 2025 13:55:43 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Michal Schmidt <mschmidt@...hat.com>
CC: Intel Wired LAN <intel-wired-lan@...ts.osuosl.org>, Anthony Nguyen
<anthony.l.nguyen@...el.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-net v3] i40e: fix Jumbo Frame support after iPXE boot
On 8/29/2025 6:50 AM, Michal Schmidt wrote:
> On Wed, Aug 27, 2025 at 11:18 PM Jacob Keller <jacob.e.keller@...el.com> wrote:
>> The i40e hardware has multiple hardware settings which define the Maximum
>> Frame Size (MFS) of the physical port. The firmware has an AdminQ command
>> (0x0603) to configure the MFS, but the i40e Linux driver never issues this
>> command.
>>
>> In most cases this is no problem, as the NVM default value has the device
>> configured for its maximum value of 9728. Unfortunately, recent versions of
>> the iPXE intelxl driver now issue the 0x0603 Set Mac Config command,
>> modifying the MFS and reducing it from its default value of 9728.
>>
>> This occurred as part of iPXE commit 6871a7de705b ("[intelxl] Use admin
>> queue to set port MAC address and maximum frame size"), a prerequisite
>> change for supporting the E800 series hardware in iPXE. Both the E700 and
>> E800 firmware support the AdminQ command, and the iPXE code shares much of
>> the logic between the two device drivers.
>>
>> The ice E800 Linux driver already issues the 0x0603 Set Mac Config command
>> early during probe, and is thus unaffected by the iPXE change.
>>
>> Since commit 3a2c6ced90e1 ("i40e: Add a check to see if MFS is set"), the
>> i40e driver does check the I40E_PRTGL_SAH register, but it only logs a
>> warning message if its value is below the 9728 default. This register also
>> only covers received packets and not transmitted packets. A warning can
>> inform system administrators, but does not correct the issue. No
>> interactions from userspace cause the driver to write to PRTGL_SAH or issue
>> the 0x0603 AdminQ command. Only a GLOBR reset will restore the value to its
>> default value. There is no obvious method to trigger a GLOBR reset from
>> user space.
>>
>> To fix this, introduce the i40e_aq_set_mac_config() function, similar to
>> the one from the ice driver. Call this during early probe to ensure that
>> the device configuration matches driver expectation. Unlike E800, the E700
>> firmware also has a bit to control whether the MAC should append CRC data.
>> It is on by default, but setting a 0 to this bit would disable CRC. The
>> i40e implementation must set this bit to ensure CRC will be appended by the
>> MAC.
>>
>> In addition to the AQ command, instead of just checking the I40E_PRTGL_SAH
>> register, update its value to the 9728 default and write it back. This
>> ensures that the hardware is in the expected state, regardless of whether
>> the iPXE (or any other early boot driver) has modified this state.
>>
>> This is a better user experience, as we now fix the issues with larger MTU
>> instead of merely warning. It also aligns with the way the ice E800 series
>> driver works.
>>
>> A final note: The Fixes tag provided here is not strictly accurate. The
>> issue occurs as a result of an external entity (the iPXE intelxl driver),
>> and this is not a regression specifically caused by the mentioned change.
>> However, I believe the original change to just warn about PRTGL_SAH being
>> too low was an insufficient fix.
>>
>> Fixes: 3a2c6ced90e1 ("i40e: Add a check to see if MFS is set")
>> Link: https://github.com/ipxe/ipxe/commit/6871a7de705b6f6a4046f0d19da9bcd689c3bc8e
>> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>> ---
>> Changes in v3:
>> - Don't disable CRC. Otherwise, Tx traffic will not be accepted
>> appropriately.
>> - Link to v2: https://lore.kernel.org/r/20250815-jk-fix-i40e-ice-pxe-9k-mtu-v2-1-ce857cdc6488@intel.com
>>
>> Changes in v2:
>> - Rewrite commit message with feedback from Paul Menzel.
>> - Add missing initialization of cmd via libie_aq_raw().
>> - Fix the Kdoc comment for i40e_aq_set_mac_config().
>> - Move clarification of the Fixes tag to the commit message.
>> - Link to v1: https://lore.kernel.org/r/20250814-jk-fix-i40e-ice-pxe-9k-mtu-v1-1-c3926287fb78@intel.com
>> ---
>> drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h | 1 +
>> drivers/net/ethernet/intel/i40e/i40e_prototype.h | 2 ++
>> drivers/net/ethernet/intel/i40e/i40e_common.c | 34 +++++++++++++++++++++++
>> drivers/net/ethernet/intel/i40e/i40e_main.c | 17 ++++++++----
>> 4 files changed, 48 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
>> index 76d872b91a38..cc02a85ad42b 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h
>> @@ -1561,6 +1561,7 @@ I40E_CHECK_CMD_LENGTH(i40e_aq_set_phy_config);
>> struct i40e_aq_set_mac_config {
>> __le16 max_frame_size;
>> u8 params;
>> +#define I40E_AQ_SET_MAC_CONFIG_CRC_EN BIT(2)
>> u8 tx_timer_priority; /* bitmap */
>> __le16 tx_timer_value;
>> __le16 fc_refresh_threshold;
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
>> index aef5de53ce3b..26bb7bffe361 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
>> @@ -98,6 +98,8 @@ int i40e_aq_set_mac_loopback(struct i40e_hw *hw,
>> struct i40e_asq_cmd_details *cmd_details);
>> int i40e_aq_set_phy_int_mask(struct i40e_hw *hw, u16 mask,
>> struct i40e_asq_cmd_details *cmd_details);
>> +int i40e_aq_set_mac_config(struct i40e_hw *hw, u16 max_frame_size,
>> + struct i40e_asq_cmd_details *cmd_details);
>> int i40e_aq_clear_pxe_mode(struct i40e_hw *hw,
>> struct i40e_asq_cmd_details *cmd_details);
>> int i40e_aq_set_link_restart_an(struct i40e_hw *hw,
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
>> index 270e7e8cf9cf..59f5c1e810eb 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
>> @@ -1189,6 +1189,40 @@ int i40e_set_fc(struct i40e_hw *hw, u8 *aq_failures,
>> return status;
>> }
>>
>> +/**
>> + * i40e_aq_set_mac_config - Configure MAC settings
>> + * @hw: pointer to the hw struct
>> + * @max_frame_size: Maximum Frame Size to be supported by the port
>> + * @cmd_details: pointer to command details structure or NULL
>> + *
>> + * Set MAC configuration (0x0603). Note that max_frame_size must be greater
>> + * than zero.
>> + *
>> + * Return: 0 on success, or a negative error code on failure.
>> + */
>> +int i40e_aq_set_mac_config(struct i40e_hw *hw, u16 max_frame_size,
>> + struct i40e_asq_cmd_details *cmd_details)
>> +{
>> + struct i40e_aq_set_mac_config *cmd;
>> + struct libie_aq_desc desc;
>> +
>> + cmd = libie_aq_raw(&desc);
>> +
>> + if (max_frame_size == 0)
>> + return -EINVAL;
>> +
>> + i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_set_mac_config);
>> +
>> + cmd->max_frame_size = cpu_to_le16(max_frame_size);
>> + cmd->params = I40E_AQ_SET_MAC_CONFIG_CRC_EN;
>> +
>> +#define I40E_AQ_SET_MAC_CONFIG_FC_DEFAULT_THRESHOLD 0x7FFF
>> + cmd->fc_refresh_threshold =
>> + cpu_to_le16(I40E_AQ_SET_MAC_CONFIG_FC_DEFAULT_THRESHOLD);
>> +
>> + return i40e_asq_send_command(hw, &desc, NULL, 0, cmd_details);
>> +}
>> +
>> /**
>> * i40e_aq_clear_pxe_mode
>> * @hw: pointer to the hw struct
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> index b83f823e4917..4796fdd0b966 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> @@ -16045,13 +16045,18 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> dev_dbg(&pf->pdev->dev, "get supported phy types ret = %pe last_status = %s\n",
>> ERR_PTR(err), libie_aq_str(pf->hw.aq.asq_last_status));
>>
>> - /* make sure the MFS hasn't been set lower than the default */
>> #define MAX_FRAME_SIZE_DEFAULT 0x2600
>> - val = FIELD_GET(I40E_PRTGL_SAH_MFS_MASK,
>> - rd32(&pf->hw, I40E_PRTGL_SAH));
>> - if (val < MAX_FRAME_SIZE_DEFAULT)
>> - dev_warn(&pdev->dev, "MFS for port %x (%d) has been set below the default (%d)\n",
>> - pf->hw.port, val, MAX_FRAME_SIZE_DEFAULT);
>> +
>> + err = i40e_aq_set_mac_config(hw, MAX_FRAME_SIZE_DEFAULT, NULL);
>> + if (err) {
>> + dev_warn(&pdev->dev, "set mac config ret = %pe last_status = %s\n",
>
> Just a nit:
> There are double spaces here after the '=' signs for no good reason.
> It's not just in this message. There are a few more like that in this file.
>
I'd hazard a guess that I copied one of those lines and thats how I
ended up with the extra spaces.
>> + ERR_PTR(err), libie_aq_str(pf->hw.aq.asq_last_status));
>> + }
>> +
>> + /* Make sure the MFS is set to the expected value */
>> + val = rd32(hw, I40E_PRTGL_SAH);
>> + FIELD_MODIFY(I40E_PRTGL_SAH_MFS_MASK, &val, MAX_FRAME_SIZE_DEFAULT);
>> + wr32(hw, I40E_PRTGL_SAH, val);
>>
>> /* Add a filter to drop all Flow control frames from any VSI from being
>> * transmitted. By doing so we stop a malicious VF from sending out
>>
>> ---
>> base-commit: ceb9515524046252c522b16f38881e8837ec0d91
>> change-id: 20250813-jk-fix-i40e-ice-pxe-9k-mtu-2b6d03621cd9
>>
>> Best regards,
>> --
>> Jacob Keller <jacob.e.keller@...el.com>
>
> Reviewed-by: Michal Schmidt <mschmidt@...hat.com>
>
Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)
Powered by blists - more mailing lists