Skip to contentSkip to conversation

net: b44: set pause params only when interface is up

PATCH Applied Peter Münster 4 participants 4 messages Apr 24, 2024 – Apr 25, 2024

Subsystems (3)
Networking Network drivers Broadcom B44 10/100 Ethernet driver
lore
Share & tools
lore

Peter Münster

author
#0 lore raw

PATCH v4 1/1 net: b44: set pause params only when interface is up

b44_free_rings() accesses b44::rx_buffers (and ::tx_buffers) unconditionally, but b44::rx_buffers is only valid when the device is up (they get allocated in b44_open(), and deallocated again in b44_close()), any other time these are just a NULL pointers.

So if you try to change the pause params while the network interface is disabled/administratively down, everything explodes (which likely netifd tries to do).

Link https://github.com/openwrt/openwrt/issues/13789
Fixes 1da177e4c3f4 (Linux-2.6.12-rc2)
Cc [email protected]
Reported-by Peter Münster <[email protected]>
Suggested-by Jonas Gorski <[email protected]>
Signed-off-by Vaclav Svoboda <[email protected]>
Tested-by Peter Münster <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>
Signed-off-by Peter Münster <[email protected]>
---
 drivers/net/ethernet/broadcom/b44.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/net/ethernet/broadcom/b44.c +8 -6
index 3e4fb3c3e834..1be6d14030bc 100644
@@ -2009,12 +2009,14 @@ static int b44_set_pauseparam(struct net_device *dev,
 		bp->flags |= B44_FLAG_TX_PAUSE;
 	else
 		bp->flags &= ~B44_FLAG_TX_PAUSE;
-	if (bp->flags & B44_FLAG_PAUSE_AUTO) {
-		b44_halt(bp);
-		b44_init_rings(bp);
-		b44_init_hw(bp, B44_FULL_RESET);
-	} else {
-		__b44_set_flow_ctrl(bp, bp->flags);
+	if (netif_running(dev)) {
+		if (bp->flags & B44_FLAG_PAUSE_AUTO) {
+			b44_halt(bp);
+			b44_init_rings(bp);
+			b44_init_hw(bp, B44_FULL_RESET);
+		} else {
+			__b44_set_flow_ctrl(bp, bp->flags);
+		}
 	}
 	spin_unlock_irq(&bp->lock);
Signature · 1 line
2.35.3
maintainer Reviewed-by on v4 1/1
#1 lore raw
Quoted 10 lines · Peter Münster

b44_free_rings() accesses b44::rx_buffers (and ::tx_buffers) unconditionally, but b44::rx_buffers is only valid when the device is up (they get allocated in b44_open(), and deallocated again in b44_close()), any other time these are just a NULL pointers.

So if you try to change the pause params while the network interface is disabled/administratively down, everything explodes (which likely netifd tries to do).

Quoted 9 lines
Link https://github.com/openwrt/openwrt/issues/13789
Fixes 1da177e4c3f4 (Linux-2.6.12-rc2)
Cc [email protected]
Reported-by Peter Münster <[email protected]>
Suggested-by Jonas Gorski <[email protected]>
Signed-off-by Vaclav Svoboda <[email protected]>
Tested-by Peter Münster <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>
Signed-off-by Peter Münster <[email protected]>
Reviewed-by Michael Chan <[email protected]>
#2 lore raw

PATCH v4 1/1 net: b44: set pause params only when interface is up

In reply to Peter Münster

Understood, this patch fixes the panic issue.
See inline for few comments,

Quoted 37 lines

b44_free_rings() accesses b44::rx_buffers (and ::tx_buffers) unconditionally, but b44::rx_buffers is only valid when the device is up (they get allocated in b44_open(), and deallocated again in b44_close()), any other time these are just a NULL pointers.

So if you try to change the pause params while the network interface is disabled/administratively down, everything explodes (which likely netifd tries to do).

Link https://github.com/openwrt/openwrt/issues/13789
Fixes 1da177e4c3f4 (Linux-2.6.12-rc2)
Cc [email protected]
Reported-by Peter Münster <[email protected]>
Suggested-by Jonas Gorski <[email protected]>
Signed-off-by Vaclav Svoboda <[email protected]>
Tested-by Peter Münster <[email protected]>
Reviewed-by Andrew Lunn <[email protected]>
Signed-off-by Peter Münster <[email protected]>
---
 drivers/net/ethernet/broadcom/b44.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/net/ethernet/broadcom/b44.c +0 -5
index 3e4fb3c3e834..1be6d14030bc 100644
@@ -2009,12 +2009,14 @@ static int b44_set_pauseparam(struct net_device
 *dev,
 		bp->flags |= B44_FLAG_TX_PAUSE;
 	else
 		bp->flags &= ~B44_FLAG_TX_PAUSE;
-	if (bp->flags & B44_FLAG_PAUSE_AUTO) {
-		b44_halt(bp);
-		b44_init_rings(bp);
-		b44_init_hw(bp, B44_FULL_RESET);
-	} else {

drivers/net/ethernet/broadcom/b44.c static int b44_set_pauseparam(struct net_device · line 2021

Quoted 10 lines
drivers/net/ethernet/broadcom/b44.c +8 -1
-		__b44_set_flow_ctrl(bp, bp->flags);
+	if (netif_running(dev)) {
+		if (bp->flags & B44_FLAG_PAUSE_AUTO) {
+			b44_halt(bp);
+			b44_init_rings(bp);
+			b44_init_hw(bp, B44_FULL_RESET);
+		} else {
+			__b44_set_flow_ctrl(bp, bp->flags);
+		}
 	}

The actual register config to enable pause frame is protected with "netif_running", does driver need to reject the request if interface is down.
Otherwise, there is mismatch if someone reads pause frame status (b44_get_pauseparam).

Thanks,
Hariprasad k

Quoted 4 lines
drivers/net/ethernet/broadcom/b44.c +0 -1
 	spin_unlock_irq(&bp->lock);
 
--

2.35.3

CI Applied
#3 lore raw
In reply to Peter Münster
This patch was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 10 lines · On Wed, 24 Apr 2024 15:51:52 +0200 you wrote:

b44_free_rings() accesses b44::rx_buffers (and ::tx_buffers) unconditionally, but b44::rx_buffers is only valid when the device is up (they get allocated in b44_open(), and deallocated again in b44_close()), any other time these are just a NULL pointers.

So if you try to change the pause params while the network interface is disabled/administratively down, everything explodes (which likely netifd tries to do).

[...]

Here is the summary with links:
- [net,v4] net: b44: set pause params only when interface is up

    https://git.kernel.org/netdev/net/c/e3eb7dd47bd4

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html