[PATCH 0/3] Fix prestera driver fail to probe twice

[PATCH 0/3] Fix prestera driver fail to probe twice

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Fix issues resulting from insmod, rmmod and insmod of the
prestera driver:

1. Time-out
2. memory referencing after freeing
3. MAC addresses wrapping

Elad Nachman (3):
  net: marvell: prestera: fix driver reload
  net: marvell: prestera: fix memory use after free
  net: marvell: prestera: force good base mac

 drivers/net/ethernet/marvell/prestera/prestera_hw.c       | 8 ++++++++
 drivers/net/ethernet/marvell/prestera/prestera_hw.h       | 1 +
 drivers/net/ethernet/marvell/prestera/prestera_main.c     | 7 +++++--
 drivers/net/ethernet/marvell/prestera/prestera_pci.c      | 7 ++++++-
 drivers/net/ethernet/marvell/prestera/prestera_router.c   | 1 -
 .../net/ethernet/marvell/prestera/prestera_router_hw.c    | 1 -
 6 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH 1/3] net: marvell: prestera: fix driver reload

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Driver rmmod after insmod would fail because of
the following issues:

1. API call to reset the switch HW and restart the
   firmware CPU firmware code loading was missing in
   driver removal code handler.
2. Timeout waiting for the firmware CPU firmware
   loader code to start was too small.

Fix by adding API call to reset the switch HW and
restart the firmware CPU firmware code loading when
handling the driver removal procedure,
increase the timeout waiting for this restart operation
from 5 to 30 seconds.

Reported-by: Köry Maincent <kory.maincent@bootlin.com>
Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_hw.c   | 8 ++++++++
 drivers/net/ethernet/marvell/prestera/prestera_hw.h   | 1 +
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 3 ++-
 drivers/net/ethernet/marvell/prestera/prestera_pci.c  | 7 ++++++-
 4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
index fc6f7d2746e8..08de8b498e0a 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
@@ -21,6 +21,7 @@
 enum prestera_cmd_type_t {
 	PRESTERA_CMD_TYPE_SWITCH_INIT = 0x1,
 	PRESTERA_CMD_TYPE_SWITCH_ATTR_SET = 0x2,
+	PRESTERA_CMD_TYPE_SWITCH_RESET = 0x4,
 
 	PRESTERA_CMD_TYPE_PORT_ATTR_SET = 0x100,
 	PRESTERA_CMD_TYPE_PORT_ATTR_GET = 0x101,
@@ -1087,6 +1088,13 @@ void prestera_hw_switch_fini(struct prestera_switch *sw)
 	WARN_ON(!list_empty(&sw->event_handlers));
 }
 
+int prestera_hw_switch_reset(struct prestera_switch *sw)
+{
+	struct prestera_msg_common_req req;
+
+	return prestera_cmd(sw, PRESTERA_CMD_TYPE_SWITCH_RESET, &req.cmd, sizeof(req));
+}
+
 int prestera_hw_switch_ageing_set(struct prestera_switch *sw, u32 ageing_ms)
 {
 	struct prestera_msg_switch_attr_req req = {
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.h b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
index 0a929279e1ce..86217bea2ca0 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_hw.h
+++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
@@ -150,6 +150,7 @@ struct prestera_neigh_info;
 
 /* Switch API */
 int prestera_hw_switch_init(struct prestera_switch *sw);
+int prestera_hw_switch_reset(struct prestera_switch *sw);
 void prestera_hw_switch_fini(struct prestera_switch *sw);
 int prestera_hw_switch_ageing_set(struct prestera_switch *sw, u32 ageing_ms);
 int prestera_hw_switch_mac_set(struct prestera_switch *sw, const char *mac);
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index 4fb886c57cd7..bcaa8ea27b49 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -1444,7 +1444,7 @@ static int prestera_switch_init(struct prestera_switch *sw)
 err_router_init:
 	prestera_netdev_event_handler_unregister(sw);
 	prestera_hw_switch_fini(sw);
-
+	prestera_hw_switch_reset(sw);
 	return err;
 }
 
@@ -1463,6 +1463,7 @@ static void prestera_switch_fini(struct prestera_switch *sw)
 	prestera_router_fini(sw);
 	prestera_netdev_event_handler_unregister(sw);
 	prestera_hw_switch_fini(sw);
+	prestera_hw_switch_reset(sw);
 	of_node_put(sw->np);
 }
 
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
index 35857dc19542..b75a263ad8cb 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
@@ -24,6 +24,11 @@
 #define PRESTERA_FW_ARM64_PATH_FMT "mrvl/prestera/mvsw_prestera_fw_arm64-v%u.%u.img"
 
 #define PRESTERA_FW_HDR_MAGIC		0x351D9D06
