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=-6.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS 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 DFFACC43381 for ; Fri, 8 Mar 2019 03:48:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A5E1720675 for ; Fri, 8 Mar 2019 03:48:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="PB6/2SbW" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726298AbfCHDsZ (ORCPT ); Thu, 7 Mar 2019 22:48:25 -0500 Received: from mail-wm1-f68.google.com ([209.85.128.68]:36955 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726266AbfCHDsY (ORCPT ); Thu, 7 Mar 2019 22:48:24 -0500 Received: by mail-wm1-f68.google.com with SMTP id x10so11104958wmg.2; Thu, 07 Mar 2019 19:48:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jVzt7WwVjBXreuKyehxEHDfhnMTFfTq4Ek2JD+SNVXU=; b=PB6/2SbW+pkRFKwOZetaDqKbrD+vgG4WAdZ61cO3T0NhtBHJ+zPm5kX7aODTYp2pCo tuMjDxKyKhRWv2/mxSvsA7OzvzwqbfB76nuOA3F7QU6tEVNTnMVXPkfPoChNwnKcKjsI QQRpUo/2byPmvHYYo/yG9YhbL8LJPUUlKsXU80CYuDnPVSIjMWe2qpvHdsBeNFy0pqog lE0+Tgd5YhiG0vB1UYvSOCNcdHj6+YIROSJGNv0BuHo42teiabf1zSUvWM1NZ2cauLf+ HsIYijQke8SDVmNg7rlR1WHauQ6Nr92a1FA65JcR8Q2CCUtVmGFAuXdFsCQAOOWUHJIu N7Dw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jVzt7WwVjBXreuKyehxEHDfhnMTFfTq4Ek2JD+SNVXU=; b=N6tBA2JKrNsoaX7BMh/VxlMcCIqYXUlQpQTOhm+nrS9BhTw+/7JiAJA3WN/VOwqjyh z1z1olExg6rlANNlGzyho5dUT0nis6m/j5ao3OpfV+D3XhLF9e3dMBFVAbDA8Jv7WUub jfBTKcdY8WZDDPqwINd4CIY6jWWNHT0cFpCzDhdoUv7IZtIHxN4UHmPJxDTHpLGhhSxX efRTNo29WTkrByv4I+RUxlHV0a/H7Mr81lXalrEBgoOS5kachIL4eYprG7hXQ0TGP83b jIi3sQ6ITBPYsl2R8hKTsgaf6HlKmnpgsuTCj2zL9q0E5K26c7QSQR7I8pkG6rMaVxFI eyeA== X-Gm-Message-State: APjAAAXLPTize7ZZlOqBIwvT6UYzYaFGbjAOrxrASTsc7qd/ZUPI4y6g qedSb5HVKZ516Yr9dcuKNNTYuJb7k6uLwTw2i0o= X-Google-Smtp-Source: APXvYqxWov3QHtZUbZ7UPTRUwMDrzSYz/1mEpNNJomT+5pKZxsGrgeXG+n833niWXoe6BDhfhP9bAaQzAFFT+EJJ12w= X-Received: by 2002:a1c:9ed5:: with SMTP id h204mr7656230wme.140.1552016902508; Thu, 07 Mar 2019 19:48:22 -0800 (PST) MIME-Version: 1.0 References: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> <20190307182503.GB13515@localhost.localdomain> In-Reply-To: <20190307182503.GB13515@localhost.localdomain> From: Xin Long Date: Fri, 8 Mar 2019 11:48:10 +0800 Message-ID: Subject: Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails To: Marcelo Ricardo Leitner Cc: network dev , linux-sctp@vger.kernel.org, davem , Neil Horman Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Mar 8, 2019 at 2:25 AM Marcelo Ricardo Leitner 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); > > + 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); > > I'm having a hard time understanding why it needs sock_release() here > and sk_common_release() in the above chunk. AFAICT by here (after > sctp_sock_migrate call) the sockets are pretty much the same. Mind > elaborating please? In sctp_do_peeloff(),'sock' is 'struct socket' created by sock_create(), which should be freed by sock_release(struct socket *sock), otherwise no place takes care of its freeing. (sock_create() also create sock->sk) In sctp_accept(), 'newsk' is 'struct sock' created by sk_alloc(), and it should be freed by sk_common_release(). Note by then sock->sk is not yet set to 'newsk', so when it return errs, outside can't free newsk when freeing sock. (sys_accept() calls sock_alloc() directly that doesn't set sock->sk) From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xin Long Date: Fri, 08 Mar 2019 03:48:10 +0000 Subject: Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails Message-Id: List-Id: References: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> <20190307182503.GB13515@localhost.localdomain> In-Reply-To: <20190307182503.GB13515@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Marcelo Ricardo Leitner Cc: network dev , linux-sctp@vger.kernel.org, davem , Neil Horman On Fri, Mar 8, 2019 at 2:25 AM Marcelo Ricardo Leitner 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); > > + 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); > > I'm having a hard time understanding why it needs sock_release() here > and sk_common_release() in the above chunk. AFAICT by here (after > sctp_sock_migrate call) the sockets are pretty much the same. Mind > elaborating please? In sctp_do_peeloff(),'sock' is 'struct socket' created by sock_create(), which should be freed by sock_release(struct socket *sock), otherwise no place takes care of its freeing. (sock_create() also create sock->sk) In sctp_accept(), 'newsk' is 'struct sock' created by sk_alloc(), and it should be freed by sk_common_release(). Note by then sock->sk is not yet set to 'newsk', so when it return errs, outside can't free newsk when freeing sock. (sys_accept() calls sock_alloc() directly that doesn't set sock->sk)