2023-06-28 09:34:57

by Florian Kauer

[permalink] [raw]
Subject: [PATCH net v2] igc: Prevent garbled TX queue with XDP ZEROCOPY

In normal operation, each populated queue item has
next_to_watch pointing to the last TX desc of the packet,
while each cleaned item has it set to 0. In particular,
next_to_use that points to the next (necessarily clean)
item to use has next_to_watch set to 0.

When the TX queue is used both by an application using
AF_XDP with ZEROCOPY as well as a second non-XDP application
generating high traffic, the queue pointers can get in
an invalid state where next_to_use points to an item
where next_to_watch is NOT set to 0.

However, the implementation assumes at several places
that this is never the case, so if it does hold,
bad things happen. In particular, within the loop inside
of igc_clean_tx_irq(), next_to_clean can overtake next_to_use.
Finally, this prevents any further transmission via
this queue and it never gets unblocked or signaled.
Secondly, if the queue is in this garbled state,
the inner loop of igc_clean_tx_ring() will never terminate,
completely hogging a CPU core.

The reason is that igc_xdp_xmit_zc() reads next_to_use
before acquiring the lock, and writing it back
(potentially unmodified) later. If it got modified
before locking, the outdated next_to_use is written
pointing to an item that was already used elsewhere
(and thus next_to_watch got written).

Fixes: 9acf59a752d4 ("igc: Enable TX via AF_XDP zero-copy")
Signed-off-by: Florian Kauer <[email protected]>
Reviewed-by: Kurt Kanzenbach <[email protected]>
Tested-by: Kurt Kanzenbach <[email protected]>
---

v1 -> v2:
I added some more context for further clarification,
but it is also just how I interpret the code.
Also the typo is fixed and it is reverse christmas again ;-)

---
drivers/net/ethernet/intel/igc/igc_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index eb4f0e562f60..1cb1df613d27 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2791,15 +2791,15 @@ static void igc_xdp_xmit_zc(struct igc_ring *ring)
struct netdev_queue *nq = txring_txq(ring);
union igc_adv_tx_desc *tx_desc = NULL;
int cpu = smp_processor_id();
- u16 ntu = ring->next_to_use;
struct xdp_desc xdp_desc;
- u16 budget;
+ u16 budget, ntu;

if (!netif_carrier_ok(ring->netdev))
return;

__netif_tx_lock(nq, cpu);

+ ntu = ring->next_to_use;
budget = igc_desc_unused(ring);

