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: <ZqOn+Lgr2DoEae6d@boxer>
Date: Fri, 26 Jul 2024 15:43:20 +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 04:07:00PM -0700, Jakub Kicinski wrote:
> On Thu, 25 Jul 2024 20:31:31 +0200 Maciej Fijalkowski wrote:
> > Does that make any sense now?
> 
> Could be brain fog due to post-netdev.conf covid but no, not really.

Huh, that makes two of us.

> 
> The _ONCE() helpers basically give you the ability to store the pointer
> to a variable on the stack, and that variable won't change behind your
> back. But the only reason to READ_ONCE(ptr->thing) something multiple
> times is to tell KCSAN that "I know what I'm doing", it just silences
> potential warnings :S

I feel like you keep on referring to _ONCE (*) being used multiple times
which might be counter-intuitive whereas I was trying from the beginning
to explain my point that xsk pool from driver POV should get the very same
treatment as xdp prog has currently. So, either mark it as __rcu variable
and use rcu helpers or use _ONCE variants plus some sync.

(*) Ok, if you meant from the very beginning that two READ_ONCE against
pool per single critical section is suspicious then I didn't get that,
sorry. With diff below I would have single READ_ONCE and work on that
variable for rest of the napi. Patch was actually trying to limit xsk_pool
accesses from ring struct by working on stack variable.

Would you be okay with that?

-----8<-----

diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 4c115531beba..5b27aaaa94ee 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -1550,14 +1550,15 @@ int ice_napi_poll(struct napi_struct *napi, int budget)
 		budget_per_ring = budget;
 
 	ice_for_each_rx_ring(rx_ring, q_vector->rx) {
+		struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
 		int cleaned;
 
 		/* A dedicated path for zero-copy allows making a single
 		 * comparison in the irq context instead of many inside the
 		 * ice_clean_rx_irq function and makes the codebase cleaner.
 		 */
-		cleaned = READ_ONCE(rx_ring->xsk_pool) ?
-			  ice_clean_rx_irq_zc(rx_ring, budget_per_ring) :
+		cleaned = rx_ring->xsk_pool ?
+			  ice_clean_rx_irq_zc(rx_ring, xsk_pool, budget_per_ring) :
 			  ice_clean_rx_irq(rx_ring, budget_per_ring);
 		work_done += cleaned;
 		/* if we clean as many as budgeted, we must not be done */
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 492a9e54d58b..dceab7619a64 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -837,13 +837,15 @@ ice_add_xsk_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *first,
 /**
  * ice_clean_rx_irq_zc - consumes packets from the hardware ring
  * @rx_ring: AF_XDP Rx ring
+ * @xsk_pool: AF_XDP pool ptr
  * @budget: NAPI budget
  *
  * Returns number of processed packets on success, remaining budget on failure.
  */
-int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget)
+int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
+			struct xsk_buff_pool *xsk_pool,
+			int budget)
 {
-	struct xsk_buff_pool *xsk_pool = READ_ONCE(rx_ring->xsk_pool);
 	unsigned int total_rx_bytes = 0, total_rx_packets = 0;
 	u32 ntc = rx_ring->next_to_clean;
 	u32 ntu = rx_ring->next_to_use;
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.h b/drivers/net/ethernet/intel/ice/ice_xsk.h
index 4cd2d62a0836..8c3675185699 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.h
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.h
@@ -20,7 +20,9 @@ struct ice_vsi;
 #ifdef CONFIG_XDP_SOCKETS
 int ice_xsk_pool_setup(struct ice_vsi *vsi, struct xsk_buff_pool *pool,
 		       u16 qid);
-int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring, int budget);
+int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
+			struct xsk_buff_pool *xsk_pool,
+			int budget);
 int ice_xsk_wakeup(struct net_device *netdev, u32 queue_id, u32 flags);
 bool ice_alloc_rx_bufs_zc(struct ice_rx_ring *rx_ring,
 			  struct xsk_buff_pool *xsk_pool, u16 count);
@@ -45,6 +47,7 @@ ice_xsk_pool_setup(struct ice_vsi __always_unused *vsi,
 
 static inline int
 ice_clean_rx_irq_zc(struct ice_rx_ring __always_unused *rx_ring,
+		    struct xsk_buff_pool __always_unused *xsk_pool,
 		    int __always_unused budget)
 {
 	return 0;

----->8-----

-- 
2.34.1