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

On Fri, May 30, 2025 at 11:16:48AM +0800, Jinjian Song wrote:
> When driver handles the napi rx polling requests, the netdev might
> have been released by the dellink logic triggered by the disconnect
> operation on user plane. However, in the logic of processing skb in
> polling, an invalid netdev is still being used, which causes a panic.
> 
> BUG: kernel NULL pointer dereference, address: 00000000000000f1
> Oops: 0000 [#1] PREEMPT SMP NOPTI
> RIP: 0010:dev_gro_receive+0x3a/0x620
> [...]
> Call Trace:
>  <IRQ>
>  ? __die_body+0x68/0xb0
>  ? page_fault_oops+0x379/0x3e0
>  ? exc_page_fault+0x4f/0xa0
>  ? asm_exc_page_fault+0x22/0x30
>  ? __pfx_t7xx_ccmni_recv_skb+0x10/0x10 [mtk_t7xx (HASH:1400 7)]
>  ? dev_gro_receive+0x3a/0x620
>  napi_gro_receive+0xad/0x170
>  t7xx_ccmni_recv_skb+0x48/0x70 [mtk_t7xx (HASH:1400 7)]
>  t7xx_dpmaif_napi_rx_poll+0x590/0x800 [mtk_t7xx (HASH:1400 7)]
>  net_rx_action+0x103/0x470
>  irq_exit_rcu+0x13a/0x310
>  sysvec_apic_timer_interrupt+0x56/0x90
>  </IRQ>
> 
> 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.

> ---
>  drivers/net/wwan/t7xx/t7xx_netdev.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wwan/t7xx/t7xx_netdev.c b/drivers/net/wwan/t7xx/t7xx_netdev.c
> index 91fa082e9cab..fc0a7cb181df 100644
> --- a/drivers/net/wwan/t7xx/t7xx_netdev.c
> +++ b/drivers/net/wwan/t7xx/t7xx_netdev.c
> @@ -302,7 +302,7 @@ static int t7xx_ccmni_wwan_newlink(void *ctxt, struct net_device *dev, u32 if_id
>  	ccmni->ctlb = ctlb;
>  	ccmni->dev = dev;
>  	atomic_set(&ccmni->usage, 0);
> -	ctlb->ccmni_inst[if_id] = ccmni;
> +	WRITE_ONCE(ctlb->ccmni_inst[if_id], ccmni);
>  
>  	ret = register_netdevice(dev);
>  	if (ret)
> @@ -324,6 +324,7 @@ static void t7xx_ccmni_wwan_dellink(void *ctxt, struct net_device *dev, struct l
>  	if (WARN_ON(ctlb->ccmni_inst[if_id] != ccmni))
>  		return;
>  
> +	WRITE_ONCE(ctlb->ccmni_inst[if_id], NULL);
>  	unregister_netdevice(dev);
>  }
>  
> @@ -419,7 +420,7 @@ static void t7xx_ccmni_recv_skb(struct t7xx_ccmni_ctrl *ccmni_ctlb, struct sk_bu
>  
>  	skb_cb = T7XX_SKB_CB(skb);
>  	netif_id = skb_cb->netif_idx;
> -	ccmni = ccmni_ctlb->ccmni_inst[netif_id];
> +	ccmni = READ_ONCE(ccmni_ctlb->ccmni_inst[netif_id]);
>  	if (!ccmni) {
>  		dev_kfree_skb(skb);
>  		return;
> @@ -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.

>  	if (netif_running(ccmni->dev) && atomic_read(&ccmni->usage) > 0) {
> @@ -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.

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