netdev - Re: [PATCH iwl-net v1] igc: Fix infinite initialization loop with early XDP redirect

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: <877cp2rhz5.fsf@intel.com>
Date: Wed, 06 Sep 2023 16:37:34 -0700
From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, sasha.neftin@...el.com, Ferenc Fejes
 <ferenc.fejes@...csson.com>, Jesse Brandeburg
 <jesse.brandeburg@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Jithu Joseph <jithu.joseph@...el.com>, Vedang Patel
 <vedang.patel@...el.com>, Andre Guedes <andre.guedes@...el.com>,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH iwl-net v1] igc: Fix infinite initialization loop with
 early XDP redirect

Maciej Fijalkowski <maciej.fijalkowski@...el.com> writes:

> On Tue, Sep 05, 2023 at 02:37:52PM -0700, Vinicius Costa Gomes wrote:
>> When a XDP redirect happens before the link is ready, that
>
> When exactly link was 'ready' in your setup? You said it was enough to
> launch traffic towards igc iface before running xdp-bench. Was the iface
> down or up or?

In short, the interface was up and it was brought down "externally".

I should have explained my test better: A is the system under test, B is
the monitor; 1. initially the link between systems A and B is up; 2. I
setup the vlans and xdp-bench; 3. I start sending traffic; 4. on system
B, I brind the NIC connected to A down; 5. infinite initialization loop.

>
>> transmission will not finish and will timeout, causing an adapter
>> reset. If the redirects do not stop, the adapter will not stop
>> resetting.
>
> Please highlight that this driver shares tx resources with netstack. I
> believe the source of this bug is that the watchdog is responsible to call
> netif_carrier_on() from a workqueue which happens to be scheduled *after*
> clearing __IGC_DOWN in igc_up().
>

Sure, will add this information to the commit message and send a v2.

>> 
>> Wait for the driver to signal that there's a carrier before allowing
>> transmissions to proceed.
>> 
>> Fixes: 4ff320361092 ("igc: Add support for XDP_REDIRECT action")
>> Reported-by: Ferenc Fejes <ferenc.fejes@...csson.com>
>> Closes: https://lore.kernel.org/netdev/0caf33cf6adb3a5bf137eeaa20e89b167c9986d5.camel@ericsson.com/
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@...el.com>
>> Tested-by: Ferenc Fejes <ferenc.fejes@...csson.com>
>> ---
>>  drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 293b45717683..98de34d0ce07 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -6491,7 +6491,7 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames,
>>  	struct igc_ring *ring;
>>  	int i, drops;
>>  
>> -	if (unlikely(test_bit(__IGC_DOWN, &adapter->state)))
>> +	if (unlikely(!netif_carrier_ok(dev)))
>>  		return -ENETDOWN;
>
> I thought about keeping the bit check as well but given what i wrote above
> it is probably redundant, so:
>
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
>
>>  
>>  	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
>> -- 
>> 2.41.0
>> 

Cheers,
-- 
Vinicius