Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 3 May 2023 17:03:47 +0000
In-Reply-To: <ZFGCy1d/wM+JC7xS@P9FQF9L96D.corp.robot.car>
Mime-Version: 1.0
References: <20230502160839.361544-1-roman.gushchin@linux.dev>
 <CAJD7tkZ7Vnk6J9cJoj66wAUzmSeAopFpuXrtxZCsCtPCvvwzuQ@mail.gmail.com> <ZFGCy1d/wM+JC7xS@P9FQF9L96D.corp.robot.car>
Message-ID: <20230503170347.ldrrtenh57trfpdy@google.com>
Subject: Re: [PATCH v2 1/2] mm: kmem: fix a NULL pointer dereference in obj_stock_flush_required()
From:   Shakeel Butt <shakeelb@google.com>
To:     Roman Gushchin <roman.gushchin@linux.dev>
Cc:     Yosry Ahmed <yosryahmed@google.com>, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Muchun Song <muchun.song@linux.dev>,
        linux-kernel@vger.kernel.org,
        syzbot+774c29891415ab0fd29d@syzkaller.appspotmail.com,
        Dmitry Vyukov <dvyukov@google.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.