Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 7 Apr 2022 14:24:56 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Duoming Zhou <duoming@zju.edu.cn>
Cc:     linux-kernel@vger.kernel.org, chris@zankel.net, jcmvbkbc@gmail.com,
        mustafa.ismail@intel.com, shiraz.saleem@intel.com, jgg@ziepe.ca,
        wg@grandegger.com, mkl@pengutronix.de, davem@davemloft.net,
        kuba@kernel.org, pabeni@redhat.com, jes@trained-monkey.org,
        gregkh@linuxfoundation.org, jirislaby@kernel.org,
        alexander.deucher@amd.com, linux-xtensa@linux-xtensa.org,
        linux-rdma@vger.kernel.org, linux-can@vger.kernel.org,
        netdev@vger.kernel.org, linux-hippi@sunsite.dk,
        linux-staging@lists.linux.dev, linux-serial@vger.kernel.org,
        linux-usb@vger.kernel.org
Subject: Re: [PATCH 09/11] drivers: infiniband: hw: Fix deadlock in
 irdma_cleanup_cm_core()
Message-ID: <20220407112455.GK3293@kadam>
References: <cover.1649310812.git.duoming@zju.edu.cn>
 <4069b99042d28c8e51b941d9e698b99d1656ed33.1649310812.git.duoming@zju.edu.cn>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4069b99042d28c8e51b941d9e698b99d1656ed33.1649310812.git.duoming@zju.edu.cn>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?UlBc+ZrT7Z18QMKD1850bZcfPEcP9YuXNf5iE3EzB8OumU5it7PCWop4Cs9R?=
 =?us-ascii?Q?coD3HiKAjd2eNM/yl8uHBh/6h00FQ+xyegj6hGKgNzw7dkJ0Ni2g4Pz10Lcf?=
 =?us-ascii?Q?JHKtpJRh43oA8DjWRRS1uOXD+//ELAVriocnaRg/hB6O+EyFTukNwlI2vDWQ?=
 =?us-ascii?Q?tSuoGAjIuG94t3r5cZkLhfDMfkqUv0EXKH/guvEeweSb2f/E9hGHHr5QTGnY?=
 =?us-ascii?Q?qaopIqUItQI8HXCa3Y62hJAW4vczs2cD/j6l7wvhrY3BYUcnqr56sfTYvOzS?=
 =?us-ascii?Q?kQYW+VypZbPwpMmfOx842fBERDIPhSk9D7C2veHZ9+20tmSp7W5y+ZDz5SQC?=
 =?us-ascii?Q?AVHiiFHsY0Pu+bXjMTTPD+Ont/Zgl1Df7Vp1XBUhAV422I8f2Y+nWML0eZyQ?=
 =?us-ascii?Q?2myACaFnDvfzzSjD+DjG/gz9OxqeOxpL7lVGN9476sGdDbVBCo4eaF7E25Vd?=
 =?us-ascii?Q?9iYEjqEL1MHCTLwyDWt65uXqhbm4bEYvgqvD+iH/q6tRoVeXas1KHhO/Efg/?=
 =?us-ascii?Q?6myfjyIJB6VSjsn0KgCpirsn4S3Fa7dBtvIwdLOx27FrrGkEmts0q9+l46sO?=
 =?us-ascii?Q?e3T1dueI/o1kUm2iLVopfBZXGA4MBqDzxuz901+b3vmtpnrvONCkZuepjloU?=
 =?us-ascii?Q?HpqrJ54ZNrjx/ZIlndshVE7fppCsBaXJgiQQ/nRtKXbA248Gxr17BRKyz7bk?=
 =?us-ascii?Q?xD8d6zRm0t58hlMchHiIx+LnnUl1Sh8lE2KQRKLrCBSJDPy2zndCEYx62p4F?=
 =?us-ascii?Q?kfKWy3H7EBAdBrpSP2SSOlSoc12RS8Stm0Bn+g8GPsS9xfSwP/SMMU2kbaS0?=
 =?us-ascii?Q?WiGxJzB/rV2Jqvu8E3xt1Gf5p3ABkzJAGFjYa1BCuKwGjui7srWXikWPwikH?=
 =?us-ascii?Q?H2b+2QhXgYUEETABbmdIXOWRhE2Cy1S3mM7VmIZMwM3VyjKpcWqpt5kEnnpu?=
 =?us-ascii?Q?KcbQ0Pa6pqk1wR3KLJcVao6lzp1d0KEH8eOitBtX5lXNSfbHCxhOvMDC05Zc?=
 =?us-ascii?Q?XoJ6Uf+dBXEbF/PPlMN9Ixycdg4GIiBO+yIDiEP80EcotKAEtShHJ5Da0D5d?=
 =?us-ascii?Q?97LPL8NkJqa3jcWmETR6tQHlMa4u8dAv/MpaR1GzWwYDG2EN4/qKFfJpeknw?=
 =?us-ascii?Q?RC/nOHjD9jXUPyAQ5HOu/iAYmFFJpPQR4dv2f6YjzPquUv6XOaPZIl8PUG35?=
 =?us-ascii?Q?QFjb3u3G1GAfdWckCnIY4v7OIbt8F1W9e3jej1om/NHjqbml5X5BiHFnZ1FV?=
 =?us-ascii?Q?KzpevXmrje4Fzv6bRExwvtn0hEX1UD293MvXKk07CtxiGPOiy+DuqgY/nlcN?=
 =?us-ascii?Q?Pj01Hyl9YhtZXpgIl7pATwN2PKCPCpC4eY2j+pR7PFUcMyEGVge5EXLJi1T0?=
 =?us-ascii?Q?nLkNx7dZW7VRjWyzshujlfyng946BQt5E9p0AN6sCS3pAxD4dyo+ERHFSIEa?=
 =?us-ascii?Q?6avNRKNGdvLaqKCSmF14LrYClUk5FqBQEOJ6QyA8UNQL4Dh1t/8YuZH0daNd?=
 =?us-ascii?Q?d3stahhFSz5g/uH3loSiHCSywh+lIXEBsjbuB52l/CRfknYD26l57JVmXqf3?=
 =?us-ascii?Q?fw3/wXdX5HotxwD/L5RnmuTimZRjsTh6HaSNVTPVCW6vMgbVtSO3cQaJCsGI?=
 =?us-ascii?Q?Yy0+bbS+KyCizq+5FjlunGuUlQmfDwqaJacmSdkR39dNlvOzj66Yc3FB0yYu?=
 =?us-ascii?Q?eWKctXjgX5H6nvRbA1KB1+RZVuK5nLyRxfGNijtZkND/rC3inOMLKgSFKzRI?=
 =?us-ascii?Q?xSPKhBnTl8noXVPmtcAsqUMxYvLEg8I=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 40027955-e4ae-48a6-a7cd-08da18894856
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Apr 2022 11:25:13.9882
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: moWB1Voj/j0hnWZ+tEVa2AUsrau5+zGHvb6hN5TzuYS+Re/ikaBVfuRc1G9AKuCZLTgNK0JolVJAf02KudX+VkkskDeXsr7i3E+JQQIFpaM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5087
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.425,18.0.850
 definitions=2022-04-07_01:2022-04-07,2022-04-07 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=806 malwarescore=0
 mlxscore=0 suspectscore=0 bulkscore=0 adultscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204070056
