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 DB296C43381 for ; Thu, 7 Mar 2019 11:59:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B33C02081B for ; Thu, 7 Mar 2019 11:59:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726428AbfCGL7x (ORCPT ); Thu, 7 Mar 2019 06:59:53 -0500 Received: from charlotte.tuxdriver.com ([70.61.120.58]:57383 "EHLO smtp.tuxdriver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726101AbfCGL7x (ORCPT ); Thu, 7 Mar 2019 06:59:53 -0500 Received: from cpe-2606-a000-111b-405a-9816-2c85-c514-8f7a.dyn6.twc.com ([2606:a000:111b:405a:9816:2c85:c514:8f7a] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1h1rgX-0007fo-Ez; Thu, 07 Mar 2019 06:59:45 -0500 Date: Thu, 7 Mar 2019 06:59:08 -0500 From: Neil Horman To: Xin Long Cc: network dev , linux-sctp@vger.kernel.org, davem , Marcelo Ricardo Leitner Subject: Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails Message-ID: <20190307115908.GA8030@hmswarspite.think-freely.org> References: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> <20190306182127.GB10683@neilslaptop.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.11.3 (2019-02-01) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Thu, Mar 07, 2019 at 06:06:21PM +0800, Xin Long wrote: > On Thu, Mar 7, 2019 at 2:21 AM Neil Horman wrote: > > > > 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? > Luckily, in sctp_sock_migrate(), it can only fail before the skbs are > moved to newsk->sk_receive_queue. So when it returns err, > newsk->sk_receive_queue must be empty. > Yeah, you're right, I misread the location of the check, thinking it came after the skb migration, this is fine. > > > > > + 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 > > > > > > >