Message-ID: <SJ0PR11MB56780456B6808A54954D9EC9C9202@SJ0PR11MB5678.namprd11.prod.outlook.com>
Date: Tue, 19 Nov 2024 22:35:43 +0000
From: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
CC: Chengming Zhou <chengming.zhou@...ux.dev>, Johannes Weiner
	<hannes@...xchg.org>, Nhat Pham <nphamcs@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>, "usamaarif642@...il.com"
	<usamaarif642@...il.com>, "ryan.roberts@....com" <ryan.roberts@....com>,
	"21cnbao@...il.com" <21cnbao@...il.com>, "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>, "Feghali, Wajdi K" <wajdi.k.feghali@...el.com>,
	"Gopal, Vinodh" <vinodh.gopal@...el.com>, "Sridhar, Kanchana P"
	<kanchana.p.sridhar@...el.com>
Subject: RE: [PATCH v1] mm: zswap: Fix a potential memory leak in
 zswap_decompress().


> -----Original Message-----
> From: Yosry Ahmed <yosryahmed@...gle.com>
> Sent: Tuesday, November 19, 2024 11:51 AM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> Cc: Chengming Zhou <chengming.zhou@...ux.dev>; Johannes Weiner
> <hannes@...xchg.org>; Nhat Pham <nphamcs@...il.com>; linux-
> kernel@...r.kernel.org; linux-mm@...ck.org; usamaarif642@...il.com;
> ryan.roberts@....com; 21cnbao@...il.com; akpm@...ux-foundation.org;
> Feghali, Wajdi K <wajdi.k.feghali@...el.com>; Gopal, Vinodh
> <vinodh.gopal@...el.com>
> Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in
> zswap_decompress().
> 
> On Tue, Nov 19, 2024 at 11:42 AM Sridhar, Kanchana P
> <kanchana.p.sridhar@...el.com> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Yosry Ahmed <yosryahmed@...gle.com>
> > > Sent: Tuesday, November 19, 2024 11:27 AM
> > > To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> > > Cc: Chengming Zhou <chengming.zhou@...ux.dev>; Johannes Weiner
> > > <hannes@...xchg.org>; Nhat Pham <nphamcs@...il.com>; linux-
> > > kernel@...r.kernel.org; linux-mm@...ck.org; usamaarif642@...il.com;
> > > ryan.roberts@....com; Huang, Ying <ying.huang@...el.com>;
> > > 21cnbao@...il.com; akpm@...ux-foundation.org; Feghali, Wajdi K
> > > <wajdi.k.feghali@...el.com>; Gopal, Vinodh <vinodh.gopal@...el.com>
> > > Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in
> > > zswap_decompress().
> > >
> > > On Tue, Nov 19, 2024 at 11:22 AM Sridhar, Kanchana P
> > > <kanchana.p.sridhar@...el.com> wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Yosry Ahmed <yosryahmed@...gle.com>
> > > > > Sent: Friday, November 15, 2024 1:49 PM
> > > > > To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> > > > > Cc: Chengming Zhou <chengming.zhou@...ux.dev>; Johannes Weiner
> > > > > <hannes@...xchg.org>; Nhat Pham <nphamcs@...il.com>; linux-
> > > > > kernel@...r.kernel.org; linux-mm@...ck.org;
> usamaarif642@...il.com;
> > > > > ryan.roberts@....com; Huang, Ying <ying.huang@...el.com>;
> > > > > 21cnbao@...il.com; akpm@...ux-foundation.org; Feghali, Wajdi K
> > > > > <wajdi.k.feghali@...el.com>; Gopal, Vinodh
> <vinodh.gopal@...el.com>
> > > > > Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in
> > > > > zswap_decompress().
> > > > >
> > > > > On Fri, Nov 15, 2024 at 1:14 PM Sridhar, Kanchana P
> > > > > <kanchana.p.sridhar@...el.com> wrote:
> > > > > >
> > > > > > Hi Chengming,
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Chengming Zhou <chengming.zhou@...ux.dev>
> > > > > > > Sent: Wednesday, November 13, 2024 11:24 PM
> > > > > > > To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>;
> Johannes
> > > > > Weiner
> > > > > > > <hannes@...xchg.org>
> > > > > > > Cc: Nhat Pham <nphamcs@...il.com>; Yosry Ahmed
> > > > > > > <yosryahmed@...gle.com>; linux-kernel@...r.kernel.org; linux-
> > > > > > > mm@...ck.org; usamaarif642@...il.com;
> ryan.roberts@....com;
> > > > > Huang,
> > > > > > > Ying <ying.huang@...el.com>; 21cnbao@...il.com; akpm@...ux-
> > > > > > > foundation.org; Feghali, Wajdi K <wajdi.k.feghali@...el.com>;
> Gopal,
> > > > > Vinodh
> > > > > > > <vinodh.gopal@...el.com>
> > > > > > > Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory leak in
> > > > > > > zswap_decompress().
> > > > > > >
> > > > > > > Hello,
> > > > > > >
> > > > > > > On 2024/11/14 14:37, Sridhar, Kanchana P wrote:
> > > > > > > >
> > > > > > > >> -----Original Message-----
> > > > > > > >> From: Johannes Weiner <hannes@...xchg.org>
> > > > > > > >> Sent: Wednesday, November 13, 2024 9:12 PM
> > > > > > > >> To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> > > > > > > >> Cc: Nhat Pham <nphamcs@...il.com>; Yosry Ahmed
> > > > > > > >> <yosryahmed@...gle.com>; linux-kernel@...r.kernel.org;
> linux-
> > > > > > > >> mm@...ck.org; chengming.zhou@...ux.dev;
> > > > > usamaarif642@...il.com;
> > > > > > > >> ryan.roberts@....com; Huang, Ying <ying.huang@...el.com>;
> > > > > > > >> 21cnbao@...il.com; akpm@...ux-foundation.org; Feghali,
> Wajdi K
> > > > > > > >> <wajdi.k.feghali@...el.com>; Gopal, Vinodh
> > > <vinodh.gopal@...el.com>
> > > > > > > >> Subject: Re: [PATCH v1] mm: zswap: Fix a potential memory
> leak in
> > > > > > > >> zswap_decompress().
> > > > > > > >>
> > > > > > > >> On Thu, Nov 14, 2024 at 01:56:16AM +0000, Sridhar, Kanchana
> P
> > > > > wrote:
> > > > > > > >>> So my question was, can we prevent the migration to a
> different
> > > cpu
> > > > > > > >>> by relinquishing the mutex lock after this conditional
> > > > > > > >>
> > > > > > > >> Holding the mutex doesn't prevent preemption/migration.
> > > > > > > >
> > > > > > > > Sure, however, is this also applicable to holding the mutex of a
> per-
> > > cpu
> > > > > > > > structure obtained via raw_cpu_ptr()?
> > > > > > >
> > > > > > > Yes, unless you use migration_disable() or cpus_read_lock() to
> protect
> > > > > > > this section.
> > > > > >
> > > > > > Ok.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Would holding the mutex prevent the acomp_ctx of the cpu prior
> to
> > > > > > > > the migration (in the UAF scenario you described) from being
> > > deleted?
> > > > > > >
> > > > > > > No, cpu offline can kick in anytime to free the acomp_ctx->buffer.
> > > > > > >
> > > > > > > >
> > > > > > > > If holding the per-cpu acomp_ctx's mutex isn't sufficient to
> prevent
> > > the
> > > > > > > > UAF, I agree, we might need a way to prevent the acomp_ctx
> from
> > > being
> > > > > > > > deleted, e.g. with refcounts as you've suggested, or to not use
> the
> > > > > > >
> > > > > > > Right, refcount solution from Johannes is very good IMHO.
> > > > > > >
> > > > > > > > acomp_ctx at all for the check, instead use a boolean.
> > > > > > >
> > > > > > > But this is not enough to just avoid using acomp_ctx for the check,
> > > > > > > the usage of acomp_ctx inside the mutex is also UAF, since cpu
> offline
> > > > > > > can kick in anytime to free the acomp_ctx->buffer.
> > > > > >
> > > > > > I see. How would the refcounts work? Would this add latency to
> zswap
> > > > > > ops? In low memory situations, could the cpu offlining code over-ride
> > > > > > the refcounts?
> > > > >
> > > > > I think what Johannes meant is that the zswap compress/decompress
> > > > > paths grab a ref on the acomp_ctx before using it, and the CPU
> > > > > offlining code only drops the initial ref, and does not free the
> > > > > buffer directly. The buffer is only freed when the ref drops to zero.
> > > > >
> > > > > I am not familiar with CPU hotplug, would it be simpler if we have a
> > > > > wrapper like get_acomp_ctx() that disables migration or calls
> > > > > cpus_read_lock() before grabbing the per-CPU acomp_ctx? A similar
> > > > > wrapper, put_acompt_ctx() will be used after we are done using the
> > > > > acomp_ctx.
> > > >
> > > > Would it be sufficient to add a check for mutex_is_locked() in
> > > > zswap_cpu_comp_dead() and if this returns true, to exit without
> deleting
> > > > the acomp?
> > >
> > > I don't think this works. First of all, it's racy. It's possible the
> > > mutex gets locked after we check mutex_is_locked() but before we
> > > delete the acomp_ctx. Also, if we find that the mutex is locked, then
> > > we do nothing and essentially leak the memory.
> >
> > Yes, this would assume the cpu offlining code retries at some interval,
> > which could prevent the memory leak.
> 
> I am not sure about that, but even so, it wouldn't handle the first
> scenario where the mutex gets locked after we check mutex_is_locked().
> 
> >
> > >
> > > Second, and probably more important, this only checks if anyone is
> > > currently holding the mutex. What about tasks that may be sleeping
> > > waiting for the mutex to be unlocked? The mutex will be deleted from
> > > under them as well.
> >
> > Wouldn't this and the race described above, also be issues for the
> > refcount based approach?
> 
> I don't think so, at least if implemented correctly. There are a lot
> of examples around the kernel that use RCU + refcounts for such use
> cases. I think there are also some examples in kernel docs.
> 
> That being said, I am wondering if we can get away with something
> simpler like holding the cpus read lock or disabling migration as I
> suggested earlier, but I am not quite sure.

Another idea to consider is how zsmalloc avoids this issue through
its use of the local_lock() on the per-cpu mapping area. This disables
preemption from zs_map_object() through zs_unmap_object().
Would changing the acomp_ctx's mutex to a local_lock solve the
problem?

> 
> >
> > Also, I am wondering if the mutex design already handles cases where
> > tasks are sleeping, waiting for a mutex that disappears?
> 
> I don't believe so. It doesn't make sense for someone to free a mutex
> while someone is waiting for it. How would the waiter know if the
> memory backing the mutex was freed?

Thanks Yosry, all good points. There would need to be some sort of
arbiter (for e.g., the cpu offlining code) that would reschedule tasks
running on a cpu before shutting it down, which could address
this specific issue. I was thinking these are not problems unique to
zswap's per-cpu acomp_ctx->mutex wrt the offlining?

Thanks,
Kanchana

Powered by blists - more mailing lists