linux-kernel - Re: [PATCH 1/2] can: bcm: registration process optimization in bcm_module

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220912120020.dlxuryltw4sii635@pengutronix.de>
Date:   Mon, 12 Sep 2022 14:00:20 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Oliver Hartkopp <socketcan@...tkopp.net>
Cc:     "Ziyang Xuan (William)" <william.xuanziyang@...wei.com>,
        edumazet@...gle.com, kuba@...nel.org, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] can: bcm: registration process optimization in
 bcm_module_init()

On 09.09.2022 17:04:06, Oliver Hartkopp wrote:
> 
> 
> On 09.09.22 05:58, Ziyang Xuan (William) wrote:
> > > 
> > > 
> > > On 9/8/22 13:14, Ziyang Xuan (William) wrote:
> > > > > 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;
> > > > > }
> > > > > 
> 
> > Yes，all these socket operations need time, most likely, register_netdevice_notifier() and register_pernet_subsys() had been done.
> > But it maybe not for some reasons, for example, cpu# that runs {raw,bcm}_module_init() is stuck temporary,
> > or pernet_ops_rwsem lock competition in register_netdevice_notifier() and register_pernet_subsys().
> > 
> > If the condition which I pointed happens, I think my solution can solve.
> > 
> 
> No, I don't think so.
> 
> We need to maintain the exact order which is depicted in the af_packet.c
> code from above as the notifier call references the sock pointer.

The notifier calls bcm_notifier() first, which will loop over the
bcm_notifier_list. The list is empty if there are no sockets open, yet.
So from my point of view this change looks fine.

IMHO it's better to make a series where all these notifiers are moved in
front of the respective socket proto_register().

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)