linux-kernel - Re: [PATCH v5 1/3] drm: Use XArray instead of IDR for minors

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: <CAFCwf112mZcB6h1QZyO9nKYnDuMB2ceD7v_myL7PycKQv6uLMA@mail.gmail.com>
Date:   Mon, 7 Nov 2022 21:18:38 +0200
From:   Oded Gabbay <ogabbay@...nel.org>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Michał Winiarski <michal.winiarski@...el.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        intel-gfx@...ts.freedesktop.org, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Simon Ser <contact@...rsion.fr>
Subject: Re: [PATCH v5 1/3] drm: Use XArray instead of IDR for minors

On Mon, Nov 7, 2022 at 6:20 PM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Sun, Nov 06, 2022 at 04:51:39PM +0200, Oded Gabbay wrote:
> > I tried executing the following code in a dummy driver I wrote:
>
> You don't need to write a dummy driver; you can write test-cases
> entirely in userspace.  Just add them to lib/test_xarray.c and then
> make -C tools/testing/radix-tree/
>
> > static DEFINE_XARRAY_ALLOC(xa_dummy);
> > void check_xa(void *pdev)
> > {
> >   void *entry;
> >   int ret, index;
> >
> >   ret = xa_alloc(&xa_dummy, &index, NULL, XA_LIMIT(0, 63), GFP_NOWAIT);
> >   if (ret < 0)
> >       return ret;
> >
> >   entry = xa_cmpxchg(&xa_dummy, index, NULL, pdev, GFP_KERNEL);
> >   if (xa_is_err(entry))
> >        return;
> >
> >   xa_lock(&xa_dummy);
> >   xa_dev = xa_load(&xa_dummy, index);
> >   xa_unlock(&xa_dummy);
> > }
> >
> > And to my surprise xa_dev is always NULL, when it should be pdev.
> > I believe that because we first allocate the entry with NULL, it is
> > considered a "zero" entry in the XA.
> > And when we replace it, this attribute doesn't change so when we do
> > xa_load, the xa code thinks the entry is a "zero" entry and returns
> > NULL.
>
> There's no "attribute" to mark a zero entry.  It's just a zero entry.
> The problem is that you're cmpxchg'ing against NULL, and it's not NULL,
> it's the ZERO entry.  This is even documented in the test code:
>
>         /* cmpxchg sees a reserved entry as ZERO */
>         XA_BUG_ON(xa, xa_reserve(xa, 12345678, GFP_KERNEL) != 0);
>         XA_BUG_ON(xa, xa_cmpxchg(xa, 12345678, XA_ZERO_ENTRY,
>                                 xa_mk_value(12345678), GFP_NOWAIT) != NULL);
>
> I'm not quite sure why you're using xa_cmpxchg() here anyway; if you
> allocated it, it's yours and you can just xa_store() to it.
>
Hi Matthew,
Thanks for the explanation. I believe Michal's will fix his original
patch and I'll take that code.

Oded