Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 13 Oct 2021 15:24:48 -0300
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Wang Hai <wanghai38@huawei.com>
Cc:     dledford@redhat.com, leon@kernel.org, markzhang@nvidia.com,
        liangwenpeng@huawei.com, liweihang@huawei.com,
        haakon.bugge@oracle.com, rolandd@cisco.com, sean.hefty@intel.com,
        linux-rdma@vger.kernel.org, linux-ide@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] IB/cm: Fix possible use-after-free in ib_cm_cleanup()
Message-ID: <20211013182448.GA3489723@nvidia.com>
References: <20211013093016.3869299-1-wanghai38@huawei.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211013093016.3869299-1-wanghai38@huawei.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?G4uagSg474xWMbTebRCWZs+kMvp2F9mjIla+X8dfqmIEGoaIto50sv4vfiJU?=
 =?us-ascii?Q?fl0OJghnSxo/mb1PUSFYfh3Yj+nGHjToTWhEqVbFR5WJaEXWL2vj89Wdrv/4?=
 =?us-ascii?Q?pRvAEwObM8SnlRy/aWAr4JywJq4TT2NGKB1AbGmk1Xgll/w7Ec0vtsRN32eK?=
 =?us-ascii?Q?K/Y6xzcUHGIw7FtAD5Uozyzfe9LpvQV7bLhk2g8N3YEvBuVkrswnSfG6IXNi?=
 =?us-ascii?Q?ljfhcDKdwnChetDBN7DiBadaiFfnG8nfsyTK+b3xR8jadCUkvLZkcpnjLt8N?=
 =?us-ascii?Q?YuMBgOSMla5joNI+wgaUlsp9ZhcryFXEdCSTRvFy240kGTCa1AGKveL86+EM?=
 =?us-ascii?Q?FJbl3LwC+SzaCfF5vSxfbGZNlv21uL1h9pcuztcssmvsgSSSAE0oevhT+W20?=
 =?us-ascii?Q?VPaOsHfo91XGC8LpRP3G5m50bstUSKdTCGjW1o2RBzWxNROKis1mz0YrC4Rk?=
 =?us-ascii?Q?pBF9oHIBBMq9B8AOEZaaDGFMrcpNRaFu0rS6BUFsECK5o+eO4wCUrLdXJvb4?=
 =?us-ascii?Q?zHlPu8YUOC4K2vBHiWSaceJBt2zqYGMMeAj32PRYnvKMfMp9doYs0VBgULla?=
 =?us-ascii?Q?e1tMu1RMTFtzaEEVrGAnRBAK8rPbxIcj8SA9yglfTC8girbFTSmigWacutId?=
 =?us-ascii?Q?SXMCGD2iPCZu4K5LDN1eZj4y/mOdaj9R65vNtsmAJO4l9lbaHhdQpUF7gqhv?=
 =?us-ascii?Q?B5xlMsC9Rcyu39vpIZU2ixn9r4lVlySc1nfrmOHbH+TaGMO8Qhjq4aSbk1yl?=
 =?us-ascii?Q?UVmhP46ajgs33zyeoPhhyvYFdGa0+er33cNDbNO7g8rcVTQ2qF+dyZRKvR25?=
 =?us-ascii?Q?Z85huAeJCu1HwBUpOCt3jX6OTGm/eU+LouQDV3J8YpWSjTEAJHQQqPK7Nj52?=
 =?us-ascii?Q?TLOftAmJ6Icr8yfl2fWqKXcfVJEch1YL80gI6OHN9qldkIfWNUU3QA5srfBp?=
 =?us-ascii?Q?9oQ31w5HxzmlIBA8P5BUquLM3dt8YPg3q8qHvcWxBAe89k8HytZp7Byi4W0I?=
 =?us-ascii?Q?EqVy9OkUTj9mDg7EHPANFgz6/gdzN1MfXixDTiXuSq8Ka5VQxMf+eLprTQDJ?=
 =?us-ascii?Q?0xOC0vVgs8QrlmeWc/RDkbDPxCOM0OEYuD8bHR9EudpqtxRTPQXIHpGMy1If?=
 =?us-ascii?Q?eN6iOVDzgT7yKTRzQqEyC6N3CM5bohTbnGAYK2yVeY8xX7lDeYQtC3AsL4i2?=
 =?us-ascii?Q?fqX32FAQ3Lvb6q4SkCbA8EoAKHQQDYqhxinW9VodWxHfROEHVw2y2aulx3VK?=
 =?us-ascii?Q?F2Kn3IbnyHtiw4Z/Qx8MiH15DmREC1pA/kU1/w5exsi/rh6pd+WJ69ALtEVt?=
 =?us-ascii?Q?+rnd2OlevXBfqmAhCozBaCVt5kj/Asxfq3THfJcw0F03rw=3D=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 38c87a9c-c254-4e29-0ffc-08d98e76be87
X-MS-Exchange-CrossTenant-AuthSource: BL0PR12MB5506.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2021 18:24:50.6280
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zlzl+jeSxUvejcuUCpzxyC89MJQXi1Dz0kzVYzmV+aba9jKj8lGQfY3kXqRyAG+e
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5032
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Oct 13, 2021 at 05:30:16PM +0800, Wang Hai wrote:
> This module's remove path calls cancel_delayed_work(). However, that
> function does not wait until the work function finishes. This means
> that the callback function may still be running after the driver's
> remove function has finished, which would result in a use-after-free.
> 
> Fix by calling cancel_delayed_work_sync(), which ensures that
> the work is properly cancelled, no longer running, and unable
> to re-schedule itself.
> 
> Fixes: 8575329d4f85 ("IB/cm: Fix timewait crash after module unload")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
>  drivers/infiniband/core/cm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index c903b74f46a4..ae0af63f3271 100644
> +++ b/drivers/infiniband/core/cm.c
> @@ -4508,7 +4508,7 @@ static void __exit ib_cm_cleanup(void)
>  
>  	spin_lock_irq(&cm.lock);
>  	list_for_each_entry(timewait_info, &cm.timewait_list, list)
> -		cancel_delayed_work(&timewait_info->work.work);
> +		cancel_delayed_work_sync(&timewait_info->work.work);
>  	spin_unlock_irq(&cm.lock);

No, this will deadlock:

static int cm_timewait_handler(struct cm_work *work)
{
	struct cm_timewait_info *timewait_info;
	struct cm_id_private *cm_id_priv;

	timewait_info = container_of(work, struct cm_timewait_info, work);
	spin_lock_irq(&cm.lock);
         ^^^^^^^^^^^^

Holds the same lock

What is your bug? The destroy_wq() a few lines below will flush out
all the work so it is already not possible that work can still exist
after the driver's remove function has finished.

Jason