Re: [tipc-discussion] [PATCH v1 1/1] tipc: fix race condition causing hung sendto

[Jon M. <jon...@er...>]

Acked-by: Jon Maloy <jon...@er...>

///jon


> -----Original Message-----
> From: Tung Nguyen <tun...@de...>
> Sent: 14-Feb-19 01:42
> To: tip...@li...; Jon Maloy
> <jon...@er...>; ma...@do...; yin...@wi...
> Subject: [tipc-discussion][PATCH v1 1/1] tipc: fix race condition causing hung
> sendto
> 
> When sending multicast messages via blocking socket, if sending link is
> congested (tsk->cong_link_cnt is set to 1), the sending thread is put into
> sleeping state. However,
> tipc_sk_filter_rcv() is called under socket spin lock but
> tipc_wait_for_cond() is not. So, there is no guarantee that the setting of tsk-
> >cong_link_cnt to 0 in tipc_sk_proto_rcv() in
> CPU-1 will be perceived by CPU-0. If that is the case, the sending thread in
> CPU-0 after being waken up, will continue to see
> tsk->cong_link_cnt as 1 and put the sending thread into sleeping
> state again. The sending thread will sleep forever.
> 
> CPU-0                                | CPU-1
> tipc_wait_for_cond()                 |
> {                                    |
>  // condition_ = !tsk->cong_link_cnt |
>  while ((rc_ = !(condition_))) {     |
>   ...                                |
>   release_sock(sk_);                 |
>   wait_woken();                      |
>                                      | if (!sock_owned_by_user(sk))
>                                      |  tipc_sk_filter_rcv()
>                                      |  {
>                                      |   ...
>                                      |   tipc_sk_proto_rcv()
>                                      |   {
>                                      |    ...
>                                      |    tsk->cong_link_cnt--;
>                                      |    ...
>                                      |    sk->sk_write_space(sk);
>                                      |    ...
>                                      |   }
>                                      |   ...
>                                      |  }
>   sched_annotate_sleep();            |
>   lock_sock(sk_);                    |
>   remove_wait_queue();               |
>  }                                   |
> }                                    |
> 
> This commit fixes it by adding memory barrier to tipc_sk_proto_rcv() and
> tipc_wait_for_cond().
> 
> Signed-off-by: Tung Nguyen <tun...@de...>
> ---
>  net/tipc/socket.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c index
> 1217c90a363b..d8f054d45941 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -383,6 +383,8 @@ static int tipc_sk_sock_err(struct socket *sock, long
> *timeout)
>  	int rc_;							       \
>  									       \
>  	while ((rc_ = !(condition_))) {					       \
> +		/* coupled with smp_wmb() in tipc_sk_proto_rcv() */            \
> +		smp_rmb();                                                     \
>  		DEFINE_WAIT_FUNC(wait_, woken_wake_function);
> \
>  		sk_ = (sock_)->sk;					       \
>  		rc_ = tipc_sk_sock_err((sock_), timeo_);		       \
> @@ -1982,6 +1984,8 @@ static void tipc_sk_proto_rcv(struct sock *sk,
>  		return;
>  	case SOCK_WAKEUP:
>  		tipc_dest_del(&tsk->cong_links, msg_orignode(hdr), 0);
> +		/* coupled with smp_rmb() in tipc_wait_for_cond() */
> +		smp_wmb();
>  		tsk->cong_link_cnt--;
>  		wakeup = true;
>  		break;
> --
> 2.17.1