From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 462F3C43381 for ; Wed, 6 Mar 2019 18:21:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 2832220684 for ; Wed, 6 Mar 2019 18:21:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730820AbfCFSVo (ORCPT ); Wed, 6 Mar 2019 13:21:44 -0500 Received: from charlotte.tuxdriver.com ([70.61.120.58]:49316 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726729AbfCFSVn (ORCPT ); Wed, 6 Mar 2019 13:21:43 -0500 Received: from [216.85.170.145] (helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1h1bAb-000355-5g; Wed, 06 Mar 2019 13:21:36 -0500 Date: Wed, 6 Mar 2019 13:21:27 -0500 From: Neil Horman To: Xin Long Cc: network dev , linux-sctp@vger.kernel.org, davem@davemloft.net, Marcelo Ricardo Leitner Subject: Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails Message-ID: <20190306182127.GB10683@neilslaptop.think-freely.org> References: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote: > It should fail to create the new sk if sctp_bind_addr_dup() fails > when accepting or peeloff an association. > > Signed-off-by: Xin Long > --- > net/sctp/socket.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index a2771b3..22adb8d 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc, > struct sctp_chunk *chunk); > static int sctp_do_bind(struct sock *, union sctp_addr *, int); > static int sctp_autobind(struct sock *sk); > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > - struct sctp_association *assoc, > - enum sctp_socket_type type); > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > + struct sctp_association *assoc, > + enum sctp_socket_type type); > > static unsigned long sctp_memory_pressure; > static atomic_long_t sctp_memory_allocated; > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern) > /* Populate the fields of the newsk from the oldsk and migrate the > * asoc to the newsk. > */ > - sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP); > + error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP); > + if (error) { > + sk_common_release(newsk); sctp_sock_migrate may fail after the pending packets have been moved from the old socket to the new socket. Normally those packets will get purged by successful transmission, or when the socket is closed (via sctp_close), but neither of those cases applies here. Whats going to dequeue and free any pending skbs on the sk_receive_queue here? > + newsk = NULL; > + } > > out: > release_sock(sk); > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp) > /* Populate the fields of the newsk from the oldsk and migrate the > * asoc to the newsk. > */ > - sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH); > + err = sctp_sock_migrate(sk, sock->sk, asoc, > + SCTP_SOCKET_UDP_HIGH_BANDWIDTH); > + if (err) { > + sock_release(sock); Same question here, what frees any pending skbs on the new socket, if the migration fails after the skbs have been queued to it? > + sock = NULL; > + } > > *sockp = sock; > > @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to, > /* Populate the fields of the newsk from the oldsk and migrate the assoc > * and its messages to the newsk. > */ > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > - struct sctp_association *assoc, > - enum sctp_socket_type type) > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > + struct sctp_association *assoc, > + enum sctp_socket_type type) > { > struct sctp_sock *oldsp = sctp_sk(oldsk); > struct sctp_sock *newsp = sctp_sk(newsk); > @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > struct sk_buff *skb, *tmp; > struct sctp_ulpevent *event; > struct sctp_bind_hashbucket *head; > + int err; > > /* Migrate socket buffer sizes and all the socket level options to the > * new socket. > @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > /* Copy the bind_addr list from the original endpoint to the new > * endpoint so that we can handle restarts properly > */ > - sctp_bind_addr_dup(&newsp->ep->base.bind_addr, > - &oldsp->ep->base.bind_addr, GFP_KERNEL); > + err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr, > + &oldsp->ep->base.bind_addr, GFP_KERNEL); > + if (err) > + return err; > > /* Move any messages in the old socket's receive queue that are for the > * peeled off association to the new socket's receive queue. > @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > } > > release_sock(newsk); > + > + return 0; > } > > > -- > 2.1.0 > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Date: Wed, 06 Mar 2019 18:21:27 +0000 Subject: Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails Message-Id: <20190306182127.GB10683@neilslaptop.think-freely.org> List-Id: References: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> In-Reply-To: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> 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 On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote: > It should fail to create the new sk if sctp_bind_addr_dup() fails > when accepting or peeloff an association. > > Signed-off-by: Xin Long > --- > net/sctp/socket.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index a2771b3..22adb8d 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc, > struct sctp_chunk *chunk); > static int sctp_do_bind(struct sock *, union sctp_addr *, int); > static int sctp_autobind(struct sock *sk); > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > - struct sctp_association *assoc, > - enum sctp_socket_type type); > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > + struct sctp_association *assoc, > + enum sctp_socket_type type); > > static unsigned long sctp_memory_pressure; > static atomic_long_t sctp_memory_allocated; > @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern) > /* Populate the fields of the newsk from the oldsk and migrate the > * asoc to the newsk. > */ > - sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP); > + error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP); > + if (error) { > + sk_common_release(newsk); sctp_sock_migrate may fail after the pending packets have been moved from the old socket to the new socket. Normally those packets will get purged by successful transmission, or when the socket is closed (via sctp_close), but neither of those cases applies here. Whats going to dequeue and free any pending skbs on the sk_receive_queue here? > + newsk = NULL; > + } > > out: > release_sock(sk); > @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp) > /* Populate the fields of the newsk from the oldsk and migrate the > * asoc to the newsk. > */ > - sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH); > + err = sctp_sock_migrate(sk, sock->sk, asoc, > + SCTP_SOCKET_UDP_HIGH_BANDWIDTH); > + if (err) { > + sock_release(sock); Same question here, what frees any pending skbs on the new socket, if the migration fails after the skbs have been queued to it? > + sock = NULL; > + } > > *sockp = sock; > > @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to, > /* Populate the fields of the newsk from the oldsk and migrate the assoc > * and its messages to the newsk. > */ > -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > - struct sctp_association *assoc, > - enum sctp_socket_type type) > +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > + struct sctp_association *assoc, > + enum sctp_socket_type type) > { > struct sctp_sock *oldsp = sctp_sk(oldsk); > struct sctp_sock *newsp = sctp_sk(newsk); > @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > struct sk_buff *skb, *tmp; > struct sctp_ulpevent *event; > struct sctp_bind_hashbucket *head; > + int err; > > /* Migrate socket buffer sizes and all the socket level options to the > * new socket. > @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > /* Copy the bind_addr list from the original endpoint to the new > * endpoint so that we can handle restarts properly > */ > - sctp_bind_addr_dup(&newsp->ep->base.bind_addr, > - &oldsp->ep->base.bind_addr, GFP_KERNEL); > + err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr, > + &oldsp->ep->base.bind_addr, GFP_KERNEL); > + if (err) > + return err; > > /* Move any messages in the old socket's receive queue that are for the > * peeled off association to the new socket's receive queue. > @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk, > } > > release_sock(newsk); > + > + return 0; > } > > > -- > 2.1.0 > >