linux-kernel - Re: [PATCH v2 1/2] mm: kmem: fix a NULL pointer dereference in obj_stock_flush

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]

Date:   Wed, 3 May 2023 17:03:47 +0000
From:   Shakeel Butt <shakeelb@...gle.com>
To:     Roman Gushchin <roman.gushchin@...ux.dev>
Cc:     Yosry Ahmed <yosryahmed@...gle.com>, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Muchun Song <muchun.song@...ux.dev>,
        linux-kernel@...r.kernel.org,
        syzbot+774c29891415ab0fd29d@...kaller.appspotmail.com,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [PATCH v2 1/2] mm: kmem: fix a NULL pointer dereference in obj_stock_flush_required()

On Tue, May 02, 2023 at 02:38:19PM -0700, Roman Gushchin wrote:
[...]
> > 
> > I believe all read accesses other than obj_stock_flush_required() are
> > done under the lock, so READ_ONCE() wouldn't be needed AFAICT. Having
> > READ_ONCE() only around the racy read can be useful to document the
> > racy read and differentiate it from others.
> > 
> > With that said, it's also inconvenient to keep track moving forward of
> > which reading sites are racy, and it may be simpler to just annotate
> > all readers with READ_ONCE().
> > 
> > I am not sure which approach is better, just thinking out loud.
> 
> Yeah, I wasn't sure either. I believe that all changes except the original
> READ_ONCE() are not leading to any meaningful asm changes, so it's a matter
> of taste.
> 
> The reason why I went with the "change them all" approach:
> reads without READ_ONCE() and subsequent writes with WRITE_ONCE()
> inside a single function looked really weird.
> 

Change them all is the right approach. This code will evolve in future
and having partial tagging will cause confusion or might be missed
altogether. Also the automated tools prefer change them all.