[PATCH] net/ncsi: fix locking in Get MAC Address handling

[PATCH] net/ncsi: fix locking in Get MAC Address handling

[Paul Fertser]

Obtaining RTNL lock in a response handler is not allowed since it runs
in an atomic softirq context. Postpone setting the MAC address by adding
a dedicated step to the configuration FSM.

Fixes: 790071347a0a ("net/ncsi: change from ndo_set_mac_address to dev_set_mac_address")
Cc: stable@vger.kernel.org
Cc: Potin Lai <potin.lai@quantatw.com>
Link: https://lore.kernel.org/20241129-potin-revert-ncsi-set-mac-addr-v1-1-94ea2cb596af@gmail.com
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 net/ncsi/internal.h    |  2 ++
 net/ncsi/ncsi-manage.c | 16 ++++++++++++++--
 net/ncsi/ncsi-rsp.c    | 19 ++++++-------------
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index ef0f8f73826f..4e0842df5234 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -289,6 +289,7 @@ enum {
 	ncsi_dev_state_config_sp	= 0x0301,
 	ncsi_dev_state_config_cis,
 	ncsi_dev_state_config_oem_gma,
+	ncsi_dev_state_config_apply_mac,
 	ncsi_dev_state_config_clear_vids,
 	ncsi_dev_state_config_svf,
 	ncsi_dev_state_config_ev,
@@ -322,6 +323,7 @@ struct ncsi_dev_priv {
 #define NCSI_DEV_RESHUFFLE	4
 #define NCSI_DEV_RESET		8            /* Reset state of NC          */
 	unsigned int        gma_flag;        /* OEM GMA flag               */
+	struct sockaddr     pending_mac;     /* MAC address received from GMA */
 	spinlock_t          lock;            /* Protect the NCSI device    */
 	unsigned int        package_probe_id;/* Current ID during probe    */
 	unsigned int        package_num;     /* Number of packages         */
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 5cf55bde366d..bf276eaf9330 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1038,7 +1038,7 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 			  : ncsi_dev_state_config_clear_vids;
 		break;
 	case ncsi_dev_state_config_oem_gma:
-		nd->state = ncsi_dev_state_config_clear_vids;
+		nd->state = ncsi_dev_state_config_apply_mac;
 
 		nca.package = np->id;
 		nca.channel = nc->id;
@@ -1050,10 +1050,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 			nca.type = NCSI_PKT_CMD_OEM;
 			ret = ncsi_gma_handler(&nca, nc->version.mf_id);
 		}
-		if (ret < 0)
+		if (ret < 0) {
+			nd->state = ncsi_dev_state_config_clear_vids;
 			schedule_work(&ndp->work);
+		}
 
 		break;
+	case ncsi_dev_state_config_apply_mac:
+		rtnl_lock();
+		ret = dev_set_mac_address(dev, &ndp->pending_mac, NULL);
+		rtnl_unlock();
+		if (ret < 0)
+			netdev_warn(dev, "NCSI: 'Writing MAC address to device failed\n");
+
+		nd->state = ncsi_dev_state_config_clear_vids;
+
+		fallthrough;
 	case ncsi_dev_state_config_clear_vids:
 	case ncsi_dev_state_config_svf:
 	case ncsi_dev_state_config_ev:
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index e28be33bdf2c..14bd66909ca4 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -628,16 +628,14 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
 static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 {
 	struct ncsi_dev_priv *ndp = nr->ndp;
+	struct sockaddr *saddr = &ndp->pending_mac;
 	struct net_device *ndev = ndp->ndev.dev;
 	struct ncsi_rsp_oem_pkt *rsp;
-	struct sockaddr saddr;
 	u32 mac_addr_off = 0;
-	int ret = 0;
 
 	/* Get the response header */
 	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
 
-	saddr.sa_family = ndev->type;
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
 	if (mfr_id == NCSI_OEM_MFR_BCM_ID)
 		mac_addr_off = BCM_MAC_ADDR_OFFSET;
@@ -646,22 +644,17 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 	else if (mfr_id == NCSI_OEM_MFR_INTEL_ID)
 		mac_addr_off = INTEL_MAC_ADDR_OFFSET;
 
-	memcpy(saddr.sa_data, &rsp->data[mac_addr_off], ETH_ALEN);
+	saddr->sa_family = ndev->type;
+	memcpy(saddr->sa_data, &rsp->data[mac_addr_off], ETH_ALEN);
 	if (mfr_id == NCSI_OEM_MFR_BCM_ID || mfr_id == NCSI_OEM_MFR_INTEL_ID)
-		eth_addr_inc((u8 *)saddr.sa_data);
-	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
+		eth_addr_inc((u8 *)saddr->sa_data);
+	if (!is_valid_ether_addr((const u8 *)saddr->sa_data))
 		return -ENXIO;
 
 	/* Set the flag for GMA command which should only be called once */
 	ndp->gma_flag = 1;
 
-	rtnl_lock();
-	ret = dev_set_mac_address(ndev, &saddr, NULL);
-	rtnl_unlock();
-	if (ret < 0)
-		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
-
-	return ret;
+	return 0;
 }
 
 /* Response handler for Mellanox card */
-- 
2.34.1

Re: [PATCH] net/ncsi: fix locking in Get MAC Address handling

[Jakub Kicinski]

On Thu,  9 Jan 2025 17:50:54 +0300 Paul Fertser wrote:
> Obtaining RTNL lock in a response handler is not allowed since it runs
> in an atomic softirq context. Postpone setting the MAC address by adding
> a dedicated step to the configuration FSM.
> 
> Fixes: 790071347a0a ("net/ncsi: change from ndo_set_mac_address to dev_set_mac_address")
> Cc: stable@vger.kernel.org
> Cc: Potin Lai <potin.lai@quantatw.com>

Neat!

Potin, please give this a test ASAP.

Re: 回覆: [External] Re: [PATCH] net/ncsi: fix locking in Get MAC Address handling

[Jakub Kicinski]

On Fri, 10 Jan 2025 06:02:04 +0000 Potin Lai (賴柏廷) wrote:
> > Neat!
> > Potin, please give this a test ASAP.  
> 
> Thanks for the new patch.
> I am currently tied up with other tasks, but I’ll make sure to test
> it as soon as possible and share the results with you.

Understood, would you be able to test it by January 13th?
Depending on how long we need to wait we may be better off
applying the patch already or waiting with committing..

Re: 回覆: [External] Re: [PATCH] net/ncsi: fix locking in Get MAC Address handling

[Potin Lai]

On Sat, Jan 11, 2025 at 10:18 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 10 Jan 2025 06:02:04 +0000 Potin Lai (賴柏廷) wrote:
> > > Neat!
> > > Potin, please give this a test ASAP.
> >
> > Thanks for the new patch.
> > I am currently tied up with other tasks, but I’ll make sure to test
> > it as soon as possible and share the results with you.
>
> Understood, would you be able to test it by January 13th?
> Depending on how long we need to wait we may be better off
> applying the patch already or waiting with committing..

Hi Jakub & Paul,

I had a test yesterday, the patch is working and the kernel panic does
not happen any more, but we notice sometimes the config_apply_mac
state runs before the gma command is handled.

Cosmo helped me to find a potential state handling issue, and I
submitted the v2 version.
Please kindly have a look at v2 version with the link below.
v2: https://lore.kernel.org/all/20250111-fix-ncsi-mac-v2-0-838e0a1a233a@gmail.com/

Best Regards,
Potin

Re: 回覆: [External] Re: [PATCH] net/ncsi: fix locking in Get MAC Address handling

[Jakub Kicinski]

On Sat, 11 Jan 2025 19:12:51 +0800 Potin Lai wrote:
> > > Thanks for the new patch.
> > > I am currently tied up with other tasks, but I’ll make sure to test
> > > it as soon as possible and share the results with you.  
> >
> > Understood, would you be able to test it by January 13th?
> > Depending on how long we need to wait we may be better off
> > applying the patch already or waiting with committing..  
> 
> Hi Jakub & Paul,
> 
> I had a test yesterday, the patch is working and the kernel panic does
> not happen any more, but we notice sometimes the config_apply_mac
> state runs before the gma command is handled.
> 
> Cosmo helped me to find a potential state handling issue, and I
> submitted the v2 version.
> Please kindly have a look at v2 version with the link below.
> v2: https://lore.kernel.org/all/20250111-fix-ncsi-mac-v2-0-838e0a1a233a@gmail.com/

Is there any reason why you reposted Paul's patch?
Patch 2 looks like a fix for a separate issue (but for the same 
use case), am I wrong?

Also one thing you have not done is to provide the Tested-by: tag 
on Paul's patch :)

Re: 回覆: [External] Re: [PATCH] net/ncsi: fix locking in Get MAC Address handling

[Potin Lai]

On Tue, Jan 14, 2025 at 5:19 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sat, 11 Jan 2025 19:12:51 +0800 Potin Lai wrote:
> > > > Thanks for the new patch.
> > > > I am currently tied up with other tasks, but I’ll make sure to test
> > > > it as soon as possible and share the results with you.
> > >
> > > Understood, would you be able to test it by January 13th?
> > > Depending on how long we need to wait we may be better off
> > > applying the patch already or waiting with committing..
> >
> > Hi Jakub & Paul,
> >
> > I had a test yesterday, the patch is working and the kernel panic does
> > not happen any more, but we notice sometimes the config_apply_mac
> > state runs before the gma command is handled.
> >
> > Cosmo helped me to find a potential state handling issue, and I
> > submitted the v2 version.
> > Please kindly have a look at v2 version with the link below.
> > v2: https://lore.kernel.org/all/20250111-fix-ncsi-mac-v2-0-838e0a1a233a@gmail.com/
>
> Is there any reason why you reposted Paul's patch?
> Patch 2 looks like a fix for a separate issue (but for the same
> use case), am I wrong?
Sorry, I thought the second patch needs to be followed by the first patch.
Yes, these 2 patches are fixing different issues, I will remove Paul's
patch in the next version (v3).

>
> Also one thing you have not done is to provide the Tested-by: tag
> on Paul's patch :)

Tested-by: Potin Lai <potin.lai.pt@gmail.com>

Re: 回覆: [External] Re: [PATCH] net/ncsi: fix locking in Get MAC Address handling

[Jakub Kicinski]

On Tue, 14 Jan 2025 09:56:13 +0800 Potin Lai wrote:
> > Also one thing you have not done is to provide the Tested-by: tag
> > on Paul's patch :)  
> 
> Tested-by: Potin Lai <potin.lai.pt@gmail.com>

Thanks!

Re: [PATCH] net/ncsi: fix locking in Get MAC Address handling

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  9 Jan 2025 17:50:54 +0300 you wrote:
> Obtaining RTNL lock in a response handler is not allowed since it runs
> in an atomic softirq context. Postpone setting the MAC address by adding
> a dedicated step to the configuration FSM.
> 
> Fixes: 790071347a0a ("net/ncsi: change from ndo_set_mac_address to dev_set_mac_address")
> Cc: stable@vger.kernel.org
> Cc: Potin Lai <potin.lai@quantatw.com>
> Link: https://lore.kernel.org/20241129-potin-revert-ncsi-set-mac-addr-v1-1-94ea2cb596af@gmail.com
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> 
> [...]

Here is the summary with links:
  - net/ncsi: fix locking in Get MAC Address handling
    https://git.kernel.org/netdev/net/c/9e2bbab94b88

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html