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

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

[Paolo Abeni]

Currently kcm holds both the RX mux lock and the socket lock when
updating the sk receive queue, except in some notable cases:

- kcm_rfree holds only the RX mux lock
- kcm_recvmsg holds only the socket lock

has results there are possible races which cause receive queue
corruption, as reported by the syzbot.

Since we can't acquire the socket lock in kcm_rfree, let's use
the RX mux lock to protect the receive queue update in kcm_recvmsg,
too. Also, let's add some commit noting which is the locking schema in use.

Fixes: ab7ac4eb9832 ("kcm: Kernel Connection Multiplexor module")
Reported-and-tested-by: syzbot+278279efdd2730dd14bf@syzkaller.appspotmail.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
This is an RFC, since I'm really new to this area, anyway the syzport
reported success in testing the proposed fix.
This is very likely a scenario where the hopefully upcoming 
skb->prev,next->list_head conversion would have helped a lot, thanks to 
list poisoning and list debug
---
 net/kcm/kcmsock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index d3601d421571..95e1d95ab24a 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -188,6 +188,7 @@ static void kcm_rfree(struct sk_buff *skb)
 	}
 }
 
+/* RX mux lock held */
 static int kcm_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
 	struct sk_buff_head *list = &sk->sk_receive_queue;
@@ -1157,7 +1158,9 @@ static int kcm_recvmsg(struct socket *sock, struct msghdr *msg,
 			/* Finished with message */
 			msg->msg_flags |= MSG_EOR;
 			KCM_STATS_INCR(kcm->stats.rx_msgs);
+			spin_lock_bh(&kcm->mux->rx_lock);
 			skb_unlink(skb, &sk->sk_receive_queue);
+			spin_unlock_bh(&kcm->mux->rx_lock);
 			kfree_skb(skb);
 		}
 	}
-- 
2.17.1

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

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue,  5 Jun 2018 12:32:33 +0200

> @@ -1157,7 +1158,9 @@ static int kcm_recvmsg(struct socket *sock, struct msghdr *msg,
>  			/* Finished with message */
>  			msg->msg_flags |= MSG_EOR;
>  			KCM_STATS_INCR(kcm->stats.rx_msgs);
> +			spin_lock_bh(&kcm->mux->rx_lock);
>  			skb_unlink(skb, &sk->sk_receive_queue);
> +			spin_unlock_bh(&kcm->mux->rx_lock);

Hmmm, maybe I don't understand the corruption.

But, skb_unlink() takes the sk->sk_receive_queue.lock which should
prevent SKB list corruption.

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

[Paolo Abeni]

Hi,

On Tue, 2018-06-05 at 08:35 -0700, Tom Herbert wrote:
> On Tue, Jun 5, 2018 at 7:53 AM, David Miller <davem@davemloft.net> wrote:
> > From: Paolo Abeni <pabeni@redhat.com>
> > Date: Tue,  5 Jun 2018 12:32:33 +0200
> >
> >> @@ -1157,7 +1158,9 @@ static int kcm_recvmsg(struct socket *sock, struct msghdr *msg,
> >>                       /* Finished with message */
> >>                       msg->msg_flags |= MSG_EOR;
> >>                       KCM_STATS_INCR(kcm->stats.rx_msgs);
> >> +                     spin_lock_bh(&kcm->mux->rx_lock);
> >>                       skb_unlink(skb, &sk->sk_receive_queue);
> >> +                     spin_unlock_bh(&kcm->mux->rx_lock);
> >
> > Hmmm, maybe I don't understand the corruption.
> >
> > But, skb_unlink() takes the sk->sk_receive_queue.lock which should
> > prevent SKB list corruption.
> 
> It looks like there is a case where the list is being manipulated
> without the queue lock. That is in requeue_rx_msgs where
> __skb_dequeue is being called instead of skb_dequeue which is in
> requeue_rx_msgs. requeue_rx_msgs holds the mux rx_lock which would
> explain why the suggested patch avoids the issue.

Yep, I belive this is the correct explanation. Sorry for the noise with
the previous patch, I underlooked the skb_queue lock already in place.

> 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)

Thanks,

Paolo

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

[Paolo Abeni]

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.

Cheers,

Paolo

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

[Kirill Tkhai]

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.

Thanks,
Kirill

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

[Paolo Abeni]

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