| Message ID | 20240626173503.87636-1-leitao@debian.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next,v2] net: thunderx: Unembed netdev structure | expand |
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!
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
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);
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(-)