netdev - Re: [PATCH net 6/8] ice: improve updating ice_{t, r}x_ring::xsk

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZqKaAz8rNOx/Sz5E@boxer>
Date: Thu, 25 Jul 2024 20:31:31 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
	<pabeni@...hat.com>, <edumazet@...gle.com>, <netdev@...r.kernel.org>,
	<magnus.karlsson@...el.com>, <aleksander.lobakin@...el.com>,
	<ast@...nel.org>, <daniel@...earbox.net>, <hawk@...nel.org>,
	<john.fastabend@...il.com>, <bpf@...r.kernel.org>, Shannon Nelson
	<shannon.nelson@....com>, Chandan Kumar Rout <chandanx.rout@...el.com>
Subject: Re: [PATCH net 6/8] ice: improve updating ice_{t, r}x_ring::xsk_pool

On Thu, Jul 25, 2024 at 06:38:58AM -0700, Jakub Kicinski wrote:
> On Wed, 24 Jul 2024 17:49:12 +0200 Maciej Fijalkowski wrote:
> > > So if we are already in the af_xdp handler, and update patch sets pool
> > > to NULL - the af_xdp handler will be fine with the pool becoming NULL?
> > > I guess it may be fine, it's just quite odd to call the function called
> > > _ONCE() multiple times..  
> > 
> > Update path before NULLing pool will go through rcu grace period, stop
> > napis, disable irqs, etc. Running napi won't be exposed to nulled pool in
> > such case.
> 
> Could you make it clearer what condition the patch is fixing, then?
> What can go wrong without this patch?

Sorry for confusion, but without this patch scenario you brought up
initially *could* happen, under some wild circumstances. When I was
responding yesterday my head was around the code with this particular
patch in place, that's why I said such pool state transistion was not
possible.

Updater does this (prior to this patch):

	(...)
	ring->xsk_pool = ice_get_xp_from_qid(vsi, qid); // set to NULL
	(...)
	ice_qvec_toggle_napi(vsi, q_vector, true);
	ice_qvec_ena_irq(vsi, q_vector);

In theory compiler is allowed to transform the code in a way that xsk_pool
assignment will happen *after* triggering napi. So in ice_napi_poll():

		if (tx_ring->xsk_pool)
			wd = ice_xmit_zc(tx_ring); // call ZC routine
		else if (ice_ring_is_xdp(tx_ring))
			wd = true;
		else
			wd = ice_clean_tx_irq(tx_ring, budget);

You will initiate ZC Tx processing because xsk_pool ptr was still valid
and crash in the middle of its job once it's finally NULLed. To prevent
that:

updater:
	(...)
	WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
	(...)
	ice_qvec_toggle_napi(vsi, q_vector, true);
	ice_qvec_ena_irq(vsi, q_vector);
	/* make sure NAPI sees updated ice_{t,x}_ring::xsk_pool */
	synchronize_net();

reader:
		if (READ_ONCE(tx_ring->xsk_pool))
			wd = ice_xmit_zc(tx_ring);
		else if (ice_ring_is_xdp(tx_ring))
			wd = true;
		else
			wd = ice_clean_tx_irq(tx_ring, budget);

Does that make any sense now?