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=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,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 1DADCC43381 for ; Fri, 8 Mar 2019 16:59:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E0EF320868 for ; Fri, 8 Mar 2019 16:59:16 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="F0w0T3XD" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727906AbfCHQ7P (ORCPT ); Fri, 8 Mar 2019 11:59:15 -0500 Received: from mail-qt1-f193.google.com ([209.85.160.193]:33918 "EHLO mail-qt1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727891AbfCHQ7O (ORCPT ); Fri, 8 Mar 2019 11:59:14 -0500 Received: by mail-qt1-f193.google.com with SMTP id x20so4321572qto.1; Fri, 08 Mar 2019 08:59:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=494b5iRk4Njjz/9p393SfoIgEVTamgr8sstjIHzu/bo=; b=F0w0T3XDEmUpfPCNnWhGAnjgG52NbpyIAn2aShW/pQOoaubwpYCtIHen5bLNfRO6uW 4kXjpXMNXJyujO86+z8h17Dj5gUpsRhGtC7XfpwLWpxpUxvRaDVBGsrSelaYDFdP4rDx D7vMW9LoDejepj1yx2OUDaxwDmsatA7EV3kLUgpF0YkeHxeH3iEiS0PaJEOyLxMbH0QT onY3d72TwJf8uGbS+J9RcJigG65nk7uTsV193Mh72eJOggvwVkJAHEuWE6NyuL2qoK5q zc0k28uVbwOMvfI/d3FzU/mdtG033Qp1wktALvcRqBo9ht9vUNDz4EbVHT3xCm7pgOAU RcNQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=494b5iRk4Njjz/9p393SfoIgEVTamgr8sstjIHzu/bo=; b=jOaTEbwzgV5hAkEIgn3AwYzvtao1DfV8YNlTyeaLeu2NSN3WesZwBPFIRS9gCYZc6K Pr4Vvcn3sjxKcfIIiJZUBEDOAP+wU//SjWNmS7zy4wxygaSCp3bD+13d/RDLBnpk38r7 YVx74MGZhsOmnltlLkXRltmAD/QO9Ihx366IsSaUi6pDC9596ymQ7Dx7ygwZZN+AUb8y hrt7MvhLTRVfigkY3KptvxjNJrXodTL5Ru/ie9XcDXZYqelw/CZ6CKypaxAo1ncILQ0q UWbAxDWUu10hNNLFXPs99OzLCAddObvyRUiicGVh2pqKG+Q5LSw04+jpo6GG+09bvAqp W/jw== X-Gm-Message-State: APjAAAVsvf6iovcWy/quNVasUCRiiNadoMDHUPmyVASNAIXiPfNhVUgO AwXj7FVpk9CGMzmueaWD7OV+3e8m X-Google-Smtp-Source: APXvYqy0BXQZKjREtUsVMo7FKtPHvXeyPzPk496QUElefg9170uHxFKAVrMwHF3YSplMQlCtacsNvw== X-Received: by 2002:ac8:3872:: with SMTP id r47mr15841864qtb.364.1552064353043; Fri, 08 Mar 2019 08:59:13 -0800 (PST) Received: from localhost.localdomain ([168.181.49.39]) by smtp.gmail.com with ESMTPSA id i6sm4331465qkf.3.2019.03.08.08.59.11 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 08 Mar 2019 08:59:12 -0800 (PST) Received: by localhost.localdomain (Postfix, from userid 1000) id 7BB23180BE0; Fri, 8 Mar 2019 13:59:09 -0300 (-03) Date: Fri, 8 Mar 2019 13:59:09 -0300 From: Marcelo Ricardo Leitner To: Xin Long Cc: network dev , linux-sctp@vger.kernel.org, davem , Neil Horman Subject: Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails Message-ID: <20190308165909.GC13515@localhost.localdomain> References: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> <20190307182503.GB13515@localhost.localdomain> 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 Fri, Mar 08, 2019 at 11:48:10AM +0800, Xin Long wrote: > 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) Thanks. Marcelo From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Date: Fri, 08 Mar 2019 16:59:09 +0000 Subject: Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr_dup() fails Message-Id: <20190308165909.GC13515@localhost.localdomain> List-Id: References: <6837e72485125c8740900fd17fa84ac68b8892a5.1551606805.git.lucien.xin@gmail.com> <20190307182503.GB13515@localhost.localdomain> 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 , Neil Horman On Fri, Mar 08, 2019 at 11:48:10AM +0800, Xin Long wrote: > 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) Thanks. Marcelo