Re: [RFC PATCH] kcm: hold rx mux lock when updating the receive queue.

[Paolo Abeni <pabeni@redhat.com>]

Hi,

On Wed, 2018-06-06 at 13:25 +0300, Kirill Tkhai wrote:
> Hi, Paolo,
> 
> below is couple my thoughts about this.
> 
> On 06.06.2018 12:44, Paolo Abeni wrote:
> > On Tue, 2018-06-05 at 18:06 +0200, Paolo Abeni wrote:
> > > On Tue, 2018-06-05 at 08:35 -0700, Tom Herbert wrote:
> > > > Paolo, thanks for looking into this! Can you try replacing
> > > > __skb_dequeue in requeue_rx_msgs with skb_dequeue to see if that is
> > > > the fix.
> > > 
> > > Sure, I'll retrigger the test, and report the result here (or directly
> > > a new patch, should the test be succesful)
> > 
> > Contrary to my expectations, the suggested change does not fix the
> > issue. I'm still investigating the overall locking schema.
> 
> kcm_rcv_strparser()->unreserve_rx_kcm()->requeue_rx_msgs()->__skb_dequeue()
> 
> seems needed to be synchronized with:
> 
> kcm_recvmsg()->kcm_wait_data().
> 
> Otherwise, requeue_rx_msgs() removes kcm_recvmsg() peeked skb.
> 
> The solution could be to take lock_sock(&kcm->sk) in requeue_rx_msgs(), but
> we can't do that since there is already locked another socket (and potentially,
> this may be a reason of deadlock).
> 
> The approach you made in initial patch seems good for me to solve this problem.
> The only thing I'm not sure is either lock_sock() is needed in kcm_recvmsg() after
> this.

Thank you for the feedback!

I tried a different approach (add en explicit 'peek' argument to
kcm_wait_data, and dequeue the packet there if not explicitly asked
otherwise). It solves the issue and looks reasonably clean.

I'll post the patch soon.

Cheers,

Paolo