while (xsk_tx_peek_desc(pool, &xdp_desc) && budget--) {
--
2.39.2



2023-06-28 21:58:29

by Vinicius Costa Gomes

[permalink] [raw]
Subject: Re: [Intel-wired-lan] [PATCH net v2] igc: Prevent garbled TX queue with XDP ZEROCOPY

Florian Kauer <[email protected]> writes:

> In normal operation, each populated queue item has
> next_to_watch pointing to the last TX desc of the packet,
> while each cleaned item has it set to 0. In particular,
> next_to_use that points to the next (necessarily clean)
> item to use has next_to_watch set to 0.
>
> When the TX queue is used both by an application using
> AF_XDP with ZEROCOPY as well as a second non-XDP application
> generating high traffic, the queue pointers can get in
> an invalid state where next_to_use points to an item
> where next_to_watch is NOT set to 0.
>
> However, the implementation assumes at several places
> that this is never the case, so if it does hold,
> bad things happen. In particular, within the loop inside
> of igc_clean_tx_irq(), next_to_clean can overtake next_to_use.
> Finally, this prevents any further transmission via
> this queue and it never gets unblocked or signaled.
> Secondly, if the queue is in this garbled state,
> the inner loop of igc_clean_tx_ring() will never terminate,
> completely hogging a CPU core.
>
> The reason is that igc_xdp_xmit_zc() reads next_to_use
> before acquiring the lock, and writing it back
> (potentially unmodified) later. If it got modified
> before locking, the outdated next_to_use is written
> pointing to an item that was already used elsewhere
> (and thus next_to_watch got written).
>
> Fixes: 9acf59a752d4 ("igc: Enable TX via AF_XDP zero-copy")
> Signed-off-by: Florian Kauer <[email protected]>
> Reviewed-by: Kurt Kanzenbach <[email protected]>
> Tested-by: Kurt Kanzenbach <[email protected]>
> ---

This patch doesn't directly apply because there's a small conflict with
commit 95b681485563 ("igc: Avoid transmit queue timeout for XDP"),
but really easy to solve.

Anyway, good catch:

Acked-by: Vinicius Costa Gomes <[email protected]>


Cheers,
--
Vinicius

2023-06-29 07:38:26

by Florian Kauer

[permalink] [raw]
Subject: Re: [Intel-wired-lan] [PATCH net v2] igc: Prevent garbled TX queue with XDP ZEROCOPY

Hi Vinicius,

On 28.06.23 23:34, Vinicius Costa Gomes wrote:
> Florian Kauer <[email protected]> writes:
>
>> In normal operation, each populated queue item has
>> next_to_watch pointing to the last TX desc of the packet,
>> while each cleaned item has it set to 0. In particular,
>> next_to_use that points to the next (necessarily clean)
>> item to use has next_to_watch set to 0.
>>
>> When the TX queue is used both by an application using
>> AF_XDP with ZEROCOPY as well as a second non-XDP application
>> generating high traffic, the queue pointers can get in
>> an invalid state where next_to_use points to an item
>> where next_to_watch is NOT set to 0.
>>
>> However, the implementation assumes at several places
>> that this is never the case, so if it does hold,
>> bad things happen. In particular, within the loop inside
>> of igc_clean_tx_irq(), next_to_clean can overtake next_to_use.
>> Finally, this prevents any further transmission via
>> this queue and it never gets unblocked or signaled.
>> Secondly, if the queue is in this garbled state,
>> the inner loop of igc_clean_tx_ring() will never terminate,
>> completely hogging a CPU core.
>>
>> The reason is that igc_xdp_xmit_zc() reads next_to_use
>> before acquiring the lock, and writing it back
>> (potentially unmodified) later. If it got modified
>> before locking, the outdated next_to_use is written
>> pointing to an item that was already used elsewhere
>> (and thus next_to_watch got written).
>>
>> Fixes: 9acf59a752d4 ("igc: Enable TX via AF_XDP zero-copy")
>> Signed-off-by: Florian Kauer <[email protected]>
>> Reviewed-by: Kurt Kanzenbach <[email protected]>
>> Tested-by: Kurt Kanzenbach <[email protected]>
>> ---
>
> This patch doesn't directly apply because there's a small conflict with
> commit 95b681485563 ("igc: Avoid transmit queue timeout for XDP"),
> but really easy to solve.
>
> Anyway, good catch:
>
> Acked-by: Vinicius Costa Gomes <[email protected]>

I am sorry, that was bad timing. I prepared the initial patch on Friday and overlooked the merge.
Shall I send a v3 or will someone else take care of the conflict resolution?

Greetings,
Florian

2023-06-29 16:44:00

by Tony Nguyen

[permalink] [raw]
Subject: Re: [Intel-wired-lan] [PATCH net v2] igc: Prevent garbled TX queue with XDP ZEROCOPY

On 6/29/2023 12:07 AM, Florian Kauer wrote:
...

>> This patch doesn't directly apply because there's a small conflict with
>> commit 95b681485563 ("igc: Avoid transmit queue timeout for XDP"),
>> but really easy to solve.
>>
>> Anyway, good catch:
>>
>> Acked-by: Vinicius Costa Gomes <[email protected]>
>
> I am sorry, that was bad timing. I prepared the initial patch on Friday and overlooked the merge.
> Shall I send a v3 or will someone else take care of the conflict resolution?

A v3 would be great.

Thanks,
Tony

2023-06-29 17:01:18

by Vinicius Costa Gomes

[permalink] [raw]
Subject: Re: [Intel-wired-lan] [PATCH net v2] igc: Prevent garbled TX queue with XDP ZEROCOPY

Florian Kauer <[email protected]> writes:

> Hi Vinicius,
>
> On 28.06.23 23:34, Vinicius Costa Gomes wrote:
>> Florian Kauer <[email protected]> writes:
>>
>>> In normal operation, each populated queue item has
>>> next_to_watch pointing to the last TX desc of the packet,
>>> while each cleaned item has it set to 0. In particular,
>>> next_to_use that points to the next (necessarily clean)
>>> item to use has next_to_watch set to 0.
>>>
>>> When the TX queue is used both by an application using
>>> AF_XDP with ZEROCOPY as well as a second non-XDP application
>>> generating high traffic, the queue pointers can get in
>>> an invalid state where next_to_use points to an item
>>> where next_to_watch is NOT set to 0.
>>>
>>> However, the implementation assumes at several places
>>> that this is never the case, so if it does hold,
>>> bad things happen. In particular, within the loop inside
>>> of igc_clean_tx_irq(), next_to_clean can overtake next_to_use.
>>> Finally, this prevents any further transmission via
>>> this queue and it never gets unblocked or signaled.
>>> Secondly, if the queue is in this garbled state,
>>> the inner loop of igc_clean_tx_ring() will never terminate,
>>> completely hogging a CPU core.
>>>
>>> The reason is that igc_xdp_xmit_zc() reads next_to_use
>>> before acquiring the lock, and writing it back
>>> (potentially unmodified) later. If it got modified
>>> before locking, the outdated next_to_use is written
>>> pointing to an item that was already used elsewhere
>>> (and thus next_to_watch got written).
>>>
>>> Fixes: 9acf59a752d4 ("igc: Enable TX via AF_XDP zero-copy")
>>> Signed-off-by: Florian Kauer <[email protected]>
>>> Reviewed-by: Kurt Kanzenbach <[email protected]>
>>> Tested-by: Kurt Kanzenbach <[email protected]>
>>> ---
>>
>> This patch doesn't directly apply because there's a small conflict with
>> commit 95b681485563 ("igc: Avoid transmit queue timeout for XDP"),
>> but really easy to solve.
>>
>> Anyway, good catch:
>>
>> Acked-by: Vinicius Costa Gomes <[email protected]>
>
> I am sorry, that was bad timing. I prepared the initial patch on Friday and overlooked the merge.
> Shall I send a v3 or will someone else take care of the conflict
> resolution?

I think it's easier if you send a v3.


Cheers,
--
Vinicius