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. Most importantly, it can happen that
next_to_use points to an entry where next_to_watch != 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 aquiring 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 entry 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]>
---
drivers/net/ethernet/intel/igc/igc_main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index eb4f0e562f60..2eff073ee771 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2791,7 +2791,7 @@ 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;
+ u16 ntu;
struct xdp_desc xdp_desc;
u16 budget;
@@ -2800,6 +2800,7 @@ static void igc_xdp_xmit_zc(struct igc_ring *ring)
__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
On Mon, Jun 26, 2023 at 01:34:29PM +0200, Florian Kauer wrote:
Hi Florian,
> 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. Most importantly, it can happen that
> next_to_use points to an entry where next_to_watch != 0.
Although what you are fixing is clearly a bug, I don't follow the
paragraph above - what does it mean that next_to_watch is not 0? What is
the purpose of it within igc driver?
Another thing is that even though txq is shared between XSK ZC and normal
app it feels that a lot of unnecessary logic is executed for XSK ZC (when
running igc_clean_tx_irq()) but that's just a side comment to consider
implementing a separate routine for XSK ZC Tx cleaning.
>
> 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 aquiring 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 entry 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]>
> ---
> drivers/net/ethernet/intel/igc/igc_main.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index eb4f0e562f60..2eff073ee771 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -2791,7 +2791,7 @@ 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;
> + u16 ntu;
> struct xdp_desc xdp_desc;
> u16 budget;
You are breaking RCT here.
>
> @@ -2800,6 +2800,7 @@ static void igc_xdp_xmit_zc(struct igc_ring *ring)
>
> __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
>
On Mon, Jun 26, 2023 at 01:34:29PM +0200, Florian Kauer wrote:
> 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. Most importantly, it can happen that
> next_to_use points to an entry where next_to_watch != 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 aquiring the lock, and writing it back
Hi Florian,
a minor nit via checkpatch --codespell: aquiring -> acquiring
> (potentially unmodified) later. If it got modified
> before locking, the outdated next_to_use is written
> pointing to an entry 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]>
...