| 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: <17b4725f-b81f-4760-bb90-ac8fa9467227@intel.com>
Date: Wed, 18 Oct 2023 15:03:13 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <anthony.l.nguyen@...el.com>,
<magnus.karlsson@...el.com>, Tushar Vyavahare <tushar.vyavahare@...el.com>
Subject: Re: [PATCH net] i40e: xsk: remove count_mask
On 10/18/2023 9:39 AM, Maciej Fijalkowski wrote:
> Cited commit introduced a neat way of updating next_to_clean that does
> not require boundary checks on each increment. This was done by masking
> the new value with (ring length - 1) mask. Problem is that this is
> applicable only for power of 2 ring sizes, for every other size this
> assumption can not be made. In turn, it leads to cleaning descriptors
> out of order as well as splats:
>
I assume that since ring size isn't a constant it isn't worth trying to
check if power of 2 and then use the shortcut?
What about just enforcing ring size is a power of 2? Any reason not to
do that?
Thanks,
Jake
> [ 1388.411915] Workqueue: events xp_release_deferred
> [ 1388.411919] RIP: 0010:xp_free+0x1a/0x50
> [ 1388.411921] Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 55 48 8b 57 70 48 8d 47 70 48 89 e5 48 39 d0 74 06 <5d> c3 cc cc cc cc 48 8b 57 60 83 82 b8 00 00 00 01 48 8b 57 60 48
> [ 1388.411922] RSP: 0018:ffa0000000a83cb0 EFLAGS: 00000206
> [ 1388.411923] RAX: ff11000119aa5030 RBX: 000000000000001d RCX: ff110001129b6e50
> [ 1388.411924] RDX: ff11000119aa4fa0 RSI: 0000000055555554 RDI: ff11000119aa4fc0
> [ 1388.411925] RBP: ffa0000000a83cb0 R08: 0000000000000000 R09: 0000000000000000
> [ 1388.411926] R10: 0000000000000001 R11: 0000000000000000 R12: ff11000115829b80
> [ 1388.411927] R13: 000000000000005f R14: 0000000000000000 R15: ff11000119aa4fc0
> [ 1388.411928] FS: 0000000000000000(0000) GS:ff11000277e00000(0000) knlGS:0000000000000000
> [ 1388.411929] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1388.411930] CR2: 00007f1f564e6c14 CR3: 000000000783c005 CR4: 0000000000771ef0
> [ 1388.411931] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 1388.411931] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 1388.411932] PKRU: 55555554
> [ 1388.411933] Call Trace:
> [ 1388.411934] <IRQ>
> [ 1388.411935] ? show_regs+0x6e/0x80
> [ 1388.411937] ? watchdog_timer_fn+0x1d2/0x240
> [ 1388.411939] ? __pfx_watchdog_timer_fn+0x10/0x10
> [ 1388.411941] ? __hrtimer_run_queues+0x10e/0x290
> [ 1388.411945] ? clockevents_program_event+0xae/0x130
> [ 1388.411947] ? hrtimer_interrupt+0x105/0x240
> [ 1388.411949] ? __sysvec_apic_timer_interrupt+0x54/0x150
> [ 1388.411952] ? sysvec_apic_timer_interrupt+0x7f/0x90
> [ 1388.411955] </IRQ>
> [ 1388.411955] <TASK>
> [ 1388.411956] ? asm_sysvec_apic_timer_interrupt+0x1f/0x30
> [ 1388.411958] ? xp_free+0x1a/0x50
> [ 1388.411960] i40e_xsk_clean_rx_ring+0x5d/0x100 [i40e]
> [ 1388.411968] i40e_clean_rx_ring+0x14c/0x170 [i40e]
> [ 1388.411977] i40e_queue_pair_disable+0xda/0x260 [i40e]
> [ 1388.411986] i40e_xsk_pool_setup+0x192/0x1d0 [i40e]
> [ 1388.411993] i40e_reconfig_rss_queues+0x1f0/0x1450 [i40e]
> [ 1388.412002] xp_disable_drv_zc+0x73/0xf0
> [ 1388.412004] ? mutex_lock+0x17/0x50
> [ 1388.412007] xp_release_deferred+0x2b/0xc0
> [ 1388.412010] process_one_work+0x178/0x350
> [ 1388.412011] ? __pfx_worker_thread+0x10/0x10
> [ 1388.412012] worker_thread+0x2f7/0x420
> [ 1388.412014] ? __pfx_worker_thread+0x10/0x10
> [ 1388.412015] kthread+0xf8/0x130
> [ 1388.412017] ? __pfx_kthread+0x10/0x10
> [ 1388.412019] ret_from_fork+0x3d/0x60
> [ 1388.412021] ? __pfx_kthread+0x10/0x10
> [ 1388.412023] ret_from_fork_asm+0x1b/0x30
> [ 1388.412026] </TASK>
>
> It comes from picking wrong ring entries when cleaning xsk buffers
> during pool detach.
>
> Remove the count_mask logic and use they boundary check when updating
> next_to_process (which used to be a next_to_clean).
>
> Fixes: c8a8ca3408dc ("i40e: remove unnecessary memory writes of the next to clean pointer")
> Reported-by: Tushar Vyavahare <tushar.vyavahare@...el.com>
> Tested-by: Tushar Vyavahare <tushar.vyavahare@...el.com>
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> ---
> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index 37f41c8a682f..7d991e4d9b89 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -437,12 +437,12 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> u16 next_to_process = rx_ring->next_to_process;
> u16 next_to_clean = rx_ring->next_to_clean;
> - u16 count_mask = rx_ring->count - 1;
> unsigned int xdp_res, xdp_xmit = 0;
> struct xdp_buff *first = NULL;
> + u32 count = rx_ring->count;
> struct bpf_prog *xdp_prog;
> + u32 entries_to_alloc;
> bool failure = false;
> - u16 cleaned_count;
>
> if (next_to_process != next_to_clean)
> first = *i40e_rx_bi(rx_ring, next_to_clean);
> @@ -475,7 +475,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> qword);
> bi = *i40e_rx_bi(rx_ring, next_to_process);
> xsk_buff_free(bi);
> - next_to_process = (next_to_process + 1) & count_mask;
> + if (++next_to_process == count)
> + next_to_process = 0;
> continue;
> }
>
> @@ -493,7 +494,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> else if (i40e_add_xsk_frag(rx_ring, first, bi, size))
> break;
>
> - next_to_process = (next_to_process + 1) & count_mask;
> + if (++next_to_process == count)
> + next_to_process = 0;
>
> if (i40e_is_non_eop(rx_ring, rx_desc))
> continue;
> @@ -513,10 +515,10 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>
> rx_ring->next_to_clean = next_to_clean;
> rx_ring->next_to_process = next_to_process;
> - cleaned_count = (next_to_clean - rx_ring->next_to_use - 1) & count_mask;
>
> - if (cleaned_count >= I40E_RX_BUFFER_WRITE)
> - failure |= !i40e_alloc_rx_buffers_zc(rx_ring, cleaned_count);
> + entries_to_alloc = I40E_DESC_UNUSED(rx_ring);
> + if (entries_to_alloc >= I40E_RX_BUFFER_WRITE)
> + failure |= !i40e_alloc_rx_buffers_zc(rx_ring, entries_to_alloc);
>
> i40e_finalize_xdp_rx(rx_ring, xdp_xmit);
> i40e_update_rx_stats(rx_ring, total_rx_bytes, total_rx_packets);
> @@ -752,14 +754,16 @@ int i40e_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
>
> void i40e_xsk_clean_rx_ring(struct i40e_ring *rx_ring)
> {
> - u16 count_mask = rx_ring->count - 1;
> u16 ntc = rx_ring->next_to_clean;
> u16 ntu = rx_ring->next_to_use;
>
> - for ( ; ntc != ntu; ntc = (ntc + 1) & count_mask) {
> + while (ntc != ntu) {
> struct xdp_buff *rx_bi = *i40e_rx_bi(rx_ring, ntc);
>
> xsk_buff_free(rx_bi);
> + ntc++;
> + if (ntc >= rx_ring->count)
> + ntc = 0;
> }
> }
>
Powered by blists - more mailing lists