Re: [Patch net v2] kcm: close race conditions on sk_receive_queue

[Cong Wang <xiyou.wangcong@gmail.com>]

On Tue, Nov 08, 2022 at 10:13:23AM +0100, Paolo Abeni wrote:
> Hello,
> On Thu, 2022-11-03 at 11:46 -0700, Cong Wang wrote:
> > @@ -1085,53 +1085,17 @@ static int kcm_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> >  	return err;
> >  }
> >  
> > -static struct sk_buff *kcm_wait_data(struct sock *sk, int flags,
> > -				     long timeo, int *err)
> > -{
> > -	struct sk_buff *skb;
> > -
> > -	while (!(skb = skb_peek(&sk->sk_receive_queue))) {
> > -		if (sk->sk_err) {
> > -			*err = sock_error(sk);
> > -			return NULL;
> > -		}
> > -
> > -		if (sock_flag(sk, SOCK_DONE))
> > -			return NULL;
> 
> It looks like skb_recv_datagram() ignores the SOCK_DONE flag, so this
> change could potentially miss some wait_data end coditions. On the flip
> side I don't see any place where the SOCK_DONE flag is set for the kcm
> socket, so this should be safe, but could you please document this in
> the commit message?

Good catch. Indeed, this flags seems only used by stream sockets, I will
include this in the commit message.

> 
> [...]
> 
> > @@ -1187,11 +1147,7 @@ static ssize_t kcm_splice_read(struct socket *sock, loff_t *ppos,
> >  
> >  	/* Only support splice for SOCKSEQPACKET */
> >  
> > -	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> > -
> > -	lock_sock(sk);
> > -
> > -	skb = kcm_wait_data(sk, flags, timeo, &err);
> > +	skb = skb_recv_datagram(sk, flags, &err);
> >  	if (!skb)
> >  		goto err_out;
> >  
> > @@ -1219,13 +1175,11 @@ static ssize_t kcm_splice_read(struct socket *sock, loff_t *ppos,
> >  	 * finish reading the message.
> >  	 */
> >  
> > -	release_sock(sk);
> > -
> > +	skb_free_datagram(sk, skb);
> >  	return copied;
> >  
> >  err_out:
> > -	release_sock(sk);
> > -
> > +	skb_free_datagram(sk, skb);
> 
> We can reach here with skb == NULL and skb_free_datagram() ->
> __kfree_skb() -> skb_release_all() does not deal correctly with NULL
> skb, you need to check for skb explicitly here (or rearrange the error
> paths in a suitable way).
> 

Are you sure? skb_free_datagram() is just consume_skb() which is guarded
by skb_unref() which takes NULL and returns false.


1195 static inline bool skb_unref(struct sk_buff *skb)
1196 {
1197         if (unlikely(!skb))
1198                 return false;

1027 void consume_skb(struct sk_buff *skb)
1028 {
1029         if (!skb_unref(skb))
1030                 return;

320 void skb_free_datagram(struct sock *sk, struct sk_buff *skb)
321 {
322         consume_skb(skb);
323 }


Thanks.