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;
From:   "Saleem, Shiraz" <shiraz.saleem@intel.com>
To:     Duoming Zhou <duoming@zju.edu.cn>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
        "jgg@ziepe.ca" <jgg@ziepe.ca>,
        "Ismail, Mustafa" <mustafa.ismail@intel.com>,
        "dan.carpenter@oracle.com" <dan.carpenter@oracle.com>
Subject: RE: [PATCH V4 09/11] drivers: infiniband: hw: Fix deadlock in
 irdma_cleanup_cm_core()
Thread-Topic: [PATCH V4 09/11] drivers: infiniband: hw: Fix deadlock in
 irdma_cleanup_cm_core()
Thread-Index: AQHYSvYSNtIPz+QHNUyVwleSCPhwc6zmPuQg
Date:   Fri, 8 Apr 2022 18:17:46 +0000
Message-ID: <MWHPR11MB00291BA918E17176BDB12578E9E99@MWHPR11MB0029.namprd11.prod.outlook.com>
References: <20220408030904.34145-1-duoming@zju.edu.cn>
In-Reply-To: <20220408030904.34145-1-duoming@zju.edu.cn>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.401.20
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?xzCrN3rs2irvBXdrmx7P4ENmeYQLpWvE0tDux7UwgMOnM23EmFlCRPcjGsju?=
 =?us-ascii?Q?JzR5xT7/JBM6JJSLjLQXVspzbV3ILbj/2+XUySdt+5bEBqy7OtMA5klKznln?=
 =?us-ascii?Q?vicyJCCaZ/iSPnDuMecq1fGQmihNtmyfDdOp9ICv4qw9998iUFpm7NRpsyBq?=
 =?us-ascii?Q?IL1L9M4e4+G+cWpUPbO0we7Wyvmrf1dakvaJA2czRmCspscQKZeqOoRbHrEz?=
 =?us-ascii?Q?rCUqSXqyR0rz0LlXHPV9W/yLIdTyG18xpTLYyRK8j4GOJ9/F4Thdp7/dlIIw?=
 =?us-ascii?Q?W1G1xmFYNKZhyQd9EzBQbdV7C0DiweU0wUXCJMdpPkXqnK9BywFYKU2sYl61?=
 =?us-ascii?Q?IUod9AxqDc9ZwSh9+Af6mp+RETb0WawTeFnx4PikHGCYdpi5FSgulqTqpoiH?=
 =?us-ascii?Q?/oqXW2w02xJyDnDcCUCQJ3rKPSO49dyrINDpT58cBmjwUsfERi5U3afQerpF?=
 =?us-ascii?Q?+iCSY8f7sW5QnhKBRyRuEcyVIznTqU+FFJ1297GM3pQofi/TXfYQeDb/Ycdb?=
 =?us-ascii?Q?WGIty6PcYyAtsl/rD+sjh9osIJuDFPUpksL2eQejHEus3nhbQUOS+K/Fcj+z?=
 =?us-ascii?Q?1iEEPW9EYotI3pRejjqwSw4/cvU+BLSJ3hM8zk9ujnEVgzU+SnNVI3MzVXGn?=
 =?us-ascii?Q?JRc1lcXTffIV3D6VfcvbeZSTksAA2Kou+Q73Sy0HljIw8+3uyI+Op2MiD+Hf?=
 =?us-ascii?Q?a7WVapH7lFWfWaO7l4/Xh69TuT6uM1fkufrmGdDCfZ7jywaMwcBPF00yRL6Y?=
 =?us-ascii?Q?C/wGKWP1D89DYPnG0Al6QGUNtdnhs7nVX14drQ+C+gUOpaiDhyUMpNrL+LxE?=
 =?us-ascii?Q?lAq4tuOePstAUQw/sCSoZAsTlZHqnuVAH7VEjHUZtycMBkEWHdiYP8ciUkgx?=
 =?us-ascii?Q?0yNA1TZTTx6+zGsVkGZXOjnvVh2bbxqDkb6oMCh/U3FpU3gv62UW6rLpnaiz?=
 =?us-ascii?Q?h2Hj6m3WOP8hInCYwWhHi70es0YQwbuodZFI+2koOWD8xc+1eW0E39rKv+v/?=
 =?us-ascii?Q?FjutXO9TRYyZsMutiGdk3mbigcS6eEXLFVHWXyYz4nZFgzCpiL2MnC0sVl+k?=
 =?us-ascii?Q?3JC5aEd6ammjbfPJ/Doe+mZu2S/kw7nbhpbI47LW4aL9CPX8Mb7Fjbck8oz5?=
 =?us-ascii?Q?CXxuZyGbgpSh/ghXyRTKZocDmgqAI54kFcQPuCu4MOIPlmLWRRT/p9u3cqQf?=
 =?us-ascii?Q?nCn/p+VKRfw+DHeHLVbnK1zQFfwQA2AhzKGo7vp3lfnMB7Xaj54k1FWY5Gt0?=
 =?us-ascii?Q?H5JaVCymlaxYvTfncqveoBOP3vXL1yUmzs8LUyoA4Sxl98sTUiwwco4EudJ+?=
 =?us-ascii?Q?C8di1rEXBjQpvDUC/QHYGgfKiwMptI4xBWzsdkD5iVKXBbLoaeInb6X/1H4j?=
 =?us-ascii?Q?JIdUmWh8uVseAXTkB4nuFkhdsSvu8lC6ENfmIdujEteh1sXjf7Leo5Tx02Pt?=
 =?us-ascii?Q?yuuxamhUzb3q0I5nsqQYCm3MbzVdjx+Z3DekrwfhZaJgaK63MtjAOWsiYi7l?=
 =?us-ascii?Q?7mTo7GAorO4lwW6QLG1WeJXodJMPXbb+C6ybJv+a9tm+SY+ep0PVLA0H17b2?=
 =?us-ascii?Q?RaielWT/pjS9FOWL67nRaPDMpGBU2Rbog+P7Et2ZsWuVIbfhh4UTanOoWX59?=
 =?us-ascii?Q?O/BkQaIVhAS4aoxW5w4Ui9KY5eMKunkD5hafGbiYgIHZO/A9Trw+oFp0NWXG?=
 =?us-ascii?Q?p2Or9w2GHAQKWjCH3C3A8UxRHPT80NnAiI9WP+w8onaXhZfYyfy9j09z/nGy?=
 =?us-ascii?Q?VrXsOugAmA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: MWHPR11MB0029.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8183f73a-2f31-46d2-444c-08da198c14ca
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Apr 2022 18:17:46.0637
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: CyGptmuQNuGSupiurDXt4rpVhgkExALqBk0rLrHHhwfnD1nKN+P5IMK8WmES5dxR4e1dTytv5lwSB/8An+pclA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB3161
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        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

