On 11/07/20(Sat) 23:51, Vitaliy Makkoveev wrote: > [...] > The way you suggest to go is to introduce rwlock and serialize > pppacopen() and pppacclose(). This is bad idea because we will sleep > while we are holding rwlock.
That's the whole point of a rwlock to be able to sleep while holding the lock. The goal is to prevent any other thread coming from userland to enter any code path leading to the same data structure. This is the same as what the KERNEL_LOCK() was supposed to do assuming there where no sleeping point in if_attach() and pipex_iface_init(). > Also this is bad idea because you should > prevent access to `sc' which is being destroyed because you can grab it > by concurrent thread. Which data structure is the other thread using to get a reference on `sc'? If the data structure is protected by the rwlock, like I suggest for ifunit(), there's no problem. If it is protected by the KERNEL_LOCK() then any sleeping point can lead to a race. That's why we're doing such changes. In case of a data structure protected by the KERNEL_LOCK() the easiest way to deal with sleeping point is to re-check when coming back to sleep. This works well if the sleeping point is not deep into another layer. Another way is to have a per-driver lock or serialization mechanism. > You must serialize *all* access to this `sc' > elsewhere your "protection" is useless. The question is not access to `sc' is access to which global data structure having a reference to this `sc'? If the data structure is common to all the network stack, like ifunit()'s then we should look a for solution for all the network stack with all the usages of the list. That includes many driver's *open() functions, the cloning ioctls, etc. If the data structure is per-driver then the locking/serialization mechanism is per-driver. > pppx(4) had no problems with unit protection. Also it had no problems > to access incomplete `pxi'. Now pppx(4) has fixed access to `pxi' which > is being destroyed. And this is the way to go in pppac(4) layer too. > > We have pppx_dev2pxd() to obtain `pxd'. While we adding extra check to > pppx_dev2pxd() this is not system wide. Also pppac(4) already has > `sc_dead' to prevent concurrent pppac_ioctl() access to dying `sc'. You > suggest to serialize pppac_ioctl() too? The way `sc_dead' is used in other drivers is a way to prevent per-driver ioctl(2) while a pseudo-device is being detached. It assumes the NET_LOCK() is held around pseuo-device ioctl(2) so the flag is protected (but not documented by the NET_LOCK(). If you want to do the same go for it, but please do not add another meaning to the same mechanism. Having drivers that work similarly reduces maintains effort.
