From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D918CC38145 for ; Thu, 8 Sep 2022 11:14:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229548AbiIHLOu (ORCPT ); Thu, 8 Sep 2022 07:14:50 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51702 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229911AbiIHLOp (ORCPT ); Thu, 8 Sep 2022 07:14:45 -0400 Received: from szxga03-in.huawei.com (szxga03-in.huawei.com [45.249.212.189]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 14ECEBCA7; Thu, 8 Sep 2022 04:14:41 -0700 (PDT) Received: from canpemm500006.china.huawei.com (unknown [172.30.72.55]) by szxga03-in.huawei.com (SkyGuard) with ESMTP id 4MNc0q6JS1zHnj4; Thu, 8 Sep 2022 19:12:43 +0800 (CST) Received: from [10.174.179.200] (10.174.179.200) by canpemm500006.china.huawei.com (7.192.105.130) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.24; Thu, 8 Sep 2022 19:14:39 +0800 Subject: Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module_init() To: Oliver Hartkopp , , , , , CC: References: <823cff0ebec33fa9389eeaf8b8ded3217c32cb38.1662606045.git.william.xuanziyang@huawei.com> <381dd961-f786-2400-0977-9639c3f7006e@hartkopp.net> From: "Ziyang Xuan (William)" Message-ID: <7b063d38-311c-76d6-4e31-02f9cccc9bcb@huawei.com> Date: Thu, 8 Sep 2022 19:14:38 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.179.200] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To canpemm500006.china.huawei.com (7.192.105.130) X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > 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 >>> --- >>>   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) > .