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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 15 Jan 2020 06:16:34 -0800 From: Jakub Kicinski <kuba@...nel.org> To: Taehee Yoo <ap420073@...il.com> Cc: Jakub Kicinski <jakub.kicinski@...ronome.com>, David Miller <davem@...emloft.net>, Netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...nulli.us> Subject: Re: [PATCH net 1/5] netdevsim: fix a race condition in netdevsim operations On Wed, 15 Jan 2020 00:26:22 +0900, Taehee Yoo wrote: > On Sun, 12 Jan 2020 at 23:19, Jakub Kicinski wrote: > Hi Jakub, > Thank you for your review! Thank you for fixing these tricky bugs! :) > > Perhaps the entire bus dev structure should be freed from release? > > I tested this like this. > > @@ -146,6 +161,8 @@ static void nsim_bus_dev_release(struct device *dev) > struct nsim_bus_dev *nsim_bus_dev = to_nsim_bus_dev(dev); > > nsim_bus_dev_vfs_disable(nsim_bus_dev); > + ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id); > + kfree(nsim_bus_dev); > } > @@ -300,8 +320,6 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count) > static void nsim_bus_dev_del(struct nsim_bus_dev *nsim_bus_dev) > { > device_unregister(&nsim_bus_dev->dev); > - ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id); > - kfree(nsim_bus_dev); > } > > It works well. but I'm not sure this is needed. My concern is that process A opens a sysfs file (eg. numvfs) then process B deletes the device, but process A still has a file descriptor (and device reference) so it may be able to write/read the numvfs file even though nsim_bus_dev was already freed. I may very well be wrong, and something else may be preventing this condition. It's just a bit strange to see release free an internal sub-structure, while the main structure is freed immediately.. > > > unsigned int num_vfs; > > > int ret; > > > > > > + if (!mutex_trylock(&nsim_bus_dev_ops_lock)) > > > + return -EBUSY; > > > > Why the trylocks? Are you trying to catch the races between unregister > > and other ops? > > > > The reason is to avoid deadlock. > If we use mutex_lock() instead of mutex_trylock(), the below message > will be printed and actual deadlock also appeared. > [ 426.907883][ T805] Possible unsafe locking scenario: > [ 426.907883][ T805] > [ 426.908715][ T805] CPU0 CPU1 > [ 426.909312][ T805] ---- ---- > [ 426.909902][ T805] lock(kn->count#170); > [ 426.910372][ T805] > lock(nsim_bus_dev_ops_lock); > [ 426.911277][ T805] lock(kn->count#170); > [ 426.912032][ T805] lock(nsim_bus_dev_ops_lock); > Locking ordering of {new/del}_device() and {new/del}_port is different. > So, a deadlock could occur. Hm, we can't use the same lock for the bus ops and port ops. But the port ops already take port lock, do we really need another lock there? Also does nsim_bus_exit() really need to iterate over devices to remove them? Does core not do it for us?
Powered by blists - more mailing lists