netdev - Re: [net v3] net: wwan: t7xx: Fix napi rx poll issue

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] [day] [month] [year] [list]
Message-ID: <aEA1hBEltWuIE-Yy@soc-5CG4396X81.clients.intel.com>
Date: Wed, 4 Jun 2025 14:01:08 +0200
From: Larysa Zaremba <larysa.zaremba@...el.com>
To: Jinjian Song <jinjian.song@...ocom.com>
CC: <andrew+netdev@...n.ch>, <angelogioacchino.delregno@...labora.com>,
	<chandrashekar.devegowda@...el.com>, <chiranjeevi.rapolu@...ux.intel.com>,
	<corbet@....net>, <danielwinkler@...gle.com>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <haijun.liu@...iatek.com>, <helgaas@...nel.org>,
	<horms@...nel.org>, <ilpo.jarvinen@...ux.intel.com>,
	<johannes@...solutions.net>, <kuba@...nel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-mediatek@...ts.infradead.org>,
	<loic.poulain@...aro.org>, <m.chetan.kumar@...ux.intel.com>,
	<matthias.bgg@...il.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<ricardo.martinez@...ux.intel.com>, <ryazanov.s.a@...il.com>,
	<sreehari.kancharla@...ux.intel.com>
Subject: Re: [net v3] net: wwan: t7xx: Fix napi rx poll issue

On Wed, Jun 04, 2025 at 06:19:53PM +0800, Jinjian Song wrote:
> From: Larysa Zaremba <larysa.zaremba@...el.com>
> 
> >> Fixes: 5545b7b9f294 ("net: wwan: t7xx: Add NAPI support")
> >> Signed-off-by: Jinjian Song <jinjian.song@...ocom.com>
> >> ---
> >> v3:
> >>  * Only Use READ_ONCE/WRITE_ONCE when the lock protecting ctlb->ccmni_inst
> >>    is not held.
> >
> >What do you mean by "lock protecting ctlb->ccmni_inst"? Please specify.
> 
> Hi Larysa,
> 
> This description might have been a bit simplified. This process is as follow:
> 
> In patch v1, I directly set ctlb->ccmni_inst. This may be not safe, as the NAPI
> processing and the driver's internal interface might not be synchronized. Therefoe,
> following Jakub's suggestion, I add READ_ONCE/WRITE_ONCE in all places where this
> pointer is accessed.
> 
> In patch v2, Paolo suggested using READ_ONCE in places that are not protected by locks.
> Some interfaces are protected by synchronization mechanisms, so it's unnecesssary to add them there.
> Therefore, I removed READ_ONCE from the interfaces.
>

I have seen the discussion for previous version, I am asking you for the symbol 
name/names for the locks that make READ_ONCE in the removed places not needed.

> >> @@ -441,7 +442,7 @@ static void t7xx_ccmni_recv_skb(struct t7xx_ccmni_ctrl *ccmni_ctlb, struct sk_bu
> >>  
> >>  static void t7xx_ccmni_queue_tx_irq_notify(struct t7xx_ccmni_ctrl *ctlb, int qno)
> >>  {
> >> -	struct t7xx_ccmni *ccmni = ctlb->ccmni_inst[0];
> >> +	struct t7xx_ccmni *ccmni = READ_ONCE(ctlb->ccmni_inst[0]);
> >>  	struct netdev_queue *net_queue;
> >> 
> >
> >You do not seem to check if ccmni is NULL here, so given ctlb->ccmni_inst[0] is 
> >not being hot-swapped, I guess that there are some guarantees of it not being 
> >NULL at this moment, so I would drop READ_ONCE here.
> 
> This ctlb->ccmni_inst[0] is checked in the upper-level interface:
> static void t7xx_ccmni_queue_state_notify([...]) {
> 	[...]
> 	if (!READ_ONCE(ctlb->ccmni_inst[0])) {
> 		return;
> 	}
> 
> 	if (state == DMPAIF_TXQ_STATE_IRQ)
> 		t7xx_ccmni_queue_tx_irq_notify(ctlb, qno);
> 	else if (state == DMPAIF_TXQ_STATE_FULL)
> 		t7xx_ccmni_queue_tx_full_notify(ctlb, qno);
> }
> 
> Since this is part of the driver's internal logic for handing queue events, would it be
> safer to add READ_ONCE here as well?
>

Well, I am not 100% sure.  What would make the code easier to reason about in 
terms of READ_ONCE/WRITE_ONCE is if you replaced struct t7xx_ccmni_ctrl *ctlb 
argument in t7xx_ccmni_queue_tx_irq_notify() and 
t7xx_ccmni_queue_tx_full_notify() with ctlb->ccmni_inst[0], the code would look 
like this:

	struct t7xx_ccmni *ccmni = 
		READ_ONCE(t7xx_dev->ccmni_ctlb->ccmni_inst[0]);

	if (!ccmni) {
		dev_warn(&t7xx_dev->pdev->dev, "No netdev registered yet\n");
		return;
	}

	if (state == DMPAIF_TXQ_STATE_IRQ)
		t7xx_ccmni_queue_tx_irq_notify(ccmni, qno);
	else if (state == DMPAIF_TXQ_STATE_FULL)
		t7xx_ccmni_queue_tx_full_notify(ccmni, qno);

This way atomic reads in notifiers would be dependent on a single READ_ONCE, 
which should prevent nasty reordering, as far as I am concerned.

The above holds if you think you do not need to check for NULL in the notifiers, 
but is such case I would rather consider proper locking or RCU.

> >> @@ -453,7 +454,7 @@ static void t7xx_ccmni_queue_tx_irq_notify(struct t7xx_ccmni_ctrl *ctlb, int qno
> >>  
> >>  static void t7xx_ccmni_queue_tx_full_notify(struct t7xx_ccmni_ctrl *ctlb, int qno)
> >>  {
> >> -	struct t7xx_ccmni *ccmni = ctlb->ccmni_inst[0];
> >> +	struct t7xx_ccmni *ccmni = READ_ONCE(ctlb->ccmni_inst[0]);
> >>  	struct netdev_queue *net_queue;
> >>
> >
> >Same as above, either READ_ONCE is not needed or NULL check is required.
> 
> Yes, This function in the same upper-level interface.
> 
> >  	if (atomic_read(&ccmni->usage) > 0) {
> > @@ -471,7 +472,7 @@ static void t7xx_ccmni_queue_state_notify(struct t7xx_pci_dev *t7xx_dev,
> >  	if (ctlb->md_sta != MD_STATE_READY)
> >  		return;
> >  
> > -	if (!ctlb->ccmni_inst[0]) {
> > +	if (!READ_ONCE(ctlb->ccmni_inst[0])) {
> >  		dev_warn(&t7xx_dev->pdev->dev, "No netdev registered yet\n");
> >  		return;
> >  	}
> > -- 
> > 2.34.1
> > 
> > 
> 
> Thanks.
> 
> Jinjian,
> Best Regards.