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 722D8E77197 for ; Tue, 7 Jan 2025 23:26:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CED696B008A; Tue, 7 Jan 2025 18:26:33 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C9C9A6B008C; Tue, 7 Jan 2025 18:26:33 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B3D1A6B0092; Tue, 7 Jan 2025 18:26:33 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 8F8156B008A for ; Tue, 7 Jan 2025 18:26:33 -0500 (EST) Received: from smtpin24.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 187ACB04D3 for ; Tue, 7 Jan 2025 23:26:33 +0000 (UTC) X-FDA: 82982242266.24.0D88901 Received: from mail-vs1-f47.google.com (mail-vs1-f47.google.com [209.85.217.47]) by imf19.hostedemail.com (Postfix) with ESMTP id 342D51A000F for ; Tue, 7 Jan 2025 23:26:31 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=kernel.org (policy=quarantine); spf=pass (imf19.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.217.47 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736292391; 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; bh=fTD+Bq3GZL1MxaBmfdHPIPo5L73mR014hM+07a3/nK0=; b=yKhS/9nmOoyjgZKT4FA5XMKcUDIY3aEXplsXJ3d8seWMXJSHNODtfOms+mhmeNb/3EY4v2 EGFwELnw3oauLfbM46Cs674mby1hDx25uflRU7HFCI+WXZMNOYKWWeh7fjsJbzKLRKYOv4 7Mcg7UzkwU4VC3iQYdJQDuFB2Vk9XLM= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736292391; a=rsa-sha256; cv=none; b=N0Tyn1UHK7ZQ6TZm2smsXoLlUpXPCTjzEMzYeFYRHkpufdr81BOmK/QjziGBPfr9zjp4xq uEHayd5sm2POi5sfM2I3JWuk41MyJ0Rx65z10u5pb4HxE4eQZ5x5XJ7qrIgAHJIT3BXEQI oHhXQRK74LaVHFWgBmfzLJZHK/ppF4k= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=kernel.org (policy=quarantine); spf=pass (imf19.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.217.47 as permitted sender) smtp.mailfrom=21cnbao@gmail.com Received: by mail-vs1-f47.google.com with SMTP id ada2fe7eead31-4aff04f17c7so206252137.0 for ; Tue, 07 Jan 2025 15:26:30 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736292390; x=1736897190; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=fTD+Bq3GZL1MxaBmfdHPIPo5L73mR014hM+07a3/nK0=; b=v7rEXN3JAL0prANQ1EDJrt9tEmZz8zDSuV1YZmeC7YdP5WcbmjLVFcAa4088fmOMDc hwtJZC3gRVpmoY+7ALj34h2HKbsZyINrk411FJi1cRzKkGD5/o8zULBgSlLyJeQtlj1U Y6JF7dxIa76neWl5Kg0TqGCo5ia/+eH59vTTPiK8vgprrcIg1xEtc4TaFYeE16h1vyxL /AQ5+IrzzuxU5z+l+LpesLWziH0+JbBEfHcSusr2mqDe6L0jAItfeClDkYuQr7ryTNUO T4rgyloRX/yDzzvMJma4rtu59njM2MX1VunPTkk33fAiKNjl5Kv+BmQMNH/HWiWcosHp f+3A== X-Forwarded-Encrypted: i=1; AJvYcCXyFJcWObBNcKuyhLe/pxQ9lD8O7xj8B2VucFKJ01XS0omcVDW7Vj7EBsRKPNr8ScM/JiE+IbZZIQ==@kvack.org X-Gm-Message-State: AOJu0YzzedUQMc+P+LExBaVZQjuaQWsCffrnr2XcpLyTACTHN0Xd3D3X +vkTXxxuAFp8wNIYFlCkqqNIUgCttERFAHiPdRFtauyxh39brfDBInj2MEFQkcZYtdIj7gWYmyG lKxAvRl3ePeBVgksXeV7ZVdc681M= X-Gm-Gg: ASbGncuVUlIalzKNqDyniY/lRl/2z40japZE6EuNyfL5EW50Bj3uFlecmmnQr1R6NPH 6ifaYjSplk0BSnq1WLt2P6kqByUK0Iq9ZZg1k6InjkE34l1IwTCB3Boj818u4FtLpxySsBrM= X-Google-Smtp-Source: AGHT+IEaO4P+lfOXcwWKAGlMUPWE2nYgb7AHRUM5xHT6kKgWYGgYWQRC0h6RgnxpXsDDKr4gfX4uYJy0P+c1KyPDY0s= X-Received: by 2002:a67:ce16:0:b0:4b1:f903:98d3 with SMTP id ada2fe7eead31-4b3c1e8395emr3138224137.7.1736292390220; Tue, 07 Jan 2025 15:26:30 -0800 (PST) MIME-Version: 1.0 References: <20250107222236.2715883-1-yosryahmed@google.com> <20250107222236.2715883-2-yosryahmed@google.com> In-Reply-To: From: Barry Song Date: Wed, 8 Jan 2025 12:26:19 +1300 X-Gm-Features: AbW1kvajFZJVoNT-AJnBwcg3e84jtNCCXGQtsdHJS74CdUWcsjCqGgheefRSmAA Message-ID: Subject: Re: [PATCH v2 2/2] mm: zswap: disable migration while using per-CPU acomp_ctx To: Yosry Ahmed Cc: Andrew Morton , Johannes Weiner , Nhat Pham , Chengming Zhou , Vitaly Wool , Sam Sun , Kanchana P Sridhar , linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 342D51A000F X-Rspam-User: X-Rspamd-Server: rspam07 X-Stat-Signature: ng46ga73jo1p3sy8qazoxrd5r5886grg X-Rspamd-Pre-Result: action=add header; module=dmarc; Action set by DMARC X-Rspam: Yes X-HE-Tag: 1736292391-174104 X-HE-Meta: U2FsdGVkX18qYIOuz4fH/BzDv4hf76aAvPthVWKJ9YZaWuHYLBkGCcBg2gkfF/Ixo70V/Bi9WWqO9bX/A7+oDdB8dtoJBk65NJXZqKWFJD62qPIVnJdTM/rFQ6AGxPEZ6qAqC86/VqtVkmPEIE3XOtay5Dki0c86booP+HKm41BB010eI+g3H8ALkumhbs6MnrUxSCp8U+MMGHdNZcDEhXx4FR+dlcab1681uhya1irjcfRTCKwUfWH6ouiFnDi7vo58MfM/dJfY4WQ93zIN8QrPisIpisA8gLmnGEdybk81URM2EuV5+94MjdbjHX0xsvTu5YKe2gFhezThRiHhpVWHjr/xXl3ZfwFL4OE5oIbKFUobBWUn13hhrxiODdRDiy2bSQbq+Q+OzM52saZMWbbMshdB2FzaC4tMMMMphOBMAw3I8m4SypCIa/qlCu0wKPIvxAEYnI1LD3SJpXNrgj4EZ31DfafQRKBWhRQYOUQQWLWkYG3z/lwRIaNDaezN/+DGbwTn+BOIH9PCOoSANBdqp+oqRbjm/H4u/lAEfTGp5Qc+KTDE7T+FfwIuQgM8Aj7Y3Jw8I49IL+3zLzVxV/9UNBXh2Y+7vLntBySkuvX2h0PtSd3ACdCKN18+CIIaczchVa+7ToNPnz35cwMrh1taVWeDQ9zJodH8YOTEpLZZusGOtBebZJ38Cbbbl1mSu/FST2uS/EVhTpUan4KG+xLz50EklYteKW+Ts4Oez1G1yuqzMwCDiapBtr0Fvn4usEgGxaajVRTTq/L1jGa3GLEXa15sLXN8jHJpx3JMv4woB0wz+FJVj3dSDG362F7IxK/FSpM8q7ls28La84EEBt55Jf3ptaRXUHB2cqUFO+mEL6//TgVjVrNTskRQkHrdMvrmviQK+ztjhoKLhCLmAY4eaMiDJ74MdnWx60uuKaPwwtGxOEGb+JfkcxlS+YyhI/2LfNQ5n16wvU+M+PI reaTZNb0 ofBtB7tEVJ7GJoUmLeo9jwgsLiM3j5/KdO1lMk5F0LVdSP7if4nrggTD99O2VZtaS8QLwbcpOhfSvlmRSV3vHy8X9YJ21YXqErHAV0Gkx8mq83sT/7ISDPS+kYbXLFfNAmnTjHRbgp1/s6cgt2tHxHlSw//yADkz9afEe/itn2E9K8dJXXh/w2IaWugZHLHZ+Gk2udX6I1LCK/vC04Vflqo80sxefJBgPiAiS/Z/g049asyYOOejyewlhMd2FGNDooIF6HjtZPNSz10L1VMvVK3cLJB0Hv5/aoJLZebRBPomGWxBrPBMyVx/PEkpel5Q5UtHS/5JdT14qU6Ye/Hy+mUoxhI1QxQETeo+FOuNXPz4f0Yd3zyEwAcQ2rFHrby59tA939dP9pQlvJV4bgLdH28DOxAjhEtiCSr8bfBrvlChEw+tmvbuDCr5dOKSp0kp/5vh+KDCXV+7wVb4Dt0wobdD0cA== 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 Wed, Jan 8, 2025 at 11:47=E2=80=AFAM Barry Song wrot= e: > > On Wed, Jan 8, 2025 at 11:22=E2=80=AFAM Yosry Ahmed wrote: > > > > In zswap_compress() and zswap_decompress(), the per-CPU acomp_ctx of th= e > > current CPU at the beginning of the operation is retrieved and used > > throughout. However, since neither preemption nor migration are disabl= ed, > > it is possible that the operation continues on a different CPU. > > > > If the original CPU is hotunplugged while the acomp_ctx is still in use= , > > we run into a UAF bug as the resources attached to the acomp_ctx are fr= eed > > during hotunplug in zswap_cpu_comp_dead(). > > > > The problem was introduced in commit 1ec3b5fe6eec ("mm/zswap: move to u= se > > crypto_acomp API for hardware acceleration") when the switch to the > > crypto_acomp API was made. Prior to that, the per-CPU crypto_comp was > > retrieved using get_cpu_ptr() which disables preemption and makes sure = the > > CPU cannot go away from under us. Preemption cannot be disabled with t= he > > crypto_acomp API as a sleepable context is needed. > > > > Commit 8ba2f844f050 ("mm/zswap: change per-cpu mutex and buffer to > > per-acomp_ctx") increased the UAF surface area by making the per-CPU > > buffers dynamic, adding yet another resource that can be freed from und= er > > zswap compression/decompression by CPU hotunplug. > > > > This cannot be fixed by holding cpus_read_lock(), as it is possible for > > code already holding the lock to fall into reclaim and enter zswap > > (causing a deadlock). It also cannot be fixed by wrapping the usage of > > acomp_ctx in an SRCU critical section and using synchronize_srcu() in > > zswap_cpu_comp_dead(), because synchronize_srcu() is not allowed in > > CPU-hotplug notifiers (see > > Documentation/RCU/Design/Requirements/Requirements.rst). > > > > This can be fixed by refcounting the acomp_ctx, but it involves > > complexity in handling the race between the refcount dropping to zero i= n > > zswap_[de]compress() and the refcount being re-initialized when the CPU > > is onlined. > > > > Keep things simple for now and just disable migration while using the > > per-CPU acomp_ctx to block CPU hotunplug until the usage is over. > > > > Fixes: 1ec3b5fe6eec ("mm/zswap: move to use crypto_acomp API for hardwa= re acceleration") > > Cc: > > Signed-off-by: Yosry Ahmed > > Reported-by: Johannes Weiner > > Closes: https://lore.kernel.org/lkml/20241113213007.GB1564047@cmpxchg.o= rg/ > > Reported-by: Sam Sun > > Closes: https://lore.kernel.org/lkml/CAEkJfYMtSdM5HceNsXUDf5haghD5+o2e7= Qv4OcuruL4tPg6OaQ@mail.gmail.com/ > > --- > > mm/zswap.c | 19 ++++++++++++++++--- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index f6316b66fb236..ecd86153e8a32 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -880,6 +880,18 @@ static int zswap_cpu_comp_dead(unsigned int cpu, s= truct hlist_node *node) > > return 0; > > } > > > > +/* Remain on the CPU while using its acomp_ctx to stop it from going o= ffline */ > > +static struct crypto_acomp_ctx *acomp_ctx_get_cpu(struct crypto_acomp_= ctx __percpu *acomp_ctx) > > +{ > > + migrate_disable(); > > I'm not entirely sure, but I feel it is quite unsafe. Allowing sleep > during migrate_disable() and > migrate_enable() would require the entire scheduler, runqueue, > waitqueue, and CPU > hotplug mechanisms to be aware that a task is pinned to a specific CPU. > > If there is no sleep during this period, it seems to be only a > runqueue issue=E2=80=94CPU hotplug can > wait for the task to be unpinned while it is always in runqueue. > However, if sleep is involved, > the situation becomes significantly more complex. After double-checking the scheduler's code, it seems fine. When a task is scheduled out, __schedule() will set its allowable cpu by: migrate_disable_switch(rq, prev); static void migrate_disable_switch(struct rq *rq, struct task_struct *p) { struct affinity_context ac =3D { .new_mask =3D cpumask_of(rq->cpu), .flags =3D SCA_MIGRATE_DISABLE, }; if (likely(!p->migration_disabled)) return; if (p->cpus_ptr !=3D &p->cpus_mask) return; /* * Violates locking rules! see comment in __do_set_cpus_allowed(). */ __do_set_cpus_allowed(p, &ac); } while woken-up, the previous cpu will be selected: /* * The caller (fork, wakeup) owns p->pi_lock, ->cpus_ptr is stable. */ static inline int select_task_rq(struct task_struct *p, int cpu, int wake_flags) { lockdep_assert_held(&p->pi_lock); if (p->nr_cpus_allowed > 1 && !is_migration_disabled(p)) cpu =3D p->sched_class->select_task_rq(p, cpu, wake_flags); else cpu =3D cpumask_any(p->cpus_ptr); ... } Anyway, not an expert :-) Hopefully, other experts can provide their input to confirm whether sleeping during migrate_disable() is all good. > > If static data doesn't consume much memory, it could be the simplest solu= tion. > > > + return raw_cpu_ptr(acomp_ctx); > > +} > > + > > +static void acomp_ctx_put_cpu(void) > > +{ > > + migrate_enable(); > > +} > > + > > static bool zswap_compress(struct page *page, struct zswap_entry *entr= y, > > struct zswap_pool *pool) > > { > > @@ -893,8 +905,7 @@ static bool zswap_compress(struct page *page, struc= t zswap_entry *entry, > > gfp_t gfp; > > u8 *dst; > > > > - acomp_ctx =3D raw_cpu_ptr(pool->acomp_ctx); > > - > > + acomp_ctx =3D acomp_ctx_get_cpu(pool->acomp_ctx); > > mutex_lock(&acomp_ctx->mutex); > > > > dst =3D acomp_ctx->buffer; > > @@ -950,6 +961,7 @@ static bool zswap_compress(struct page *page, struc= t zswap_entry *entry, > > zswap_reject_alloc_fail++; > > > > mutex_unlock(&acomp_ctx->mutex); > > + acomp_ctx_put_cpu(); > > return comp_ret =3D=3D 0 && alloc_ret =3D=3D 0; > > } > > > > @@ -960,7 +972,7 @@ static void zswap_decompress(struct zswap_entry *en= try, struct folio *folio) > > struct crypto_acomp_ctx *acomp_ctx; > > u8 *src; > > > > - acomp_ctx =3D raw_cpu_ptr(entry->pool->acomp_ctx); > > + acomp_ctx =3D acomp_ctx_get_cpu(entry->pool->acomp_ctx); > > mutex_lock(&acomp_ctx->mutex); > > > > src =3D zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO); > > @@ -990,6 +1002,7 @@ static void zswap_decompress(struct zswap_entry *e= ntry, struct folio *folio) > > > > if (src !=3D acomp_ctx->buffer) > > zpool_unmap_handle(zpool, entry->handle); > > + acomp_ctx_put_cpu(); > > } > > > > /********************************* > > -- > > 2.47.1.613.gc27f4b7a9f-goog > > > Thanks Barry