Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 4 May 2023 18:43:38 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Thadeu Lima de Souza Cascardo <cascardo@canonical.com>,
        Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] net: avoid double iput when sock_alloc_file fails
Message-ID: <16116c13-5f45-4d9c-ada8-480e570397f0@kili.mountain>
References: <20230307173707.468744-1-cascardo@canonical.com>
 <95e36d70-7841-4e7d-960a-116b8595b820@kili.mountain>
 <53A81159-DD6C-4A5E-994F-49A536F7BD31@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <53A81159-DD6C-4A5E-994F-49A536F7BD31@oracle.com>
Precedence: bulk

On Thu, May 04, 2023 at 03:40:15PM +0000, Chuck Lever III wrote:
> 
> 
> > On May 3, 2023, at 7:31 AM, Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > 
> > I changed the CC list from netdev to linux-nfs.
> > 
> > On Tue, Mar 07, 2023 at 02:37:07PM -0300, Thadeu Lima de Souza Cascardo wrote:
> >> When sock_alloc_file fails to allocate a file, it will call sock_release.
> >> __sys_socket_file should then not call sock_release again, otherwise there
> >> will be a double free.
> >> 
> > 
> > It's just a one liner to make Smatch warn about these sorts of user
> > after free bugs.
> > 
> > { "sock_release", PARAM_FREED, 0, "$" },
> > 
> > Smatch found a related bug in nfs.
> > 
> > net/sunrpc/svcsock.c:938 svc_tcp_accept() warn: 'newsock' was already freed.
> > net/sunrpc/svcsock.c:1633 svc_create_socket() warn: 'sock' was already freed.
> > net/sunrpc/svcsock.c:1555 svc_addsock() error: dereferencing freed memory 'so'
> > net/sunrpc/svcsock.c:1633 svc_create_socket() warn: passing freed memory 'sock'
> > net/sunrpc/svcsock.c:938 svc_tcp_accept() warn: passing freed memory 'newsock'
> > 
> > I guess the svc_setup_socket() function should free sock on every error
> > path?  It seems kind of gnarly.
> 
> I'm looking at current upstream, and I don't see svc_setup_socket()
> releasing @sock on any return path. What am I missing?

The sock_alloc_file() function releases it on error...

> 
> Does your tree have "SUNRPC: Ensure server-side sockets have a sock->file" ?

Ah, that's the commit that introduces the call sock_alloc_file().

regards,
dan carpenter