+/* Timeout waiting for prestera firmware CPU to reboot and
+ * restart the firmware loading software layer, hence becoming
+ * ready for the next firmware downloading phase:
+ */
+#define PRESTERA_FW_READY_TIMEOUT_SECS	30
 #define PRESTERA_FW_DL_TIMEOUT_MS	50000
 #define PRESTERA_FW_BLK_SZ		1024
 
@@ -765,7 +770,7 @@ static int prestera_fw_load(struct prestera_fw *fw)
 
 	err = prestera_ldr_wait_reg32(fw, PRESTERA_LDR_READY_REG,
 				      PRESTERA_LDR_READY_MAGIC,
-				      5 * MSEC_PER_SEC);
+				      PRESTERA_FW_READY_TIMEOUT_SECS * MSEC_PER_SEC);
 	if (err) {
 		dev_err(fw->dev.dev, "waiting for FW loader is timed out");
 		return err;
-- 
2.25.1

Re: [PATCH 1/3] net: marvell: prestera: fix driver reload

[Andrew Lunn]

On Mon, Mar 11, 2024 at 03:51:10PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Driver rmmod after insmod would fail because of
> the following issues:
> 
> 1. API call to reset the switch HW and restart the
>    firmware CPU firmware code loading was missing in
>    driver removal code handler.
> 2. Timeout waiting for the firmware CPU firmware
>    loader code to start was too small.
> 
> Fix by adding API call to reset the switch HW and
> restart the firmware CPU firmware code loading when
> handling the driver removal procedure,
> increase the timeout waiting for this restart operation
> from 5 to 30 seconds.

The commit messages suggests this does two things. So it should be two
patches.

Please could you explain why the firmware needs resetting during
shutdown. That is odd, so should be explained. The commit message is
about Why?, not What?

      Andrew

Re: [PATCH 1/3] net: marvell: prestera: fix driver reload

[Denis Kirjanov]

On 3/11/24 16:51, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Driver rmmod after insmod would fail because of
> the following issues:
> 
> 1. API call to reset the switch HW and restart the
>    firmware CPU firmware code loading was missing in
>    driver removal code handler.
> 2. Timeout waiting for the firmware CPU firmware
>    loader code to start was too small.
> 
> Fix by adding API call to reset the switch HW and
> restart the firmware CPU firmware code loading when
> handling the driver removal procedure,
> increase the timeout waiting for this restart operation
> from 5 to 30 seconds.
> 
> Reported-by: Köry Maincent <kory.maincent@bootlin.com>
> Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
> Signed-off-by: Elad Nachman <enachman@marvell.com>
Please add Fixes tag
> ---
>  drivers/net/ethernet/marvell/prestera/prestera_hw.c   | 8 ++++++++
>  drivers/net/ethernet/marvell/prestera/prestera_hw.h   | 1 +
>  drivers/net/ethernet/marvell/prestera/prestera_main.c | 3 ++-
>  drivers/net/ethernet/marvell/prestera/prestera_pci.c  | 7 ++++++-
>  4 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> index fc6f7d2746e8..08de8b498e0a 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> @@ -21,6 +21,7 @@
>  enum prestera_cmd_type_t {
>  	PRESTERA_CMD_TYPE_SWITCH_INIT = 0x1,
>  	PRESTERA_CMD_TYPE_SWITCH_ATTR_SET = 0x2,
> +	PRESTERA_CMD_TYPE_SWITCH_RESET = 0x4,
>  
>  	PRESTERA_CMD_TYPE_PORT_ATTR_SET = 0x100,
>  	PRESTERA_CMD_TYPE_PORT_ATTR_GET = 0x101,
> @@ -1087,6 +1088,13 @@ void prestera_hw_switch_fini(struct prestera_switch *sw)
>  	WARN_ON(!list_empty(&sw->event_handlers));
>  }
>  
> +int prestera_hw_switch_reset(struct prestera_switch *sw)
> +{
> +	struct prestera_msg_common_req req;
> +
> +	return prestera_cmd(sw, PRESTERA_CMD_TYPE_SWITCH_RESET, &req.cmd, sizeof(req));
> +}
> +
>  int prestera_hw_switch_ageing_set(struct prestera_switch *sw, u32 ageing_ms)
>  {
>  	struct prestera_msg_switch_attr_req req = {
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.h b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
> index 0a929279e1ce..86217bea2ca0 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_hw.h
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
> @@ -150,6 +150,7 @@ struct prestera_neigh_info;
>  
>  /* Switch API */
>  int prestera_hw_switch_init(struct prestera_switch *sw);
> +int prestera_hw_switch_reset(struct prestera_switch *sw);
>  void prestera_hw_switch_fini(struct prestera_switch *sw);
>  int prestera_hw_switch_ageing_set(struct prestera_switch *sw, u32 ageing_ms);
>  int prestera_hw_switch_mac_set(struct prestera_switch *sw, const char *mac);
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> index 4fb886c57cd7..bcaa8ea27b49 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> @@ -1444,7 +1444,7 @@ static int prestera_switch_init(struct prestera_switch *sw)
>  err_router_init:
>  	prestera_netdev_event_handler_unregister(sw);
>  	prestera_hw_switch_fini(sw);
> -
> +	prestera_hw_switch_reset(sw);
>  	return err;
>  }
>  
> @@ -1463,6 +1463,7 @@ static void prestera_switch_fini(struct prestera_switch *sw)
>  	prestera_router_fini(sw);
>  	prestera_netdev_event_handler_unregister(sw);
>  	prestera_hw_switch_fini(sw);
> +	prestera_hw_switch_reset(sw);
>  	of_node_put(sw->np);
>  }
>  
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> index 35857dc19542..b75a263ad8cb 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> @@ -24,6 +24,11 @@
>  #define PRESTERA_FW_ARM64_PATH_FMT "mrvl/prestera/mvsw_prestera_fw_arm64-v%u.%u.img"
>  
>  #define PRESTERA_FW_HDR_MAGIC		0x351D9D06
> +/* Timeout waiting for prestera firmware CPU to reboot and
> + * restart the firmware loading software layer, hence becoming
> + * ready for the next firmware downloading phase:
> + */
> +#define PRESTERA_FW_READY_TIMEOUT_SECS	30
>  #define PRESTERA_FW_DL_TIMEOUT_MS	50000
>  #define PRESTERA_FW_BLK_SZ		1024
>  
> @@ -765,7 +770,7 @@ static int prestera_fw_load(struct prestera_fw *fw)
>  
>  	err = prestera_ldr_wait_reg32(fw, PRESTERA_LDR_READY_REG,
>  				      PRESTERA_LDR_READY_MAGIC,
> -				      5 * MSEC_PER_SEC);
> +				      PRESTERA_FW_READY_TIMEOUT_SECS * MSEC_PER_SEC);
>  	if (err) {
>  		dev_err(fw->dev.dev, "waiting for FW loader is timed out");
>  		return err;

Re: [PATCH 1/3] net: marvell: prestera: fix driver reload

[Przemek Kitszel]

On 3/11/24 14:51, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>

Just a note, not an issue for me:
it's unusual to have "From:" line about yourself, especially that
you use the very same address that your "Sender", and your Signoff.

> 
> Driver rmmod after insmod would fail because of
> the following issues:
> 
> 1. API call to reset the switch HW and restart the
>     firmware CPU firmware code loading was missing in
>     driver removal code handler.
> 2. Timeout waiting for the firmware CPU firmware
>     loader code to start was too small.
> 
> Fix by adding API call to reset the switch HW and
> restart the firmware CPU firmware code loading when

'firmware CPU firmware' sounds off;
you could also reflow your commit message to use up to 75 chars,
instead of shorter lines.

> handling the driver removal procedure,
> increase the timeout waiting for this restart operation
> from 5 to 30 seconds.
> 
> Reported-by: Köry Maincent <kory.maincent@bootlin.com>
> Closes: https://lore.kernel.org/netdev/20240208101005.29e8c7f3@kmaincent-XPS-13-7390/T/
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>   drivers/net/ethernet/marvell/prestera/prestera_hw.c   | 8 ++++++++
>   drivers/net/ethernet/marvell/prestera/prestera_hw.h   | 1 +
>   drivers/net/ethernet/marvell/prestera/prestera_main.c | 3 ++-
>   drivers/net/ethernet/marvell/prestera/prestera_pci.c  | 7 ++++++-
>   4 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> index fc6f7d2746e8..08de8b498e0a 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> @@ -21,6 +21,7 @@
>   enum prestera_cmd_type_t {
>   	PRESTERA_CMD_TYPE_SWITCH_INIT = 0x1,
>   	PRESTERA_CMD_TYPE_SWITCH_ATTR_SET = 0x2,
> +	PRESTERA_CMD_TYPE_SWITCH_RESET = 0x4,
>   
>   	PRESTERA_CMD_TYPE_PORT_ATTR_SET = 0x100,
>   	PRESTERA_CMD_TYPE_PORT_ATTR_GET = 0x101,
> @@ -1087,6 +1088,13 @@ void prestera_hw_switch_fini(struct prestera_switch *sw)
>   	WARN_ON(!list_empty(&sw->event_handlers));
>   }
>   
> +int prestera_hw_switch_reset(struct prestera_switch *sw)
> +{
> +	struct prestera_msg_common_req req;
> +
> +	return prestera_cmd(sw, PRESTERA_CMD_TYPE_SWITCH_RESET, &req.cmd, sizeof(req));
> +}
> +
>   int prestera_hw_switch_ageing_set(struct prestera_switch *sw, u32 ageing_ms)
>   {
>   	struct prestera_msg_switch_attr_req req = {
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.h b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
> index 0a929279e1ce..86217bea2ca0 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_hw.h
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
> @@ -150,6 +150,7 @@ struct prestera_neigh_info;
>   
>   /* Switch API */
>   int prestera_hw_switch_init(struct prestera_switch *sw);
> +int prestera_hw_switch_reset(struct prestera_switch *sw);
>   void prestera_hw_switch_fini(struct prestera_switch *sw);
>   int prestera_hw_switch_ageing_set(struct prestera_switch *sw, u32 ageing_ms);
>   int prestera_hw_switch_mac_set(struct prestera_switch *sw, const char *mac);
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> index 4fb886c57cd7..bcaa8ea27b49 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> @@ -1444,7 +1444,7 @@ static int prestera_switch_init(struct prestera_switch *sw)
>   err_router_init:
>   	prestera_netdev_event_handler_unregister(sw);
>   	prestera_hw_switch_fini(sw);
> -
> +	prestera_hw_switch_reset(sw);
>   	return err;
>   }
>   
> @@ -1463,6 +1463,7 @@ static void prestera_switch_fini(struct prestera_switch *sw)
>   	prestera_router_fini(sw);
>   	prestera_netdev_event_handler_unregister(sw);
>   	prestera_hw_switch_fini(sw);
> +	prestera_hw_switch_reset(sw);
>   	of_node_put(sw->np);
>   }
>   
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_pci.c b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> index 35857dc19542..b75a263ad8cb 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> @@ -24,6 +24,11 @@
>   #define PRESTERA_FW_ARM64_PATH_FMT "mrvl/prestera/mvsw_prestera_fw_arm64-v%u.%u.img"
>   
>   #define PRESTERA_FW_HDR_MAGIC		0x351D9D06
> +/* Timeout waiting for prestera firmware CPU to reboot and

it's implicit in your driver that firmware is "prestera firmware"

> + * restart the firmware loading software layer, hence becoming
> + * ready for the next firmware downloading phase:
> + */
> +#define PRESTERA_FW_READY_TIMEOUT_SECS	30
>   #define PRESTERA_FW_DL_TIMEOUT_MS	50000

nice that you have separate timeout for Download and 
BeReadyToStartDownload ;)

