linux-kernel - Re: [PATCH] zsmalloc: fix a race with deferred

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] [day] [month] [year] [list]

Message-ID: <Y9ncjPEFEU33ENVq@google.com>
Date:   Wed, 1 Feb 2023 12:29:16 +0900
From:   Sergey Senozhatsky <senozhatsky@...omium.org>
To:     Nhat Pham <nphamcs@...il.com>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        akpm@...ux-foundation.org, hannes@...xchg.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, minchan@...nel.org,
        ngupta@...are.org, sjenning@...hat.com, ddstreet@...e.org,
        vitaly.wool@...sulko.com, kernel-team@...a.com
Subject: Re: [PATCH] zsmalloc: fix a race with deferred_handles storing

On (23/01/31 18:28), Nhat Pham wrote:
> > On (23/01/10 15:17), Nhat Pham wrote:
> > [..]
> > >  #ifdef CONFIG_ZPOOL
> > > +static void restore_freelist(struct zs_pool *pool, struct size_class *class,
> > > +             struct zspage *zspage)
> > > +{
> > > +     unsigned int obj_idx = 0;
> > > +     unsigned long handle, off = 0; /* off is within-page offset */
> > > +     struct page *page = get_first_page(zspage);
> > > +     struct link_free *prev_free = NULL;
> > > +     void *prev_page_vaddr = NULL;
> > > +
> > > +     /* in case no free object found */
> > > +     set_freeobj(zspage, (unsigned int)(-1UL));
> >
> > I'm not following this. I see how -1UL works for link_free, but this
> > cast of -1UL to 4 bytes looks suspicious.
> 
> (resending this since I forgot to forward this to other recipients)
> 
> It is a bit convoluted indeed. But the idea is that for the last object,
> the last link is given by:
> 
> link->next = -1UL << OBJ_TAG_BITS
> 
> And at malloc time, we update freeobj as follows
> set_freeobj(zspage, link->next >> OBJ_TAG_BITS);
> 
> Which means the freeobj value would be set to something like this:
> (-1UL << OBJ_TAG_BITS) >> OBJ_TAG_BITS

Oh, good point. I see what you did there.

> I want to emulate this here (i.e in the case we have no free object).

Makes sense.

> As for the casting, I believe set_freeobj requires an unsigned int for
> the second field.
> 
> Alternatively, to be 100% safe, we can do something like this:
> (unsigned int)((-1UL << OBJ_TAG_BITS) >> OBJ_TAG_BITS)
> 
> But I think I got the same result as just (unsigned int)(-1UL)

Yeah, I guess they should be the same, as we take the lower 4 bytes
only.