> Subject: [PATCH V4 09/11] drivers: infiniband: hw: Fix deadlock in
> irdma_cleanup_cm_core()
>=20
> There is a deadlock in irdma_cleanup_cm_core(), which is shown
> below:
>=20
>    (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)      |  ...
>=20
> We hold cm_core->ht_lock in position (1) of thread 1 and use del_timer_sy=
nc() to
> wait timer to stop, but timer handler also need cm_core->ht_lock in posit=
ion (2) of
> thread 2.
> As a result, irdma_cleanup_cm_core() will block forever.
>=20
> This patch removes the check of timer_pending() in irdma_cleanup_cm_core(=
),
> because the del_timer_sync() function will just return directly if there =
isn't a pending
> timer. As a result, the lock is redundant, because there is no resource i=
t could
> protect.
>=20
> What`s more, we add mod_timer() in order to guarantee the timer in
> irdma_schedule_cm_timer() and irdma_cm_timer_tick() could be executed.
>=20
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
> Changes in V4:
>   - Add mod_timer() in order to guarantee the timer could be executed.
>=20
>  drivers/infiniband/hw/irdma/cm.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>=20
> diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/ird=
ma/cm.c
> index dedb3b7edd8..e4117b978bf 100644
> --- a/drivers/infiniband/hw/irdma/cm.c
> +++ b/drivers/infiniband/hw/irdma/cm.c
> @@ -1184,6 +1184,8 @@ int irdma_schedule_cm_timer(struct irdma_cm_node
> *cm_node,
>  	if (!was_timer_set) {
>  		cm_core->tcp_timer.expires =3D new_send->timetosend;
>  		add_timer(&cm_core->tcp_timer);
> +	} else {
> +		mod_timer(&cm_core->tcp_timer, new_send->timetosend);
>  	}

There is no need to do a mod_timer. In the timer pending case, the handler =
will fire when the current armed timer expires.

The handler batch process connection nodes of interest. And this connection=
 node is marked for processing.


>  	spin_unlock_irqrestore(&cm_core->ht_lock, flags);
>=20
> @@ -1367,6 +1369,8 @@ static void irdma_cm_timer_tick(struct timer_list *=
t)
>  		if (!timer_pending(&cm_core->tcp_timer)) {
>  			cm_core->tcp_timer.expires =3D nexttimeout;
>  			add_timer(&cm_core->tcp_timer);
> +		} else {
> +			mod_timer(&cm_core->tcp_timer, nexttimeout);

ditto. Please remove.

>  		}
>  		spin_unlock_irqrestore(&cm_core->ht_lock, flags);
>  	}
> @@ -3251,10 +3255,7 @@ void irdma_cleanup_cm_core(struct irdma_cm_core
> *cm_core)
>  	if (!cm_core)
>  		return;
>=20
> -	spin_lock_irqsave(&cm_core->ht_lock, flags);
> -	if (timer_pending(&cm_core->tcp_timer))
> -		del_timer_sync(&cm_core->tcp_timer);
> -	spin_unlock_irqrestore(&cm_core->ht_lock, flags);
> +	del_timer_sync(&cm_core->tcp_timer);

This is fine.

Shiraz