it would be less typing and more consistent to have 30000 and _MS
suffix, but that's just a nitpick

>   #define PRESTERA_FW_BLK_SZ		1024
>   
> @@ -765,7 +770,7 @@ static int prestera_fw_load(struct prestera_fw *fw)
>   
>   	err = prestera_ldr_wait_reg32(fw, PRESTERA_LDR_READY_REG,
>   				      PRESTERA_LDR_READY_MAGIC,
> -				      5 * MSEC_PER_SEC);
> +				      PRESTERA_FW_READY_TIMEOUT_SECS * MSEC_PER_SEC);
>   	if (err) {
>   		dev_err(fw->dev.dev, "waiting for FW loader is timed out");
>   		return err;

Re: [PATCH 1/3] net: marvell: prestera: fix driver reload

[Köry Maincent]

On Mon, 11 Mar 2024 15:51:10 +0200
Elad Nachman <enachman@marvell.com> wrote:

> From: Elad Nachman <enachman@marvell.com>
> 
> Driver rmmod after insmod would fail because of
> the following issues:
> 
> 1. API call to reset the switch HW and restart the
>    firmware CPU firmware code loading was missing in
>    driver removal code handler.
> 2. Timeout waiting for the firmware CPU firmware
>    loader code to start was too small.
> 
> Fix by adding API call to reset the switch HW and
> restart the firmware CPU firmware code loading when
> handling the driver removal procedure,
> increase the timeout waiting for this restart operation
> from 5 to 30 seconds.

Tested-by: Kory Maincent <kory.maincent@bootlin.com>

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH 2/3] net: marvell: prestera: fix memory use after free

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Prestera driver routing module cleanup process would
release memory and then reference it again, and eventually
free it again.
Remove the redundant first memory free call.
All such double free calls were detected using KASAN.

Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_router.c    | 1 -
 drivers/net/ethernet/marvell/prestera/prestera_router_hw.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router.c b/drivers/net/ethernet/marvell/prestera/prestera_router.c
