netdev - Re: [Intel-wired-lan] [PATCH v1 iwl-net] igb: fix link test skipping when interface is admin down

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250816041557.71794-2-enjuk@amazon.com>
Date: Sat, 16 Aug 2025 13:15:28 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <pmenzel@...gen.mpg.de>
CC: <alexanderduyck@...com>, <andrew+netdev@...n.ch>,
	<anthony.l.nguyen@...el.com>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<enjuk@...zon.com>, <intel-wired-lan@...ts.osuosl.org>,
	<kohei.enju@...il.com>, <kuba@...nel.org>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, <przemyslaw.kitszel@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v1 iwl-net] igb: fix link test skipping
 when interface is admin down

On Fri, 15 Aug 2025 09:41:28 +0200, Paul Menzel wrote:

>[Correct Alexander’s email address]

Thank you for the correction.
I'll use the correct address going forward.

>
>Am 15.08.25 um 09:38 schrieb Paul Menzel:
>> Dear Kohei,
>> 
>> 
>> Thank you very much for your patch.
>> 
>> Am 15.08.25 um 08:26 schrieb Kohei Enju:
>>> The igb driver incorrectly skips the link test when the network
>>> interface is admin down (if_running == false), causing the test to
>>> always report PASS regardless of the actual physical link state.
>>>
>>> This behavior is inconsistent with other drivers (e.g. i40e, ice, ixgbe,
>>> etc.) which correctly test the physical link state regardless of admin
>>> state.
>> 
>> I’d collapse the above two sentences into one paragraph and add an empty 
>> line here to visually separate the paragraph below.

Yes, that makes sense. I'll do that.

>> 
>>> Remove the if_running check to ensure link test always reflects the
>>> physical link state.
>> 
>> Please add how to verify your change, that means the command to run.

Sure, I tested the change using the ip and ethtool commands, with the
physical link down. 
Should I include the following test steps in the commit message?

Before:
  $ sudo ip link set dev enp7s0f0 down
  
  $ ip --json link show enp7s0f0 | jq -c ".[].flags"
  ["BROADCAST","MULTICAST"]
  
  $ sudo ethtool --test enp7s0f0 online
  The test result is PASS
  The test extra info:
  Register test  (offline)         0
  Eeprom test    (offline)         0
  Interrupt test (offline)         0
  Loopback test  (offline)         0
  Link test   (on/offline)         0 # <- Not expected

After:
  $ sudo ip link set dev enp7s0f0 down
  
  $ ip --json link show enp7s0f0 | jq -c ".[].flags"
  ["BROADCAST","MULTICAST"]
  
  $ sudo ethtool --test enp7s0f0 online
  The test result is FAIL
  The test extra info:
  Register test  (offline)         0
  Eeprom test    (offline)         0
  Interrupt test (offline)         0
  Loopback test  (offline)         0
  Link test   (on/offline)         1 # <- Expected

>> 
>>> Fixes: 8d420a1b3ea6 ("igb: correct link test not being run when link is down")
>>> Signed-off-by: Kohei Enju <enjuk@...zon.com>
>>> ---
>>>   drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 +----
>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/ 
>>> net/ethernet/intel/igb/igb_ethtool.c
>>> index ca6ccbc13954..6412c84e2d17 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>> @@ -2081,11 +2081,8 @@ static void igb_diag_test(struct net_device 
>>> *netdev,
>>>       } else {
>>>           dev_info(&adapter->pdev->dev, "online testing starting\n");
>>> -        /* PHY is powered down when interface is down */
>>> -        if (if_running && igb_link_test(adapter, &data[TEST_LINK]))
>>> +        if (igb_link_test(adapter, &data[TEST_LINK]))
>>>               eth_test->flags |= ETH_TEST_FL_FAILED;
>>> -        else
>>> -            data[TEST_LINK] = 0;
>>>           /* Online tests aren't run; pass by default */
>>>           data[TEST_REG] = 0;
>> 
>> Regardless of my style comments above:
>> 
>> Reviewed-by: Paul Menzel <pmenzel@...gen.mpg.de>
>> 
>> 
>> Kind regards,
>> 
>> Paul

Thank you for the review, Paul.