[PATCH net v2] Revert "net/ncsi: change from ndo_set_mac_address to dev_set_mac_address"

[PATCH net v2] Revert "net/ncsi: change from ndo_set_mac_address to dev_set_mac_address"

[Jakub Kicinski]

From: Potin Lai <potin.lai@quantatw.com>

This reverts commit 790071347a0a1a89e618eedcd51c687ea783aeb3.

We are seeing kernel panic when enabling two NCSI interfaces at same
time. It looks like mutex lock is being used in softirq caused the
issue.

Kernel panic log:

  8021q: adding VLAN 0 to HW filter on device eth0
  ftgmac100 1e670000.ethernet eth0: NCSI: Handler for packet type 0x82 returned -19
  BUG: scheduling while atomic: systemd-network/697/0x00000100
  Modules linked in:
  8021q: adding VLAN 0 to HW filter on device eth1
  CPU: 0 PID: 697 Comm: systemd-network Tainted: G        W          6.6.62-8ea1fc6-dirty-cbd80d0-gcbd80d04d13c #1
  Hardware name: Generic DT based system
   unwind_backtrace from show_stack+0x18/0x1c
   show_stack from dump_stack_lvl+0x40/0x4c
   dump_stack_lvl from __schedule_bug+0x5c/0x70
   __schedule_bug from __schedule+0x884/0x968
   __schedule from schedule+0x58/0xa8
   schedule from schedule_preempt_disabled+0x14/0x18
   schedule_preempt_disabled from __mutex_lock.constprop.0+0x350/0x76c
   __mutex_lock.constprop.0 from ncsi_rsp_handler_oem_gma+0x104/0x1a0
   ncsi_rsp_handler_oem_gma from ncsi_rcv_rsp+0x120/0x2cc
   ncsi_rcv_rsp from __netif_receive_skb_one_core+0x60/0x84
   __netif_receive_skb_one_core from netif_receive_skb+0x38/0x148
   netif_receive_skb from ftgmac100_poll+0x358/0x444
   ftgmac100_poll from __napi_poll.constprop.0+0x34/0x1d0
   __napi_poll.constprop.0 from net_rx_action+0x350/0x43c
   net_rx_action from handle_softirqs+0x114/0x32c
   handle_softirqs from irq_exit+0x88/0xb8
   irq_exit from call_with_stack+0x18/0x20
   call_with_stack from __irq_usr+0x78/0xa0
  Exception stack(0xe075dfb0 to 0xe075dff8)
  dfa0:                                     00000000 00000000 00000000 00000020
  dfc0: 00000069 aefde3e0 00000000 00000000 00000000 00000000 00000000 aefde4e4
  dfe0: 01010101 aefddf20 a6b4331c a6b43618 600f0010 ffffffff

Fixes: 790071347a0a ("net/ncsi: change from ndo_set_mac_address to dev_set_mac_address")
Signed-off-by: Potin Lai <potin.lai.pt@gmail.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
Looks like we're not making any progress on this one, so let's
go with the revert for 6.13.

Original posting by Potin Lai:
  https://lore.kernel.org/20241129-potin-revert-ncsi-set-mac-addr-v1-1-94ea2cb596af@gmail.com

I added the Fixes tag and trimmed the kernel log.

