[PATCH net-next v2] net: thunderx: Unembed netdev structure

[PATCH net-next v2] net: thunderx: Unembed netdev structure

[Breno Leitao]

Embedding net_device into structures prohibits the usage of flexible
arrays in the net_device structure. For more details, see the discussion
at [1].

Un-embed the net_devices from struct lmac by converting them
into pointers, and allocating them dynamically. Use the leverage
alloc_netdev() to allocate the net_device object at
bgx_lmac_enable().

The free of the device occurs at bgx_lmac_disable().

 Do not free_netdevice() if bgx_lmac_enable() fails after lmac->netdev
is allocated, since bgx_lmac_disable() is called if bgx_lmac_enable()
fails, and lmac->netdev will be freed there (similarly to lmac->dmacs).

Link: https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/ [1]
Signed-off-by: Breno Leitao <leitao@debian.org>
---
Changelog:

v2:
	* Fixed a wrong dereference in netdev_priv (Jakub)

 .../net/ethernet/cavium/thunder/thunder_bgx.c | 21 +++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
index a317feb8decb..a40c266c37f2 100644
--- a/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
+++ b/drivers/net/ethernet/cavium/thunder/thunder_bgx.c
@@ -54,7 +54,7 @@ struct lmac {
 	bool			link_up;
 	int			lmacid; /* ID within BGX */
 	int			lmacid_bd; /* ID on board */
-	struct net_device       netdev;
+	struct net_device       *netdev;
 	struct phy_device       *phydev;
 	unsigned int            last_duplex;
 	unsigned int            last_link;
@@ -590,10 +590,12 @@ static void bgx_sgmii_change_link_state(struct lmac *lmac)
 
 static void bgx_lmac_handler(struct net_device *netdev)
 {
-	struct lmac *lmac = container_of(netdev, struct lmac, netdev);
 	struct phy_device *phydev;
+	struct lmac *lmac, **priv;
 	int link_changed = 0;
 
+	priv = netdev_priv(netdev);
+	lmac = *priv;
 	phydev = lmac->phydev;
 
 	if (!phydev->link && lmac->last_link)
@@ -1052,12 +1054,18 @@ static int phy_interface_mode(u8 lmac_type)
 
 static int bgx_lmac_enable(struct bgx *bgx, u8 lmacid)
 {
-	struct lmac *lmac;
+	struct lmac *lmac, **priv;
 	u64 cfg;
 
 	lmac = &bgx->lmac[lmacid];
 	lmac->bgx = bgx;
 
+	lmac->netdev = alloc_netdev_dummy(sizeof(struct lmac *));
+	if (!lmac->netdev)
+		return -ENOMEM;
+	priv = netdev_priv(lmac->netdev);
+	*priv = lmac;
+
 	if ((lmac->lmac_type == BGX_MODE_SGMII) ||
 	    (lmac->lmac_type == BGX_MODE_QSGMII) ||
 	    (lmac->lmac_type == BGX_MODE_RGMII)) {
@@ -1116,7 +1124,7 @@ static int bgx_lmac_enable(struct bgx *bgx, u8 lmacid)
 		}
 		lmac->phydev->dev_flags = 0;
 
-		if (phy_connect_direct(&lmac->netdev, lmac->phydev,
+		if (phy_connect_direct(lmac->netdev, lmac->phydev,
 				       bgx_lmac_handler,
 				       phy_interface_mode(lmac->lmac_type)))
 			return -ENODEV;
@@ -1183,6 +1191,7 @@ static void bgx_lmac_disable(struct bgx *bgx, u8 lmacid)
 	    (lmac->lmac_type != BGX_MODE_10G_KR) && lmac->phydev)
 		phy_disconnect(lmac->phydev);
 
+	free_netdev(lmac->netdev);
 	lmac->phydev = NULL;
 }
 
@@ -1414,7 +1423,7 @@ static acpi_status bgx_acpi_register_phy(acpi_handle handle,
 
 	acpi_get_mac_address(dev, adev, bgx->lmac[bgx->acpi_lmac_idx].mac);
 
-	SET_NETDEV_DEV(&bgx->lmac[bgx->acpi_lmac_idx].netdev, dev);
+	SET_NETDEV_DEV(bgx->lmac[bgx->acpi_lmac_idx].netdev, dev);
 
 	bgx->lmac[bgx->acpi_lmac_idx].lmacid = bgx->acpi_lmac_idx;
 	bgx->acpi_lmac_idx++; /* move to next LMAC */
@@ -1483,7 +1492,7 @@ static int bgx_init_of_phy(struct bgx *bgx)
 
 		of_get_mac_address(node, bgx->lmac[lmac].mac);
 
-		SET_NETDEV_DEV(&bgx->lmac[lmac].netdev, &bgx->pdev->dev);
+		SET_NETDEV_DEV(bgx->lmac[lmac].netdev, &bgx->pdev->dev);
 		bgx->lmac[lmac].lmacid = lmac;
 
 		phy_np = of_parse_phandle(node, "phy-handle", 0);
-- 
2.43.0

Re: [PATCH net-next v2] net: thunderx: Unembed netdev structure

[patchwork-bot+netdevbpf]

Hello:

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

On Wed, 26 Jun 2024 10:35:02 -0700 you wrote:
> Embedding net_device into structures prohibits the usage of flexible
> arrays in the net_device structure. For more details, see the discussion
> at [1].
> 
> Un-embed the net_devices from struct lmac by converting them
> into pointers, and allocating them dynamically. Use the leverage
> alloc_netdev() to allocate the net_device object at
> bgx_lmac_enable().
> 
> [...]

Here is the summary with links:
  - [net-next,v2] net: thunderx: Unembed netdev structure
    https://git.kernel.org/netdev/net-next/c/94833addfaba

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

Re: [PATCH net-next v2] net: thunderx: Unembed netdev structure

[Marc Zyngier]

On Wed, 26 Jun 2024 18:35:02 +0100,
Breno Leitao <leitao@debian.org> wrote:
> 
> Embedding net_device into structures prohibits the usage of flexible
> arrays in the net_device structure. For more details, see the discussion
> at [1].
> 
> Un-embed the net_devices from struct lmac by converting them
> into pointers, and allocating them dynamically. Use the leverage
> alloc_netdev() to allocate the net_device object at
> bgx_lmac_enable().
> 
> The free of the device occurs at bgx_lmac_disable().
> 
>  Do not free_netdevice() if bgx_lmac_enable() fails after lmac->netdev
> is allocated, since bgx_lmac_disable() is called if bgx_lmac_enable()
> fails, and lmac->netdev will be freed there (similarly to lmac->dmacs).
> 
> Link: https://lore.kernel.org/all/20240229225910.79e224cf@kernel.org/ [1]
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> Changelog:
> 
> v2:
> 	* Fixed a wrong dereference in netdev_priv (Jakub)
> 
>  .../net/ethernet/cavium/thunder/thunder_bgx.c | 21 +++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)

This patch causes my ThunderX box to explode badly:

[   10.022118] thunder_bgx, ver 1.0
[   10.022594] libata version 3.00 loaded.
[   10.023226] mdio_thunder 0000:01:01.3: Added bus at 87e005003800
[   10.023757] mdio_thunder 0000:01:01.3: Added bus at 87e005003880
[   10.035431] thunder_bgx 0000:01:10.0: BGX0 QLM mode: XFI
[   10.045225] Unable to handle kernel NULL pointer dereference at virtual address 00000000000005e8
[   10.069901] Mem abort info:
[   10.085236]   ESR = 0x0000000096000044
[   10.109767]   EC = 0x25: DABT (current EL), IL = 32 bits
[   10.145191]   SET = 0, FnV = 0
[   10.148272]   EA = 0, S1PTW = 0
[   10.151422]   FSC = 0x04: level 0 translation fault
[   10.156309] Data abort info:
[   10.159196]   ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
[   10.164689]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
[   10.169752]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[   10.175076] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000111b43000
[   10.181533] [00000000000005e8] pgd=0000000000000000, p4d=0000000000000000
[   10.188328] Internal error: Oops: 0000000096000044 [#1] PREEMPT SMP
[   10.194585] Modules linked in: libahci(E) nvme(E) nvme_core(E) t10_pi(E) mdio_thunder(E) thunder_bgx(E+) libata(E) mdio_devres(E) crc64_rocksoft(E) scsi_mod(E) igb(E+) thunder_xcv(E) mdio_cavium(E) crc64(E) i2c_algo_bit(E) gpio_keys(E) usbhid(E) scsi_common(E) of_mdio(E) fixed_phy(E) fwnode_mdio(E) i2c_thunderx(E) libphy(E)
[   10.223291] CPU: 0 PID: 341 Comm: kworker/0:4 Tainted: G            E      6.10.0-rc5-01073-g94833addfaba #3309
[   10.233368] Hardware name: GIGABYTE MT30-GS0/MT30-GS0, BIOS F02 08/06/2019
[   10.240231] Workqueue: events work_for_cpu_fn
[   10.244588] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   10.251540] pc : bgx_probe+0x44c/0x640 [thunder_bgx]
[   10.256502] lr : bgx_probe+0x410/0x640 [thunder_bgx]
[   10.261460] sp : ffff800084dd3c80
[   10.264876] x29: ffff800084dd3c80 x28: 0000000000000000 x27: ffff000ff6772a70
[   10.272006] x26: ffff00010cb02480 x25: 0000000000000000 x24: ffff80007a325700
[   10.279136] x23: ffff00010c0e60c8 x22: ffff80008100a3d8 x21: ffff000ff6772a88
[   10.286266] x20: ffff00010c0e6000 x19: ffff00010cb02480 x18: ffffffffffffffff
[   10.293396] x17: 000000004b2d2331 x16: 00000000b606f3da x15: 0000000000000006
[   10.300526] x14: 0000000000000000 x13: 3030383330303530 x12: 3065373820746120
[   10.307656] x11: 7375622064656464 x10: ffff800081e158e8 x9 : ffff800080aa9a30
[   10.314786] x8 : 0101010101010101 x7 : 0000000000000000 x6 : ffff00010c0e60c8
[   10.321916] x5 : ffff800084dd3cf8 x4 : 0000000000000000 x3 : 0000000000000000
[   10.329046] x2 : 0000000000000000 x1 : ffff80007a3296d0 x0 : ffff000ff6772a70
[   10.336176] Call trace:
[   10.338613]  bgx_probe+0x44c/0x640 [thunder_bgx]
[   10.343225]  local_pci_probe+0x48/0xb8
[   10.346966]  work_for_cpu_fn+0x24/0x40
[   10.350706]  process_one_work+0x170/0x400
[   10.354707]  worker_thread+0x26c/0x388
[   10.358446]  kthread+0xfc/0x110
[   10.361580]  ret_from_fork+0x10/0x20
[   10.365150] Code: 52800004 52800003 d2800002 f9401f47 (f902f4e6) 
[   10.371232] ---[ end trace 0000000000000000 ]---

and I've confirmed that reverting this patch on top of -rc3 restores
normal behaviour.

There are two issues with this change:

- bgx_lmac_enable() is called *after* bgx_acpi_register_phy() and
  bgx_init_of_phy(), both expecting netdev to be a valid pointer.

- bgx_init_of_phy() populates the MAC addresses for *all* LMACs
  attached to a given BGX instance, and thus needs netdev for each of
  them to have been allocated.

I have posted a potential fix at [1].

Thanks,

	M.

[1] https://lore.kernel.org/r/20240812141322.1742918-1-maz@kernel.org

-- 
Without deviation from the norm, progress is not possible.