netdev - Re: [PATCH iwl-net v2 3/6] ice: check for XDP rings instead of bpf program when unconfiguring

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: <ZromrjCGGWn5FcbU@lzaremba-mobl.ger.corp.intel.com>
Date: Mon, 12 Aug 2024 17:13:50 +0200
From: Larysa Zaremba <larysa.zaremba@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, "David S. Miller" <davem@...emloft.net>, "Jacob
 Keller" <jacob.e.keller@...el.com>, Eric Dumazet <edumazet@...gle.com>,
	"Jakub Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "Alexei
 Starovoitov" <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
	"Jesper Dangaard Brouer" <hawk@...nel.org>, John Fastabend
	<john.fastabend@...il.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <bpf@...r.kernel.org>,
	<magnus.karlsson@...el.com>, Michal Kubiak <michal.kubiak@...el.com>,
	Wojciech Drewek <wojciech.drewek@...el.com>, Amritha Nambiar
	<amritha.nambiar@...el.com>
Subject: Re: [PATCH iwl-net v2 3/6] ice: check for XDP rings instead of bpf
 program when unconfiguring

On Mon, Aug 12, 2024 at 02:58:00PM +0200, Maciej Fijalkowski wrote:
> On Wed, Jul 24, 2024 at 06:48:34PM +0200, Larysa Zaremba wrote:
> > If VSI rebuild is pending, .ndo_bpf() can attach/detach the XDP program on
> > VSI without applying new ring configuration. When unconfiguring the VSI, we
> > can encounter the state in which there is an XDP program but no XDP rings
> > to destroy or there will be XDP rings that need to be destroyed, but no XDP
> > program to indicate their presence.
> > 
> > When unconfiguring, rely on the presence of XDP rings rather then XDP
> > program, as they better represent the current state that has to be
> > destroyed.
> > 
> 
> No Fixes: tag?
>

This is basically a fixup for a previous patch. The need to replace program 
check with a ring check emerges from the existence of an in-between state that 
my synchronization introduces. So it does not fix anything from a referencable 
patch. I have separated this because it was seriously getting in the way when 
trying to read the synchronization logic.

> > Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_lib.c  | 4 ++--
> >  drivers/net/ethernet/intel/ice/ice_main.c | 4 ++--
> >  drivers/net/ethernet/intel/ice/ice_xsk.c  | 6 +++---
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> [...]
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > index 2c1a843ba200..5dd50a2866cc 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > @@ -39,7 +39,7 @@ static void ice_qp_reset_stats(struct ice_vsi *vsi, u16 q_idx)
> >  	       sizeof(vsi_stat->rx_ring_stats[q_idx]->rx_stats));
> >  	memset(&vsi_stat->tx_ring_stats[q_idx]->stats, 0,
> >  	       sizeof(vsi_stat->tx_ring_stats[q_idx]->stats));
> > -	if (ice_is_xdp_ena_vsi(vsi))
> > +	if (vsi->xdp_rings)
> >  		memset(&vsi->xdp_rings[q_idx]->ring_stats->stats, 0,
> >  		       sizeof(vsi->xdp_rings[q_idx]->ring_stats->stats));
> >  }
> > @@ -52,7 +52,7 @@ static void ice_qp_reset_stats(struct ice_vsi *vsi, u16 q_idx)
> >  static void ice_qp_clean_rings(struct ice_vsi *vsi, u16 q_idx)
> >  {
> >  	ice_clean_tx_ring(vsi->tx_rings[q_idx]);
> > -	if (ice_is_xdp_ena_vsi(vsi)) {
> > +	if (vsi->xdp_rings) {
> >  		synchronize_rcu();
> >  		ice_clean_tx_ring(vsi->xdp_rings[q_idx]);
> >  	}
> > @@ -189,7 +189,7 @@ static int ice_qp_dis(struct ice_vsi *vsi, u16 q_idx)
> >  	err = ice_vsi_stop_tx_ring(vsi, ICE_NO_RESET, 0, tx_ring, &txq_meta);
> >  	if (err)
> >  		return err;
> > -	if (ice_is_xdp_ena_vsi(vsi)) {
> > +	if (vsi->xdp_rings) {
> 
> From XSK POV these checks are false positive and I will be sending a patch
> that gets rid of it (I had this on my tree when working on timeout issues
> but I pulled this out as it was not a -net candidate IMHO).
> 
> Just a heads up, this can go as-is currently.
>

Ok, thanks.

> >  		struct ice_tx_ring *xdp_ring = vsi->xdp_rings[q_idx];
> >  
> >  		memset(&txq_meta, 0, sizeof(txq_meta));
> > -- 
> > 2.43.0
> >