linux-kernel - Re: xarray reserve/release?

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: <20190220174333.GI8429@ziepe.ca>
Date:   Wed, 20 Feb 2019 10:43:33 -0700
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: xarray reserve/release?

On Wed, Feb 20, 2019 at 09:14:14AM -0800, Matthew Wilcox wrote:

> > void __xa_release(struct xarray *xa, unsigned long index)
> > {
> > 	XA_STATE(xas, xa, index);
> > 	void *curr;
> > 
> > 	curr = xas_load(&xas);
> > 	if (curr == XA_ZERO_ENTRY)
> > 		xas_store(&xas, NULL);
> > }
> > 
> > ?
> 
> I decided to instead remove the magic from xa_cmpxchg().  I used
> to prohibit any internal entry being passed to the regular API, but
> I recently changed that with 76b4e5299565 ("XArray: Permit storing
> 2-byte-aligned pointers").  Now that we can pass XA_ZERO_ENTRY, I
> think this all makes much more sense.

Except that for allocating arrays xa_cmpxchg and xa_store now do
different things with NULL. Not necessarily bad, but if you have this
ABI variation it should be mentioned in the kdoc comment.

This is a bit worrysome though:

                curr = xas_load(&xas);
-               if (curr == XA_ZERO_ENTRY)
-                       curr = NULL;
                if (curr == old) {

It means any cmpxchg user has to care explicitly about the possibility
for true-NULL vs reserved. Seems like a difficult API.

What about writing it like this:

   if ((curr == XA_ZERO_ENTRY && old == NULL) || curr == old)

? I can't think of a use case to cmpxchg against real-null only.

And here:
                        xas_store(&xas, entry);
-                       if (xa_track_free(xa))
+                       if (xa_track_free(xa) && !old)
                                xas_clear_mark(&xas, XA_FREE_MARK);

Should this be

    if (xa_track_free(xa) && entry && !old)

? Ie we don't want to clear the XA_FREE_MARK if we just wrote NULL

Also I would think !curr is clearer? I assume the point is to not pay
the price of xas_clear_mark if we already know the index stored is
marked?

> > Also, I wonder if xa_reserve() is better written as as
> > 
> >        xa_cmpxchg(xa, index, NULL, XA_ZERO_ENTRY)
> > 
> > Bit clearer what is going on..
> 
> Yes, I agree.  I've pushed a couple of new commits to
> http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray

That looks really readable now that reserve and release are tidy
paired operations.

Thanks,
Jason