diff mbox series

[v2] net/ncsi: Fix netlink registration

Message ID 20201112061210.914621-1-joel@jms.id.au
State New
Headers show
Series [v2] net/ncsi: Fix netlink registration | expand

Commit Message

Joel Stanley Nov. 12, 2020, 6:12 a.m. UTC 
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.

 # 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 <joel@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(-)

-- 
2.28.0

Comments

Samuel Mendoza-Jonas Nov. 12, 2020, 4:25 p.m. UTC | #1 
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 <sam@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 <joel@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__ */
Jakub Kicinski Nov. 13, 2020, 1:02 a.m. UTC | #2 
On Thu, 12 Nov 2020 08:25:39 -0800 Samuel Mendoza-Jonas wrote:
> 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 <sam@mendozajonas.com>


Applied to net, thanks!
diff mbox series

Patch

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__ */