index de317179a7dc..2da04a17efad 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_router.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_router.c
@@ -1638,7 +1638,6 @@ void prestera_router_fini(struct prestera_switch *sw)
 	prestera_k_arb_abort(sw);
 
 	kfree(sw->router->nhgrp_hw_state_cache);
-	rhashtable_destroy(&sw->router->kern_fib_cache_ht);
 	prestera_router_hw_fini(sw);
 	kfree(sw->router);
 	sw->router = NULL;
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
index 02faaea2aefa..254107f664b4 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_router_hw.c
@@ -102,7 +102,6 @@ void prestera_router_hw_fini(struct prestera_switch *sw)
 				    prestera_fib_node_destroy_ht_cb, sw);
 	WARN_ON(!list_empty(&sw->router->vr_list));
 	WARN_ON(!list_empty(&sw->router->rif_entry_list));
-	rhashtable_destroy(&sw->router->fib_ht);
 	rhashtable_destroy(&sw->router->nexthop_group_ht);
 	rhashtable_destroy(&sw->router->nh_neigh_ht);
 }
-- 
2.25.1

Re: [PATCH 2/3] net: marvell: prestera: fix memory use after free

[Kory Maincent]

On Mon, 11 Mar 2024 15:51:11 +0200
Elad Nachman <enachman@marvell.com> wrote:

