lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <7b063d38-311c-76d6-4e31-02f9cccc9bcb@huawei.com> Date: Thu, 8 Sep 2022 19:14:38 +0800 From: "Ziyang Xuan (William)" <william.xuanziyang@...wei.com> To: Oliver Hartkopp <socketcan@...tkopp.net>, <mkl@...gutronix.de>, <edumazet@...gle.com>, <kuba@...nel.org>, <linux-can@...r.kernel.org>, <netdev@...r.kernel.org> CC: <linux-kernel@...r.kernel.org> Subject: Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init() > Just another reference which make it clear that the reordering of function calls in your patch is likely not correct: > > https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734 > > static int __init packet_init(void) > { > int rc; > > rc = proto_register(&packet_proto, 0); > if (rc) > goto out; > rc = sock_register(&packet_family_ops); > if (rc) > goto out_proto; > rc = register_pernet_subsys(&packet_net_ops); > if (rc) > goto out_sock; > rc = register_netdevice_notifier(&packet_netdev_notifier); > if (rc) > goto out_pernet; > > return 0; > > out_pernet: > unregister_pernet_subsys(&packet_net_ops); > out_sock: > sock_unregister(PF_PACKET); > out_proto: > proto_unregister(&packet_proto); > out: > return rc; > } > I had a simple test with can_raw. kernel modification as following: --- a/net/can/af_can.c +++ b/net/can/af_can.c @@ -118,6 +118,8 @@ static int can_create(struct net *net, struct socket *sock, int protocol, const struct can_proto *cp; int err = 0; + printk("%s: protocol: %d\n", __func__, protocol); + sock->state = SS_UNCONNECTED; if (protocol < 0 || protocol >= CAN_NPROTO) diff --git a/net/can/raw.c b/net/can/raw.c index 5dca1e9e44cf..6052fd0cc7b2 100644 --- a/net/can/raw.c +++ b/net/can/raw.c @@ -943,6 +943,9 @@ static __init int raw_module_init(void) pr_info("can: raw protocol\n"); err = can_proto_register(&raw_can_proto); + printk("%s: can_proto_register done\n", __func__); + msleep(5000); // 5s + printk("%s: to register_netdevice_notifier\n", __func__); if (err < 0) pr_err("can: registration of raw protocol failed\n"); else I added 5 seconds delay after can_proto_register() and some debugs. Testcase codes just try to create a CAN_RAW socket in user space as following: int main(int argc, char **argv) { int s; s = socket(PF_CAN, SOCK_RAW, CAN_RAW); if (s < 0) { perror("socket"); return 0; } close(s); return 0; } Execute 'modprobe can_raw' and the testcase we can get message as following: [ 109.312767] can: raw protocol [ 109.312772] raw_module_init: can_proto_register done [ 111.296178] can_create: protocol: 1 [ 114.809141] raw_module_init: to register_netdevice_notifier It proved that it can create CAN_RAW socket and process socket once can_proto_register() successfully. CAN_BCM is the same. In the vast majority of cases, creating protocol socket and operating it are after protocol module initialization. The scenario that I pointed in my patch is a low probability. af_packet.c and af_key.c do like that doesn't mean it's very correct. I think so. Thank you for your prompt reply. > > > On 08.09.22 09:10, Oliver Hartkopp wrote: >> >> >> On 08.09.22 05:04, Ziyang Xuan wrote: >>> Now, register_netdevice_notifier() and register_pernet_subsys() are both >>> after can_proto_register(). It can create CAN_BCM socket and process socket >>> once can_proto_register() successfully, so it is possible missing notifier >>> event or proc node creation because notifier or bcm proc directory is not >>> registered or created yet. Although this is a low probability scenario, it >>> is not impossible. >>> >>> Move register_pernet_subsys() and register_netdevice_notifier() to the >>> front of can_proto_register(). In addition, register_pernet_subsys() and >>> register_netdevice_notifier() may fail, check their results are necessary. >>> >>> Signed-off-by: Ziyang Xuan <william.xuanziyang@...wei.com> >>> --- >>> net/can/bcm.c | 18 +++++++++++++++--- >>> 1 file changed, 15 insertions(+), 3 deletions(-) >>> >>> diff --git a/net/can/bcm.c b/net/can/bcm.c >>> index e60161bec850..e2783156bfd1 100644 >>> --- a/net/can/bcm.c >>> +++ b/net/can/bcm.c >>> @@ -1744,15 +1744,27 @@ static int __init bcm_module_init(void) >>> pr_info("can: broadcast manager protocol\n"); >>> + err = register_pernet_subsys(&canbcm_pernet_ops); >>> + if (err) >>> + return err; >> >> Analogue to your patch for the CAN_RAW socket here (which has been applied to can-next right now) ... >> >> https://lore.kernel.org/linux-can/7af9401f0d2d9fed36c1667b5ac9b8df8f8b87ee.1661584485.git.william.xuanziyang@huawei.com/T/#u >> >> ... I'm not sure whether this is the right sequence to acquire the different resources here. >> >> E.g. in ipsec_pfkey_init() in af_key.c >> >> https://elixir.bootlin.com/linux/v5.19.7/source/net/key/af_key.c#L3887 >> >> proto_register() is executed before register_pernet_subsys() >> >> Which seems to be more natural to me. >> >> Best regards, >> Oliver >> >>> + >>> + err = register_netdevice_notifier(&canbcm_notifier); >>> + if (err) >>> + goto register_notifier_failed; >>> + >>> err = can_proto_register(&bcm_can_proto); >>> if (err < 0) { >>> printk(KERN_ERR "can: registration of bcm protocol failed\n"); >>> - return err; >>> + goto register_proto_failed; >>> } >>> - register_pernet_subsys(&canbcm_pernet_ops); >>> - register_netdevice_notifier(&canbcm_notifier); >>> return 0; >>> + >>> +register_proto_failed: >>> + unregister_netdevice_notifier(&canbcm_notifier); >>> +register_notifier_failed: >>> + unregister_pernet_subsys(&canbcm_pernet_ops); >>> + return err; >>> } >>> static void __exit bcm_module_exit(void) > .
Powered by blists - more mailing lists