linux-kernel - Re: [PATCH v9 10/17] refcount: introduce __refcount_{add|inc}_not_zero

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: <20250115093955.GW5388@noisy.programming.kicks-ass.net>
Date: Wed, 15 Jan 2025 10:39:55 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: Hillf Danton <hdanton@...a.com>, akpm@...ux-foundation.org,
	willy@...radead.org, hannes@...xchg.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH v9 10/17] refcount: introduce
 __refcount_{add|inc}_not_zero_limited

On Sat, Jan 11, 2025 at 09:11:52AM -0800, Suren Baghdasaryan wrote:
> On Sat, Jan 11, 2025 at 4:13 AM Hillf Danton <hdanton@...a.com> wrote:
> >
> > On Sat, 11 Jan 2025 01:59:41 -0800 Suren Baghdasaryan <surenb@...gle.com>
> > > On Fri, Jan 10, 2025 at 10:32 PM Hillf Danton <hdanton@...a.com> wrote:
> > > > On Fri, 10 Jan 2025 20:25:57 -0800 Suren Baghdasaryan <surenb@...gle.com>
> > > > > -bool __refcount_add_not_zero(int i, refcount_t *r, int *oldp)
> > > > > +bool __refcount_add_not_zero_limited(int i, refcount_t *r, int *oldp,
> > > > > +                                  int limit)
> > > > >  {
> > > > >       int old = refcount_read(r);
> > > > >
> > > > >       do {
> > > > >               if (!old)
> > > > >                       break;
> > > > > +
> > > > > +             if (statically_true(limit == INT_MAX))
> > > > > +                     continue;
> > > > > +
> > > > > +             if (i > limit - old) {
> > > > > +                     if (oldp)
> > > > > +                             *oldp = old;
> > > > > +                     return false;
> > > > > +             }
> > > > >       } while (!atomic_try_cmpxchg_relaxed(&r->refs, &old, old + i));
> > > >
> > > > The acquire version should be used, see atomic_long_try_cmpxchg_acquire()
> > > > in kernel/locking/rwsem.c.
> > >
> > > This is how __refcount_add_not_zero() is already implemented and I'm
> > > only adding support for a limit. If you think it's implemented wrong
> > > then IMHO it should be fixed separately.
> > >
> > Two different things - refcount has nothing to do with locking at the
> > first place, while what you are adding to the mm directory is something
> > that replaces rwsem, so from the locking POV you have to mark the
> > boundaries of the locking section.
> 
> I see your point. I think it's a strong argument to use atomic
> directly instead of refcount for this locking. I'll try that and see
> how it looks. Thanks for the feedback!

Sigh; don't let hillf confuse you. *IF* you need an acquire it will be
in the part where you wait for readers to go away. But even there, think
about what you're serializing against. Readers don't typically modify
things.

And modifications are fully serialized by mmap_sem^H^H^Hlock.