linux-kernel - Re: [RFC PATCH 5/6] zsmalloc: introduce handle mapping API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z5gpr-2u_bqiVpHa@google.com>
Date: Tue, 28 Jan 2025 00:49:51 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sergey Senozhatsky <senozhatsky@...omium.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Minchan Kim <minchan@...nel.org>,
	Johannes Weiner <hannes@...xchg.org>, Nhat Pham <nphamcs@...il.com>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 5/6] zsmalloc: introduce handle mapping API

On Tue, Jan 28, 2025 at 09:37:20AM +0900, Sergey Senozhatsky wrote:
> On (25/01/27 21:26), Yosry Ahmed wrote:
> > On Mon, Jan 27, 2025 at 04:59:30PM +0900, Sergey Senozhatsky wrote:
> > > Introduce new API to map/unmap zsmalloc handle/object.  The key
> > > difference is that this API does not impose atomicity restrictions
> > > on its users, unlike zs_map_object() which returns with page-faults
> > > and preemption disabled
> > 
> > I think that's not entirely accurate, see below.
> 
> Preemption is disabled via zspage-s rwlock_t - zs_map_object() returns
> with it being locked and it's being unlocked in zs_unmap_object().  Then
> the function disables pagefaults and per-CPU local lock (protects per-CPU
> vm-area) additionally disables preemption.

Right, I meant it does not always disable page faults.

> 
> > [..]
> > > @@ -1309,12 +1297,14 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
> > >  		goto out;
> > >  	}
> > >  
> > > -	/* this object spans two pages */
> > > -	zpdescs[0] = zpdesc;
> > > -	zpdescs[1] = get_next_zpdesc(zpdesc);
> > > -	BUG_ON(!zpdescs[1]);
> > > +	ret = area->vm_buf;
> > > +	/* disable page faults to match kmap_local_page() return conditions */
> > > +	pagefault_disable();
> > 
> > Is this accurate/necessary? I am looking at kmap_local_page() and I
> > don't see it. Maybe that's remnant from the old code using
> > kmap_atomic()?
> 
> No, this does not look accuare nor neccesary to me.  I asume that's from
> a very long time ago, but regardless of that I don't really understand
> why that API wants to resemblwe kmap_atomic() (I think that was the
> intention).  This interface if expected to be gone so I didn't want
> to dig into it and fix it.

My assumption has been that back when we were using kmap_atomic(), which
disables page faults, we wanted to make this API's behavior consistent
for users where or not we called kmap_atomic() -- so this makes sure it
always disables page faults.

Now that we switched to kmap_local_page(), which doesn't disable page
faults, this was left behind, ulitmately making the interface
inconsistent and contradicting the purpose of its existence.

This is 100% speculation on my end :)

Anyway, if this function will be removed soon then it's not worth
revisiting it now.