On Thu, 2020-11-12 at 16:42 +1030, Joel Stanley wrote:
> If a user unbinds and re-binds a NC-SI aware driver the kernel will
> attempt to register the netlink interface at runtime. The structure
> is
> marked __ro_after_init so registration fails spectacularly at this
> point.
Reviewed-by: Samuel Mendoza-Jonas <s...@mendozajonas.com>
>
> # echo 1e660000.ethernet >
> /sys/bus/platform/drivers/ftgmac100/unbind
> # echo 1e660000.ethernet > /sys/bus/platform/drivers/ftgmac100/bind
> ftgmac100 1e660000.ethernet: Read MAC address 52:54:00:12:34:56
> from chip
> ftgmac100 1e660000.ethernet: Using NCSI interface
> 8<--- cut here ---
> Unable to handle kernel paging request at virtual address 80a8f858
> pgd = 8c768dd6
> [80a8f858] *pgd=80a0841e(bad)
> Internal error: Oops: 80d [#1] SMP ARM
> CPU: 0 PID: 116 Comm: sh Not tainted 5.10.0-rc3-next-20201111-
> 00003-gdd25b227ec1e #51
> Hardware name: Generic DT based system
> PC is at genl_register_family+0x1f8/0x6d4
> LR is at 0xff26ffff
> pc : [<8073f930>] lr : [<ff26ffff>] psr: 20000153
> sp : 8553bc80 ip : 81406244 fp : 8553bd04
> r10: 8085d12c r9 : 80a8f73c r8 : 85739000
> r7 : 00000017 r6 : 80a8f860 r5 : 80c8ab98 r4 : 80a8f858
> r3 : 00000000 r2 : 00000000 r1 : 81406130 r0 : 00000017
> Flags: nzCv IRQs on FIQs off Mode SVC_32 ISA ARM Segment none
> Control: 00c5387d Table: 85524008 DAC: 00000051
> Process sh (pid: 116, stack limit = 0x1f1988d6)
> ...
> Backtrace:
> [<8073f738>] (genl_register_family) from [<80860ac0>]
> (ncsi_init_netlink+0x20/0x48)
> r10:8085d12c r9:80c8fb0c r8:85739000 r7:00000000 r6:81218000
> r5:85739000
> r4:8121c000
> [<80860aa0>] (ncsi_init_netlink) from [<8085d740>]
> (ncsi_register_dev+0x1b0/0x210)
> r5:8121c400 r4:8121c000
> [<8085d590>] (ncsi_register_dev) from [<805a8060>]
> (ftgmac100_probe+0x6e0/0x778)
> r10:00000004 r9:80950228 r8:8115bc10 r7:8115ab00 r6:9eae2c24
> r5:813b6f88
> r4:85739000
> [<805a7980>] (ftgmac100_probe) from [<805355ec>]
> (platform_drv_probe+0x58/0xa8)
> r9:80c76bb0 r8:00000000 r7:80cd4974 r6:80c76bb0 r5:8115bc10
> r4:00000000
> [<80535594>] (platform_drv_probe) from [<80532d58>]
> (really_probe+0x204/0x514)
> r7:80cd4974 r6:00000000 r5:80cd4868 r4:8115bc10
>
> Jakub pointed out that ncsi_register_dev is obviously broken, because
> there is only one family so it would never work if there was more
> than
> one ncsi netdev.
>
> Fix the crash by registering the netlink family once on boot, and
> drop
> the code to unregister it.
>
> Fixes: 955dc68cb9b2 ("net/ncsi: Add generic netlink family")
> Signed-off-by: Joel Stanley <j...@jms.id.au>
> ---
> v2: Implement Jakub's suggestion
> - drop __ro_after_init change
> - register netlink with subsys_intcall
> - remove unregister function
>
> net/ncsi/ncsi-manage.c | 5 -----
> net/ncsi/ncsi-netlink.c | 22 +++-------------------
> net/ncsi/ncsi-netlink.h | 3 ---
> 3 files changed, 3 insertions(+), 27 deletions(-)
>
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index f1be3e3f6425..a9cb355324d1 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -1726,9 +1726,6 @@ struct ncsi_dev *ncsi_register_dev(struct
> net_device *dev,
> ndp->ptype.dev = dev;
> dev_add_pack(&ndp->ptype);
>
> - /* Set up generic netlink interface */
> - ncsi_init_netlink(dev);
> -
> pdev = to_platform_device(dev->dev.parent);
> if (pdev) {
> np = pdev->dev.of_node;
> @@ -1892,8 +1889,6 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
> list_del_rcu(&ndp->node);
> spin_unlock_irqrestore(&ncsi_dev_lock, flags);
>
> - ncsi_unregister_netlink(nd->dev);
> -
> kfree(ndp);
> }
> EXPORT_SYMBOL_GPL(ncsi_unregister_dev);
> diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
> index adddc7707aa4..bb5f1650f11c 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c
> @@ -766,24 +766,8 @@ static struct genl_family ncsi_genl_family
> __ro_after_init = {
> .n_small_ops = ARRAY_SIZE(ncsi_ops),
> };
>
> -int ncsi_init_netlink(struct net_device *dev)
> +static int __init ncsi_init_netlink(void)
> {
> - int rc;
> -
> - rc = genl_register_family(&ncsi_genl_family);
> - if (rc)
> - netdev_err(dev, "ncsi: failed to register netlink
> family\n");
> -
> - return rc;
> -}
> -
> -int ncsi_unregister_netlink(struct net_device *dev)
> -{
> - int rc;
> -
> - rc = genl_unregister_family(&ncsi_genl_family);
> - if (rc)
> - netdev_err(dev, "ncsi: failed to unregister netlink
> family\n");
> -
> - return rc;
> + return genl_register_family(&ncsi_genl_family);
> }
> +subsys_initcall(ncsi_init_netlink);
> diff --git a/net/ncsi/ncsi-netlink.h b/net/ncsi/ncsi-netlink.h
> index 7502723fba83..39a1a9d7bf77 100644
> --- a/net/ncsi/ncsi-netlink.h
> +++ b/net/ncsi/ncsi-netlink.h
> @@ -22,7 +22,4 @@ int ncsi_send_netlink_err(struct net_device *dev,
> struct nlmsghdr *nlhdr,
> int err);
>
> -int ncsi_init_netlink(struct net_device *dev);
> -int ncsi_unregister_netlink(struct net_device *dev);
> -
> #endif /* __NCSI_NETLINK_H__ */
