On 10/07/20(Fri) 14:38, Vitaliy Makkoveev wrote: > On Fri, Jul 10, 2020 at 01:22:40PM +0200, Martin Pieuchot wrote: > > On 10/07/20(Fri) 14:07, Vitaliy Makkoveev wrote: > > > We have some races in pppac(4) > > > 1. malloc(9) can sleep so we must check `sc' presence after malloc(9) > > > > Makes sense. > > > > > 2. we can sleep between `sc' insertion to `sc_entry' list and > > > `sc_pipex_iface' initialization. Concurrent pppacioctl() can touch > > > this incomplete `sc'. > > > > Why not insert the descriptor at the end? Shouldn't the order of > > operations be: > > > > pipex_iface_init(); > > if_attach(); > > LIST_INSERT_HEAD() > > > > This way there's no need for a `ready' flag since the descriptor is only > > added to global data structures once it is completely initialized. > > > > Using a `sc_ready' or `sc_dead' approach is something that require > > touching all drivers whereas serializing insertions to global data > > structures can be done at once for all the kernel. > > No, because we introduce the races with if_attach(). The similar races > are in if_clone_attach(). We can do multiple `ifp' attachment with the > same name.
Yes that's the same problem. It is also present in other parts of the userland/network stack boundary. That's why I'm arguing that the best approach is to use a lock and document which data structures it protects. We should concentrate on protecting access to data structures and not code paths.
