From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1FFFEC021B8 for ; Thu, 27 Feb 2025 02:24:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8EBE36B0088; Wed, 26 Feb 2025 21:24:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 89BC66B0089; Wed, 26 Feb 2025 21:24:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7646C6B008A; Wed, 26 Feb 2025 21:24:54 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 523C86B0088 for ; Wed, 26 Feb 2025 21:24:54 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 05F6FA3B53 for ; Thu, 27 Feb 2025 02:24:54 +0000 (UTC) X-FDA: 83164131708.12.D67582E Received: from out-186.mta0.migadu.com (out-186.mta0.migadu.com [91.218.175.186]) by imf10.hostedemail.com (Postfix) with ESMTP id 383A2C000C for ; Thu, 27 Feb 2025 02:24:52 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=tKpN6NKg; spf=pass (imf10.hostedemail.com: domain of chengming.zhou@linux.dev designates 91.218.175.186 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740623092; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=GKRxFCKu4oxfV1QzoclFLAwqr7hAo95BPBl9HIQDinE=; b=RjnFIU+VAsYsqMnQvBj1T0M3VkNj1gf2UXCGUSAOn/JmpTw56lNQjmH98NCNy9GRDULp9o UuVzKkDZtm2y4SqiCmtkXzlsIpewoPBItMIZ36PcdEFJOFxyigDHRjs2plhQamHKcmD0DB 3KI1DRwo3NQvWKQbxmZwblVoTR/gObw= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=tKpN6NKg; spf=pass (imf10.hostedemail.com: domain of chengming.zhou@linux.dev designates 91.218.175.186 as permitted sender) smtp.mailfrom=chengming.zhou@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740623092; a=rsa-sha256; cv=none; b=nNaRdJgH33ckSidRQVR/8xeX9P5MEXa2F85cdHxFvmEV0Qf24vyIeaWKKFXdf9BYiVs4qo ZfLmdHnC+61X5zwlX3qdlte2Fnqftn8olLDUSbA86m4M1axhbMHZVG6Ili8gfUhfsUOvx9 yAnBx13RE2qH8xUyQ9wCCZ6ZaFDZqwM= Message-ID: <08e2e50a-f289-4019-9d74-62ecc45473e3@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1740623089; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=GKRxFCKu4oxfV1QzoclFLAwqr7hAo95BPBl9HIQDinE=; b=tKpN6NKg6sJnT/SzcNzMIeFUIPKyWoXRe0iH44xHowIwU/Ny37tt8fimJE5m7BcPcOtea2 qc6c81VOr1vWbV++NsLtU0KJFDBgbiE/kgExIxSUb3sKf9/r5uoC336vrmADpaCL9ITq/B k7Sgu+7h80pHH5nZi8V9RtUTEtnKBu4= Date: Thu, 27 Feb 2025 10:24:40 +0800 MIME-Version: 1.0 Subject: Re: [PATCH v2] mm: zswap: fix crypto_free_acomp() deadlock in zswap_cpu_comp_dead() To: Yosry Ahmed , Andrew Morton Cc: Johannes Weiner , Nhat Pham , "David S. Miller" , Herbert Xu , linux-mm@kvack.org, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com, syzbot+1a517ccfcbc6a7ab0f82@syzkaller.appspotmail.com, stable@vger.kernel.org References: <20250226185625.2672936-1-yosry.ahmed@linux.dev> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Chengming Zhou In-Reply-To: <20250226185625.2672936-1-yosry.ahmed@linux.dev> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 383A2C000C X-Stat-Signature: fa71xn4c5wciw3ojk3cuozqp3scug6cx X-Rspam-User: X-HE-Tag: 1740623092-164588 X-HE-Meta: U2FsdGVkX18psdxZos5m44Zl4tSvWK4IV6FCQcpciYu5BylUuQIzACvDNaJt7UMRIr6hKw2vUyAyQuqhxkPzQKsp3zi/9yWhQQw3mmhIsDwo8QDBj//kM/y8wP5FkUtBomLHmUht0lXI9FoYQYxLjvmMcRoIrf72KGV4M2l0OGuAUcUC0AxbNt/aqlIvMraHrCELTfc49aOs/D7MYR7qQW9juHWJsbVMjn852Kc1aEth+leWx0BJnPF38CYNM+3FhDXeVPAXv9qSgYtUuwXFk/tJA5uDIxyYm1Iq71Ar5Xm1y+VoMrQMlj9CIT4Ex4rIte4OULgiAYQr2nbOozHAnSE4PkiiZCfhZsJNT5XMVuqftGLxy4GM3ayGfby2ANqnGImFTY/uirnSmLWbHWb07Mz21C0TfrDdiy/22y8wpQyGJP5vzrdGWGlNxjCA97Yts/U3hKLoBwTLYtZMi0STiWitd1myk+49k/sZDmrkb8P9781Y0VnPPhScoBvW9gnHWC/Yjjj5iKnHUUCSRoeBaMDSUPCy6iY9cjtEOW5kAird+0c7cGD0L6UON71UJdIY/a1P2VhqB5pZdUtH+NPr6ojzT9/sdhaqB0bhCcy3TiC7VUTcEXXq4zlBcBWX2kIdr9wrHgbpJVggvffKM/vxIpO32sUdLa3TixvfIAE+raHrJaAdUcxaSpVnwHjd41d0RynAGb2D2Ihr1zCOHF1O9Irf3IvgJKlEeFgSOZWKGk623qNsdmE0Qndw/jhzHbiVZ+GMdFvAn9cWFvwIpfRR8qC1+Mf4vr4ESnbkKCjvX5dy/KFjbYo+02hqTOXc8a26fkKYf0qfY2+sJCxZ5osaRAS9aQCJIhR8tGsyAUv7kDShEFD8G0zinaDVbohtiAJF12sOORv79cWoDRimi6RYztRvTXqWRv1DxEIp1HWwUEOCuEZVgaquZDJycZGip33xqL/fhI4ij9QlIlbhVp1 50mysWaJ tNgThKidiLh9w8tsW/4YOmUqZVYwTlYbM83gFKXbCpRQnBJ4ecZlwvUuyu4qEs+r4a18ypJNQr7aN7Pq45wNIbwCokD/hB3v9LN6T6QhBemlZ+No51Lpk0CEMBQHgzAcvs87l8Bk0rdN0CPInFaY2H+i2BSV+Nv2VrNuunETYsMqPsuh/wGu5dqI7bD/WAmxMUwrhjtLaNjlkECPjjyb3aWm5LFnNMLslmhTkKalSbGBn/Qngev3YxTlkmyXQjd9OokVNaqKGCl9Q2z9qHevgeVriSVxHeUoRdCBXNTFZ2SVJCdBdFUJ+lRMZp1tBP3xEEHsHi+2YE3DNw7RPRDLrMD+QEmsSJEoCMeFGk1vDdKgKwicf5YwPN5RCHEz9Gc/X6lI+Bo0lTIYwuMqWMyBON3LlosKP8azcRBUUKkzVHEQWuYApgV6Cmh7Kpbn0poRGrF2mZuB8aTU8Tk+e6BdhPGfqMiGVDTnpzaPjnIag5xNpIP095cQlzVa18w== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 2025/2/27 02:56, Yosry Ahmed wrote: > Currently, zswap_cpu_comp_dead() calls crypto_free_acomp() while holding > the per-CPU acomp_ctx mutex. crypto_free_acomp() then holds scomp_lock > (through crypto_exit_scomp_ops_async()). > > On the other hand, crypto_alloc_acomp_node() holds the scomp_lock > (through crypto_scomp_init_tfm()), and then allocates memory. > If the allocation results in reclaim, we may attempt to hold the per-CPU > acomp_ctx mutex. > > The above dependencies can cause an ABBA deadlock. For example in the > following scenario: > > (1) Task A running on CPU #1: > crypto_alloc_acomp_node() > Holds scomp_lock > Enters reclaim > Reads per_cpu_ptr(pool->acomp_ctx, 1) > > (2) Task A is descheduled > > (3) CPU #1 goes offline > zswap_cpu_comp_dead(CPU #1) > Holds per_cpu_ptr(pool->acomp_ctx, 1)) > Calls crypto_free_acomp() > Waits for scomp_lock > > (4) Task A running on CPU #2: > Waits for per_cpu_ptr(pool->acomp_ctx, 1) // Read on CPU #1 > DEADLOCK > > Since there is no requirement to call crypto_free_acomp() with the > per-CPU acomp_ctx mutex held in zswap_cpu_comp_dead(), move it after the > mutex is unlocked. Also move the acomp_request_free() and kfree() calls > for consistency and to avoid any potential sublte locking dependencies > in the future. > > With this, only setting acomp_ctx fields to NULL occurs with the mutex > held. This is similar to how zswap_cpu_comp_prepare() only initializes > acomp_ctx fields with the mutex held, after performing all allocations > before holding the mutex. > > Opportunistically, move the NULL check on acomp_ctx so that it takes > place before the mutex dereference. > > Fixes: 12dcb0ef5406 ("mm: zswap: properly synchronize freeing resources during CPU hotunplug") > Reported-by: syzbot+1a517ccfcbc6a7ab0f82@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/all/67bcea51.050a0220.bbfd1.0096.GAE@google.com/ > Cc: > Co-developed-by: Herbert Xu > Signed-off-by: Herbert Xu > Signed-off-by: Yosry Ahmed > Acked-by: Herbert Xu Looks good to me: Reviewed-by: Chengming Zhou Thanks! > --- > > v1 -> v2: > - Explained the problem more clearly in the commit message. > - Moved all freeing calls outside the lock critical section. > v1: https://lore.kernel.org/all/Z72FJnbA39zWh4zS@gondor.apana.org.au/ > > --- > mm/zswap.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index ac9d299e7d0c1..adf745c66aa1d 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -881,18 +881,32 @@ static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) > { > struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node); > struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu); > + struct acomp_req *req; > + struct crypto_acomp *acomp; > + u8 *buffer; > + > + if (IS_ERR_OR_NULL(acomp_ctx)) > + return 0; > > mutex_lock(&acomp_ctx->mutex); > - if (!IS_ERR_OR_NULL(acomp_ctx)) { > - if (!IS_ERR_OR_NULL(acomp_ctx->req)) > - acomp_request_free(acomp_ctx->req); > - acomp_ctx->req = NULL; > - if (!IS_ERR_OR_NULL(acomp_ctx->acomp)) > - crypto_free_acomp(acomp_ctx->acomp); > - kfree(acomp_ctx->buffer); > - } > + req = acomp_ctx->req; > + acomp = acomp_ctx->acomp; > + buffer = acomp_ctx->buffer; > + acomp_ctx->req = NULL; > + acomp_ctx->acomp = NULL; > + acomp_ctx->buffer = NULL; > mutex_unlock(&acomp_ctx->mutex); > > + /* > + * Do the actual freeing after releasing the mutex to avoid subtle > + * locking dependencies causing deadlocks. > + */ > + if (!IS_ERR_OR_NULL(req)) > + acomp_request_free(req); > + if (!IS_ERR_OR_NULL(acomp)) > + crypto_free_acomp(acomp); > + kfree(buffer); > + > return 0; > } >