> From: Elad Nachman <enachman@marvell.com>
> 
> Prestera driver routing module cleanup process would
> release memory and then reference it again, and eventually
> free it again.
> Remove the redundant first memory free call.
> All such double free calls were detected using KASAN.
 
Not directly related to this patch but I am wondering if
the call to prestera_port_sfp_unbind(port) is not missing in
prestera_destroy_ports() function?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

RE: [EXTERNAL] Re: [PATCH 2/3] net: marvell: prestera: fix memory use after free

[Elad Nachman]

> -----Original Message-----
> From: Kory Maincent <kory.maincent@bootlin.com>
> Sent: Tuesday, March 12, 2024 5:45 PM
> To: Elad Nachman <enachman@marvell.com>
> Cc: Taras Chornyi <taras.chornyi@plvision.eu>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> andrew@lunn.ch; thomas.petazzoni@bootlin.com;
> miquel.raynal@bootlin.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH 2/3] net: marvell: prestera: fix memory use
> after free
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> On Mon, 11 Mar 2024 15:51:11 +0200
> Elad Nachman <enachman@marvell.com> wrote:
> 
> > From: Elad Nachman <enachman@marvell.com>
> >
> > Prestera driver routing module cleanup process would release memory
> > and then reference it again, and eventually free it again.
> > Remove the redundant first memory free call.
> > All such double free calls were detected using KASAN.
> 
> Not directly related to this patch but I am wondering if the call to
> prestera_port_sfp_unbind(port) is not missing in
> prestera_destroy_ports() function?

