From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2130.oracle.com ([156.151.31.86]:43240 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030328AbeCAMUK (ORCPT ); Thu, 1 Mar 2018 07:20:10 -0500 Subject: Re: [PATCH net] rds: Incorrect reference counting in TCP socket creation To: Sowmini Varadhan Cc: netdev , santosh.shilimkar@oracle.com, David Miller , rds-devel@oss.oracle.com, Sowmini Varadhan References: <3403bcbebd03837eec0f86a346daf5c109db2a19.1519879295.git.ka-cheong.poon@oracle.com> From: Ka-Cheong Poon Message-ID: <7940eadb-a01c-172f-5c57-e372993f4bd9@oracle.com> Date: Thu, 1 Mar 2018 20:19:57 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On 03/01/2018 07:54 PM, Sowmini Varadhan wrote: > On Wed, Feb 28, 2018 at 11:44 PM, Ka-Cheong Poon > wrote: >> Commit 0933a578cd55 ("rds: tcp: use sock_create_lite() to create the >> accept socket") has a reference counting issue in TCP socket creation >> when accepting a new connection. The code uses sock_create_lite() to >> create a kernel socket. But it does not do __module_get() on the >> socket owner. When the connection is shutdown and sock_release() is >> called to free the socket, the owner's reference count is decremented >> and becomes incorrect. Note that this bug only shows up when the socket >> owner is configured as a kernel module. > >> >> - new_sock->type = sock->type; >> - new_sock->ops = sock->ops; >> ret = sock->ops->accept(sock, new_sock, O_NONBLOCK, true); >> if (ret < 0) >> goto out; >> >> + new_sock->ops = sock->ops; > > How is this delta relevant to the commit comment? Seems unrelated? Note that sock_release() checks if sock->ops is set before decrementing the refcnt. By moving the ops assignment after the ops->accept() call, we save increasing the refcnt in case the ops->accept() fails. Otherwise, the __module_get() needs to be moved before ops->accept() to handle this failure case. -- K. Poon ka-cheong.poon@oracle.com