On 28/05/20(Thu) 15:27, Vitaliy Makkoveev wrote: > On Thu, May 28, 2020 at 12:26:39PM +0200, Martin Pieuchot wrote: > > On 27/05/20(Wed) 11:54, Vitaliy Makkoveev wrote: > > > pipex(4) is simultaneously protected by KERNEL_LOCK() and NET_LOCK() and > > > the last is not required. I guess to start remove NET_LOCK(). Diff below > > > drops NET_LOCK() in pipex_ioctl() and underlaying routines. At least > > > this helps to kill unlock/lock mess in pppx_add_session() and > > > pppx_if_destroy(). > > > > Getting rid of the NET_LOCK() might indeed help to untangle the current > > locking mess. However we should aim towards removing the KERNEL_LOCK() > > from, at least, the packet processing path starting with pipexintr(). > > I guess we can made `pipexoutq' processing NET_LOCK() free. Also > `pipexinq' processing requires a little amount of places where NET_LOCK() > is required. So we can implement special locks for pipex(4).
After second though, it seems that the easiest way forward is to protect `pipex_session_list' by the NET_LOCK(). The rational is that this global list is dereferenced in pipexintr() and its members are compared to the content of `ph_cookie'. There's currently no mechanism to ensure the reference saved in `ph_cookie' is still valid. That means what ensures the pointer is kind-of correct is the NET_LOCK(). I'm saying "kind-of" because comparing pointers is questionable, especially if sessions can be destroyed without purging `pipexoutq' or `pipexinq'. Since the KERNEL_LOCK() is not always held when the network stack runs, `ph_cookie' can be modified by a CPU without holding it. So what actually protects the data structures here is the NET_LOCK(). Does that make sense?
