From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH net] sctp: change sk state only when it has assocs in sctp_shutdown Date: Mon, 14 Nov 2016 10:52:21 -0500 Message-ID: <20161114155221.GB15570@hmsreliant.think-freely.org> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: network dev , linux-sctp@vger.kernel.org, davem@davemloft.net, Marcelo Ricardo Leitner , Vlad Yasevich , andreyknvl@google.com To: Xin Long Return-path: Received: from charlotte.tuxdriver.com ([70.61.120.58]:42592 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753205AbcKNPws (ORCPT ); Mon, 14 Nov 2016 10:52:48 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Sun, Nov 13, 2016 at 09:44:37PM +0800, Xin Long wrote: > Now when users shutdown a sock with SEND_SHUTDOWN in sctp, even if > this sock has no connection (assoc), sk state would be changed to > SCTP_SS_CLOSING, which is not as we expect. > > Besides, after that if users try to listen on this sock, kernel > could even panic when it dereference sctp_sk(sk)->bind_hash in > sctp_inet_listen, as bind_hash is null when sock has no assoc. > > This patch is to move sk state change after checking sk assocs > is not empty, and also merge these two if() conditions and reduce > indent level. > > Fixes: d46e416c11c8 ("sctp: sctp should change socket state when shutdown is received") > Reported-by: Andrey Konovalov > Tested-by: Andrey Konovalov > Signed-off-by: Xin Long > --- > net/sctp/socket.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index faa48ff..f23ad91 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -4285,19 +4285,18 @@ static void sctp_shutdown(struct sock *sk, int how) > { > struct net *net = sock_net(sk); > struct sctp_endpoint *ep; > - struct sctp_association *asoc; > > if (!sctp_style(sk, TCP)) > return; > > - if (how & SEND_SHUTDOWN) { > + ep = sctp_sk(sk)->ep; > + if (how & SEND_SHUTDOWN && !list_empty(&ep->asocs)) { > + struct sctp_association *asoc; > + > sk->sk_state = SCTP_SS_CLOSING; > - ep = sctp_sk(sk)->ep; > - if (!list_empty(&ep->asocs)) { > - asoc = list_entry(ep->asocs.next, > - struct sctp_association, asocs); > - sctp_primitive_SHUTDOWN(net, asoc, NULL); > - } > + asoc = list_entry(ep->asocs.next, > + struct sctp_association, asocs); > + sctp_primitive_SHUTDOWN(net, asoc, NULL); > } > } > > -- > 2.1.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Acked-by: Neil Horman From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Date: Mon, 14 Nov 2016 15:52:21 +0000 Subject: Re: [PATCH net] sctp: change sk state only when it has assocs in sctp_shutdown Message-Id: <20161114155221.GB15570@hmsreliant.think-freely.org> List-Id: References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Xin Long Cc: network dev , linux-sctp@vger.kernel.org, davem@davemloft.net, Marcelo Ricardo Leitner , Vlad Yasevich , andreyknvl@google.com On Sun, Nov 13, 2016 at 09:44:37PM +0800, Xin Long wrote: > Now when users shutdown a sock with SEND_SHUTDOWN in sctp, even if > this sock has no connection (assoc), sk state would be changed to > SCTP_SS_CLOSING, which is not as we expect. > > Besides, after that if users try to listen on this sock, kernel > could even panic when it dereference sctp_sk(sk)->bind_hash in > sctp_inet_listen, as bind_hash is null when sock has no assoc. > > This patch is to move sk state change after checking sk assocs > is not empty, and also merge these two if() conditions and reduce > indent level. > > Fixes: d46e416c11c8 ("sctp: sctp should change socket state when shutdown is received") > Reported-by: Andrey Konovalov > Tested-by: Andrey Konovalov > Signed-off-by: Xin Long > --- > net/sctp/socket.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index faa48ff..f23ad91 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -4285,19 +4285,18 @@ static void sctp_shutdown(struct sock *sk, int how) > { > struct net *net = sock_net(sk); > struct sctp_endpoint *ep; > - struct sctp_association *asoc; > > if (!sctp_style(sk, TCP)) > return; > > - if (how & SEND_SHUTDOWN) { > + ep = sctp_sk(sk)->ep; > + if (how & SEND_SHUTDOWN && !list_empty(&ep->asocs)) { > + struct sctp_association *asoc; > + > sk->sk_state = SCTP_SS_CLOSING; > - ep = sctp_sk(sk)->ep; > - if (!list_empty(&ep->asocs)) { > - asoc = list_entry(ep->asocs.next, > - struct sctp_association, asocs); > - sctp_primitive_SHUTDOWN(net, asoc, NULL); > - } > + asoc = list_entry(ep->asocs.next, > + struct sctp_association, asocs); > + sctp_primitive_SHUTDOWN(net, asoc, NULL); > } > } > > -- > 2.1.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Acked-by: Neil Horman