CC: sam@mendozajonas.com
CC: fr0st61te@gmail.com
CC: fercerpav@gmail.com
---
 net/ncsi/ncsi-rsp.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index e28be33bdf2c..0cd7b916d3f8 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -629,6 +629,7 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 {
 	struct ncsi_dev_priv *ndp = nr->ndp;
 	struct net_device *ndev = ndp->ndev.dev;
+	const struct net_device_ops *ops = ndev->netdev_ops;
 	struct ncsi_rsp_oem_pkt *rsp;
 	struct sockaddr saddr;
 	u32 mac_addr_off = 0;
@@ -655,9 +656,7 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 	/* 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();
+	ret = ops->ndo_set_mac_address(ndev, &saddr);
 	if (ret < 0)
 		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
 
-- 
2.47.1

Re: [PATCH net v2] Revert "net/ncsi: change from ndo_set_mac_address to dev_set_mac_address"

[Paul Fertser]

Hello,

On Wed, Jan 08, 2025 at 11:23:46AM -0800, Jakub Kicinski wrote:
> Looks like we're not making any progress on this one, so let's
> go with the revert for 6.13.

But this does break userspace, the commit was there for a reason.

Potin Lai, have you tried deferring this to a work queue instead of
reverting to the code which has always been wrong?

> Original posting by Potin Lai:
>   https://lore.kernel.org/20241129-potin-revert-ncsi-set-mac-addr-v1-1-94ea2cb596af@gmail.com

Unfortunately I wasn't on CC and missed that. Can we fix this proper
way please, do we have few more days?

If I get it right dev_set_mac_address() just isn't meant to be ever
called from a softirq (and NCSI here is special in getting a MAC
address update from a "network" frame so that happens in
net_rx_action() context). So postponing the actual processing of this
reply looks like the way to go, right?

> -	rtnl_lock();
> -	ret = dev_set_mac_address(ndev, &saddr, NULL);
> -	rtnl_unlock();
> +	ret = ops->ndo_set_mac_address(ndev, &saddr);
>
>  	if (ret < 0)
>  		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");

I expect if this is scheduled on events/x with queue_work() then we'll
get what we need.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: [PATCH net v2] Revert "net/ncsi: change from ndo_set_mac_address to dev_set_mac_address"

[Jakub Kicinski]

On Wed, 8 Jan 2025 23:23:23 +0300 Paul Fertser wrote:
> > Original posting by Potin Lai:
> >   https://lore.kernel.org/20241129-potin-revert-ncsi-set-mac-addr-v1-1-94ea2cb596af@gmail.com  
> 
> Unfortunately I wasn't on CC and missed that. Can we fix this proper
> way please, do we have few more days?

Sure thing, we need it in the tree early next week, so probably on
the list by Friday so that folks have a chance to review.

> If I get it right dev_set_mac_address() just isn't meant to be ever
> called from a softirq (and NCSI here is special in getting a MAC
> address update from a "network" frame so that happens in
> net_rx_action() context). So postponing the actual processing of this
> reply looks like the way to go, right?

I think so. Only trickiness I can think of is the ordering of the work
vs admin setting the address from user space. Do venture a patch,
I don't know enough about NC-SI.. :(

Re: [PATCH net v2] Revert "net/ncsi: change from ndo_set_mac_address to dev_set_mac_address"

[Ivan Mikhaylov]

On Wed, 2025-01-08 at 23:23 +0300, Paul Fertser wrote:
> Hello,
> 
> On Wed, Jan 08, 2025 at 11:23:46AM -0800, Jakub Kicinski wrote:
> > Looks like we're not making any progress on this one, so let's
> > go with the revert for 6.13.
> 
> But this does break userspace, the commit was there for a reason.
> 
> Potin Lai, have you tried deferring this to a work queue instead of
> reverting to the code which has always been wrong?

Jakub, thanks for letting know about revert. ndo_set_mac_address do not
notify userspace about MAC change and as Paul stated it was always been
wrong here.

And we talked about reverts in this thread -
https://lore.kernel.org/all/20231210215356.4154-1-fr0st61te@gmail.com/

Probably, incremental proper fix would be better here?

Common case is 1 NCSI interface for server, we tested on this one and
works fine for us and that's the reason why we didn't catch that
situation. Nowadays some new servers has more than one NCSI interface.
Probably we missed that is softirq context which is obviously not a
place for rtnl_lock/unlock. Is there any other solution about except
delaying dev_set_mac_address in work queue? Or any suggestions about
how to deal with that in a proper way?

Don't have any hw with >2 NCSI interface, probably need help with
testing.

Potin, can you help with checking the fix?

Thanks.

Re: [PATCH net v2] Revert "net/ncsi: change from ndo_set_mac_address to dev_set_mac_address"

[Jakub Kicinski]

On Thu, 09 Jan 2025 00:48:07 +0300 Ivan Mikhaylov wrote:
> Common case is 1 NCSI interface for server, we tested on this one and
> works fine for us and that's the reason why we didn't catch that
> situation. Nowadays some new servers has more than one NCSI interface.
> Probably we missed that is softirq context which is obviously not a
> place for rtnl_lock/unlock. Is there any other solution about except
> delaying dev_set_mac_address in work queue? Or any suggestions about
> how to deal with that in a proper way?

Nothing obvious comes to mind, unfortunately.
But the workqueue shouldn't be too bad.

[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