Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Fri, 22 Sep 2023 16:13:40 +0100
From:   Giovanni Cabiddu <giovanni.cabiddu@intel.com>
To:     Mikulas Patocka <mpatocka@redhat.com>
CC:     Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        Michael Hrivnak <mhrivnak@redhat.com>,
        Eric Garver <egarver@redhat.com>, <qat-linux@intel.com>,
        <linux-crypto@vger.kernel.org>, <dm-devel@redhat.com>
Subject: Re: [PATCH] qat: fix deadlock in backlog processing
Message-ID: <ZQ2vJNs/7ZzY44z1@gcabiddu-mobl1.ger.corp.intel.com>
References: <af9581e2-58f9-cc19-428f-6f18f1f83d54@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <af9581e2-58f9-cc19-428f-6f18f1f83d54@redhat.com>
Organization: Intel Research and Development Ireland Ltd - Co. Reg. #308263 -
 Collinstown Industrial Park, Leixlip, County Kildare - Ireland
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Kdv7ou9WzeFoQbPoy3Ov5PA8VrAkIBjHNiLhlaz3zYy9gt/fWmOCij+cQ8bB?=
 =?us-ascii?Q?gEtIWca180e9nowvjfVcWmvc5nYTYSwIr2+/kP035DQqGQGMBdwC1K9uSavW?=
 =?us-ascii?Q?TqXPj19YBKG1ETn44b15ZBbHQE0IqgmGwgeR1kcQRck2m1WBfCOkvNH5Rb6+?=
 =?us-ascii?Q?q6zJ1rU6iTQRLC53S4q40j6XxuqNMnOCS31+YGra6OnskXa1DvSKhP/YHF8T?=
 =?us-ascii?Q?66sMm9+7nRLDjQ6oJapNEl6cxXshNGTg6ItdKDw5n/2IIbYZfoeCK/LY/GrV?=
 =?us-ascii?Q?+5kIiJnb5//onf23Wo7sLQ2R657ZgAGyTtv46cUAJA6TfGdwjE/mty7rBnVM?=
 =?us-ascii?Q?Rn/mIo9Kh5oBxoeQLgHlH/Shguv+mn26MOfggRt4LTWcaro08aYT6yXHsoQy?=
 =?us-ascii?Q?Fe8PvSPMoGMV6gJASIN7h8STpjAhtYDd9dxBXchMaPdt0ke5ARGPsKdUz/cY?=
 =?us-ascii?Q?Lo5IFD1XfqscjaC32w2/3gcxA+Z70rcFc9fYF0QnwO7cOZhiT8y6MDSmbMze?=
 =?us-ascii?Q?qI0R5pZq8GWx47cw/81SggMGzSnbCfRYRGXppNE1EmEgVcKWCKCTU9sRpmRs?=
 =?us-ascii?Q?hELqW80Y+0wCJpt8mT6YnfCINmQerk7K2thfxp5lHNZNVa35erEien122JWQ?=
 =?us-ascii?Q?OmXUlh/+RatuQrKKhhhHp/ZobvIJnXDcuvLyBhUrwbLCipuf0d9ISJfcAVvJ?=
 =?us-ascii?Q?239fJnhVZXWhOrkXiKuTKpMAE4Es+zpsGYmrE7M+04kjh9yztXdTJeJm6OPJ?=
 =?us-ascii?Q?/JKumxu8DHb6TI+/mVDRSAORO+YtE/HrCxMyyVciSmGfODwBxxpmbpC9FMLZ?=
 =?us-ascii?Q?jNpAwL/kXBlLEDlWI4lTAtxUMveOSN8BqkxyvjRFpQevj4t6DTRTPetreMB1?=
 =?us-ascii?Q?RDqsz5erxvZWfqyB1cRYedxpw/kS+eWLjvNw80w6emdNo4eOUZflnsNY/J8+?=
 =?us-ascii?Q?pJIeZ0IlaxNkucw3xHfYPlBVvxGgfM4BimYDSTOqI0KXGAw6D3qJbqoWTP30?=
 =?us-ascii?Q?PA3gt626f/HBYrliEUvgaiwpGPUekYnc5TfoF+I8QpJHG++EaPUjV1/k93R6?=
 =?us-ascii?Q?9RNDdACVs7exLLS5x8ysybj88NN0Meo2V1tiCNmolPz0YcE/T9APRQJMD4k/?=
 =?us-ascii?Q?z15vYxWQt7b2rIQiM71GeWSafjF0lORAA/yvdR0rc2Q6PFHW173UakxnfNgM?=
 =?us-ascii?Q?J5RHjK2YKDxGIjdnk4aLHS1W0O8wnN5cjUx25zPSgmYyaxIM33WcIfOD4qbj?=
 =?us-ascii?Q?0zFvCDJvg+VZP84+SHEQIL/VJxrhmxxvhv4altwPDarjMOfSozPz97FSRVoY?=
 =?us-ascii?Q?qq0p2575UxX90jr+vI+eKmQCwbhX1wviwhiBq23I1EsJhhV0Ajz/T7LaF0Cz?=
 =?us-ascii?Q?ADJP+1FTRBHKV5Wos9Oj8wBK1GIdfRQ2ZDdVtUukJVPY5y+/fY17E0rwOPWp?=
 =?us-ascii?Q?qou69GbqZShJ/L+X5kJv5+OtjslW7eYS3bKw4aryUAMP/1FvM4JN3HJIB9rA?=
 =?us-ascii?Q?SLQ5zPIc+OSmBYLmeelS58xtU0t8A/JP2pppxnUwENfJ6ZJr/f1Z3NPByso0?=
 =?us-ascii?Q?AnJT8c77vmVCqBiofb5oy1DjyqORrN/tHwtsyiNjYJd2QVgNO+UloSiDS3Gs?=
 =?us-ascii?Q?1Q=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0c124889-06dc-47ea-9763-08dbbb7e8882
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6366.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Sep 2023 15:13:53.6204
 (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: WyTw61UEIyyDowd/LNbwgI1kKAn61dOH+TKeAF/+rVmz9/UdMXtMmGog6rpjN3iz0xLNmHh7jjmCr/6fMo/m878XGfXnmWYC5knG2jGxoP0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6282
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        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-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Fri, 22 Sep 2023 08:14:01 -0700 (PDT)

Hi Mikulas,

many thanks for reporting this issue and finding a solution.

On Thu, Sep 21, 2023 at 10:53:55PM +0200, Mikulas Patocka wrote:
> I was evaluating whether it is feasible to use QAT with dm-crypt (the 
> answer is that it is not - QAT is slower than AES-NI for this type of 
> workload; QAT starts to be benefical for encryption requests longer than 
> 64k).
Correct. Is there anything that we can do to batch requests in a single
call?

Sometime ago there was some work done to build a geniv template cipher
and optimize dm-crypt to encrypt larger block sizes in a single call,
see [1][2]. Don't know if that work was completed.

>And I got some deadlocks.
Ouch!

> The reason for the deadlocks is this: suppose that one of the "if"
> conditions in "qat_alg_send_message_maybacklog" is true and we jump to the
> "enqueue" label. At this point, an interrupt comes in and clears all
> pending messages. Now, the interrupt returns, we grab backlog->lock, add
> the message to the backlog, drop backlog->lock - and there is no one to
> remove the backlogged message out of the list and submit it.
Makes sense. In my testing I wasn't able to reproduce this condition.

> I fixed it with this patch - with this patch, the test passes and there
> are no longer any deadlocks. I didn't want to add a spinlock to the hot
> path, so I take it only if some of the condition suggests that queuing may
> be required.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Cc: stable@vger.kernel.org
The commit message requires a bit of rework to describe the change.
Also, deserves a fixes tag.

> 
> ---
>  drivers/crypto/intel/qat/qat_common/qat_algs_send.c |   31 ++++++++++++--------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> Index: linux-2.6/drivers/crypto/intel/qat/qat_common/qat_algs_send.c
> ===================================================================
> --- linux-2.6.orig/drivers/crypto/intel/qat/qat_common/qat_algs_send.c
> +++ linux-2.6/drivers/crypto/intel/qat/qat_common/qat_algs_send.c
> @@ -40,16 +40,6 @@ void qat_alg_send_backlog(struct qat_ins
>  	spin_unlock_bh(&backlog->lock);
>  }
>  
> -static void qat_alg_backlog_req(struct qat_alg_req *req,
> -				struct qat_instance_backlog *backlog)
> -{
> -	INIT_LIST_HEAD(&req->list);
Is the initialization of an element no longer needed?

> -
> -	spin_lock_bh(&backlog->lock);
> -	list_add_tail(&req->list, &backlog->list);
> -	spin_unlock_bh(&backlog->lock);
> -}
> -
>  static int qat_alg_send_message_maybacklog(struct qat_alg_req *req)
>  {
>  	struct qat_instance_backlog *backlog = req->backlog;
> @@ -71,8 +61,27 @@ static int qat_alg_send_message_maybackl
>  	return -EINPROGRESS;
>  
>  enqueue:
> -	qat_alg_backlog_req(req, backlog);
> +	spin_lock_bh(&backlog->lock);
> +
> +	/* If any request is already backlogged, then add to backlog list */
> +	if (!list_empty(&backlog->list))
> +		goto enqueue2;
>  
> +	/* If ring is nearly full, then add to backlog list */
> +	if (adf_ring_nearly_full(tx_ring))
> +		goto enqueue2;
> +
> +	/* If adding request to HW ring fails, then add to backlog list */
> +	if (adf_send_message(tx_ring, fw_req))
> +		goto enqueue2;
In a nutshell, you are re-doing the same steps taking the backlog lock.

It should be possible to re-write it so that there is a function that
attempts enqueuing and if it fails, then the same is called again taking
the lock.
If you want I can rework it and resubmit.

> +
> +	spin_unlock_bh(&backlog->lock);
> +	return -EINPROGRESS;
> +
> +enqueue2:
> +	list_add_tail(&req->list, &backlog->list);
> +
> +	spin_unlock_bh(&backlog->lock);
>  	return -EBUSY;
>  }

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1276510.html
[2] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1428293.html

Regards,

-- 
Giovanni