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
| ||
|
Date: Mon, 10 Jun 2019 11:05:41 +0300 From: Ilya Maximets <i.maximets@...sung.com> To: Jakub Kicinski <jakub.kicinski@...ronome.com> Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, bpf@...r.kernel.org, xdp-newbies@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Björn Töpel <bjorn.topel@...el.com>, Magnus Karlsson <magnus.karlsson@...el.com>, Jonathan Lemon <jonathan.lemon@...il.com> Subject: Re: [PATCH bpf v2] xdp: fix hang while unregistering device bound to xdp socket On 08.06.2019 2:31, Jakub Kicinski wrote: > On Fri, 7 Jun 2019 20:31:43 +0300, Ilya Maximets wrote: >> +static int xsk_notifier(struct notifier_block *this, >> + unsigned long msg, void *ptr) >> +{ >> + struct sock *sk; >> + struct net_device *dev = netdev_notifier_info_to_dev(ptr); >> + struct net *net = dev_net(dev); >> + int i, unregister_count = 0; > > Please order the var declaration lines longest to shortest. > (reverse christmas tree) Hi. I'm not a fan of mixing 'struct's with bare types in the declarations. Moving the 'sk' to the third place will make a hole like this: struct net_device *dev = netdev_notifier_info_to_dev(ptr); struct net *net = dev_net(dev); struct sock *sk; int i, unregister_count = 0; Which is not looking good. Moving to the 4th place: struct net_device *dev = netdev_notifier_info_to_dev(ptr); struct net *net = dev_net(dev); int i, unregister_count = 0; struct sock *sk; This variant doesn't look good for me because of mixing 'struct's with bare integers. Do you think I need to use one of above variants? > >> + mutex_lock(&net->xdp.lock); >> + sk_for_each(sk, &net->xdp.list) { >> + struct xdp_sock *xs = xdp_sk(sk); >> + >> + mutex_lock(&xs->mutex); >> + switch (msg) { >> + case NETDEV_UNREGISTER: > > You should probably check the msg type earlier and not take all the > locks and iterate for other types.. Yeah. I thought about it too. Will fix in the next version. Best regards, Ilya Maximets.
Powered by blists - more mailing lists