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

Message-Id: <aD7BsIXPxYtZYBH_@soc-5CG4396X81.clients.intel.com>
Date: Wed,  4 Jun 2025 18:19:53 +0800
From: Jinjian Song <jinjian.song@...ocom.com>
To: larysa.zaremba@...el.com,
	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

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.

>> @@ -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?

>> @@ -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.