Yes, it is.

> 
> Regards,
> --
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__bootlin.com&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-
> TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=VnjwUBq4QS_Onl_07gp1OUp2XoG
> pT-
> bZ5snEOAg5gf3CM3l5GPgXQ1pb3GzEa6bb&s=GchzqI3lHyGuFKhepEFMFsXwp
> 5oMSH9iqwBZuTtzhjc&e=

Elad.

[PATCH 3/3] net: marvell: prestera: force good base mac

[Elad Nachman]

From: Elad Nachman <enachman@marvell.com>

Since each switchport MAC address uses the switch base mac address
and adds the physical port number to it,
Force the last byte of the switch base mac address to be at
least 128, so when adding to it, we will not wrap around the
previous (more significant) mac address byte, resulting in a
warning message.

Signed-off-by: Elad Nachman <enachman@marvell.com>
---
 drivers/net/ethernet/marvell/prestera/prestera_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
index bcaa8ea27b49..e17b1a24fe18 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
@@ -859,7 +859,9 @@ static int prestera_switch_set_base_mac_addr(struct prestera_switch *sw)
 	if (sw->np)
 		ret = of_get_mac_address(sw->np, sw->base_mac);
 	if (!is_valid_ether_addr(sw->base_mac) || ret) {
-		eth_random_addr(sw->base_mac);
+		do {
+			eth_random_addr(sw->base_mac);
+		} while (sw->base_mac[5] > 0x80);
 		dev_info(prestera_dev(sw), "using random base mac address\n");
 	}
 
-- 
2.25.1

Re: [PATCH 3/3] net: marvell: prestera: force good base mac

[Przemek Kitszel]

On 3/11/24 14:51, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Since each switchport MAC address uses the switch base mac address
> and adds the physical port number to it,
> Force the last byte of the switch base mac address to be at
> least 128, so when adding to it, we will not wrap around the

to be at *most* 128

> previous (more significant) mac address byte, resulting in a
> warning message.
> 
> Signed-off-by: Elad Nachman <enachman@marvell.com>
> ---
>   drivers/net/ethernet/marvell/prestera/prestera_main.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> index bcaa8ea27b49..e17b1a24fe18 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_main.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> @@ -859,7 +859,9 @@ static int prestera_switch_set_base_mac_addr(struct prestera_switch *sw)
>   	if (sw->np)
>   		ret = of_get_mac_address(sw->np, sw->base_mac);
>   	if (!is_valid_ether_addr(sw->base_mac) || ret) {
> -		eth_random_addr(sw->base_mac);
> +		do {
> +			eth_random_addr(sw->base_mac);
> +		} while (sw->base_mac[5] > 0x80);

Instead of this loop, that uses 6 bytes of random data at each step,
I would just fix the last byte.

Either by calling get_random_u8() in a loop, or perhaps better, just
toggle of MSB unconditionally:
sw->base_mac[5] &= ~0x80; // or '&= 127'

what would change your condition in commit message to "to be at most
127", but I think that should be fine, granted simpler code.

>   		dev_info(prestera_dev(sw), "using random base mac address\n");
>   	}
>   

Re: [PATCH 3/3] net: marvell: prestera: force good base mac

[Andrew Lunn]

On Mon, Mar 11, 2024 at 03:51:12PM +0200, Elad Nachman wrote:
> From: Elad Nachman <enachman@marvell.com>
> 
> Since each switchport MAC address uses the switch base mac address
> and adds the physical port number to it,
> Force the last byte of the switch base mac address to be at
> least 128, so when adding to it, we will not wrap around the
> previous (more significant) mac address byte, resulting in a
> warning message.

It is not clear to me what the real problem is here. Does the hardware
require that the MAC addresses always have the same upper 5 bytes?

	Andrew