Hi Partha, Forwarding this to you, since I misspelled your name in the original mail.
BR ///jon -----Original Message----- From: Ying Xue <[email protected]> Sent: September 26, 2018 8:39 AM To: Jon Maloy <[email protected]>; Jon Maloy <[email protected]> Cc: Mohan Krishna Ghanta Krishnamurthy <[email protected]>; [email protected]; Tung Quang Nguyen <[email protected]>; Hoang Huu Le <[email protected]>; Canh Duc Luu <[email protected]>; Gordan Mihaljevic <[email protected]>; [email protected]; [email protected] Subject: Re: [net 1/1] tipc: queue socket protocol error messages into socket receive buffer On 09/26/2018 03:14 AM, Jon Maloy wrote: > From: Parthasarathy Bhuvaragan <[email protected]> > > In tipc_sk_filter_rcv(), when we detect protocol messages with error > we call tipc_sk_conn_proto_rcv() and let it reset the connection and > notify the socket by calling sk->sk_state_change(). > > However, tipc_sk_filter_rcv() may have been called from the function > tipc_backlog_rcv(), in which case the socket lock is held and the > socket already awake. This means that the sk_state_change() call is > ignored and the error notification lost. Now the receive queue will > remain empty and the socket sleeps forever. > > In this commit, we convert the protocol message into a connection > abort message and enqueue it into the socket's receive queue. By this > addition to the above state change we cover all conditions. > I am not 100% sure whether I completely understand the deadlock scenario, but in my opinion, we don't need to add an abort message into receive queue. Instead, we should set an error code to sk->sk_err before sk->sk_state_change(sk) in tipc_sk_conn_proto_rcv(), which can avoid this issue. Even if the error notification is lost, when user tries to receive message from the socket, it's found the socket is not in a proper status with sock_error(sk). As a consequence, the socket receive function will immediately return. No any socket will sleep forever. > Signed-off-by: Parthasarathy Bhuvaragan > <[email protected]> > Signed-off-by: Jon Maloy <[email protected]> > --- > net/tipc/socket.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/net/tipc/socket.c b/net/tipc/socket.c index > b6f99b0..49810fd 100644 > --- a/net/tipc/socket.c > +++ b/net/tipc/socket.c > @@ -1196,6 +1196,7 @@ void tipc_sk_mcast_rcv(struct net *net, struct > sk_buff_head *arrvq, > * @skb: pointer to message buffer. > */ > static void tipc_sk_conn_proto_rcv(struct tipc_sock *tsk, struct > sk_buff *skb, > + struct sk_buff_head *inputq, > struct sk_buff_head *xmitq) > { > struct tipc_msg *hdr = buf_msg(skb); @@ -1213,7 +1214,16 @@ static > void tipc_sk_conn_proto_rcv(struct tipc_sock *tsk, struct sk_buff *skb, > tipc_node_remove_conn(sock_net(sk), tsk_peer_node(tsk), > tsk_peer_port(tsk)); In other words, we should set an error code to sk->sk_err here: For example: sk->sk_err = EINVAL; > sk->sk_state_change(sk); If I misunderstood the scenario, please help correct me. Thanks, Ying > - goto exit; > + > + /* State change is ignored if socket already awake, > + * - convert msg to abort msg and add to inqueue > + */ > + msg_set_user(hdr, TIPC_CRITICAL_IMPORTANCE); > + msg_set_type(hdr, TIPC_CONN_MSG); > + msg_set_size(hdr, BASIC_H_SIZE); > + msg_set_hdr_sz(hdr, BASIC_H_SIZE); > + __skb_queue_tail(inputq, skb); > + return; > } > > tsk->probe_unacked = false; > @@ -1936,7 +1946,7 @@ static void tipc_sk_proto_rcv(struct sock *sk, > > switch (msg_user(hdr)) { > case CONN_MANAGER: > - tipc_sk_conn_proto_rcv(tsk, skb, xmitq); > + tipc_sk_conn_proto_rcv(tsk, skb, inputq, xmitq); > return; > case SOCK_WAKEUP: > tipc_dest_del(&tsk->cong_links, msg_orignode(hdr), 0); > _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