X-Proofpoint-ORIG-GUID: vyeuNwlmkuby6vzU39pv8MDCglSJkh-L
X-Proofpoint-GUID: vyeuNwlmkuby6vzU39pv8MDCglSJkh-L
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Apr 07, 2022 at 02:37:12PM +0800, Duoming Zhou wrote:
> There is a deadlock in irdma_cleanup_cm_core(), which is shown
> below:
> 
>    (Thread 1)              |      (Thread 2)
>                            | irdma_schedule_cm_timer()
> irdma_cleanup_cm_core()    |  add_timer()
>  spin_lock_irqsave() //(1) |  (wait a time)
>  ...                       | irdma_cm_timer_tick()
>  del_timer_sync()          |  spin_lock_irqsave() //(2)
>  (wait timer to stop)      |  ...
> 
> We hold cm_core->ht_lock in position (1) of thread 1 and
> use del_timer_sync() to wait timer to stop, but timer handler
> also need cm_core->ht_lock in position (2) of thread 2.
> As a result, irdma_cleanup_cm_core() will block forever.
> 
> This patch extracts del_timer_sync() from the protection of
> spin_lock_irqsave(), which could let timer handler to obtain
> the needed lock.
> 
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
>  drivers/infiniband/hw/irdma/cm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c
> index dedb3b7edd8..019dd8bfe08 100644
> --- a/drivers/infiniband/hw/irdma/cm.c
> +++ b/drivers/infiniband/hw/irdma/cm.c
> @@ -3252,8 +3252,11 @@ void irdma_cleanup_cm_core(struct irdma_cm_core *cm_core)
>  		return;
>  
>  	spin_lock_irqsave(&cm_core->ht_lock, flags);
> -	if (timer_pending(&cm_core->tcp_timer))
> +	if (timer_pending(&cm_core->tcp_timer)) {
> +		spin_unlock_irqrestore(&cm_core->ht_lock, flags);
>  		del_timer_sync(&cm_core->tcp_timer);
> +		spin_lock_irqsave(&cm_core->ht_lock, flags);
> +	}
>  	spin_unlock_irqrestore(&cm_core->ht_lock, flags);

This lock doesn't seem to be protecting anything.  Also do we need to
check timer_pending()?  I think the del_timer_sync() function will just
return directly if there isn't a pending lock?

regards,
dan carpenter