Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1626665029-49104-1-git-send-email-xiyuyang19@fudan.edu.cn>
 <20210720160127.ac5e76d1e03a374b46f25077@linux-foundation.org>
 <20210819132131.GA15779@willie-the-truck> <YR5ldaQvAnCKMnkk@hirez.programming.kicks-ass.net>
 <YR52igt/lJ7gQqOG@hirez.programming.kicks-ass.net>
In-Reply-To: <YR52igt/lJ7gQqOG@hirez.programming.kicks-ass.net>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 19 Aug 2021 12:09:37 -0700
Message-ID: <CAHk-=wh_vEzmYnMufOa=03WAU=DRM5+n6uZy=dVtJERFJm3Q-Q@mail.gmail.com>
Subject: Re: [PATCH] mm/rmap: Convert from atomic_t to refcount_t on anon_vma->refcount
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Will Deacon <will@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Borislav Petkov <bp@alien8.de>,
        Xiyu Yang <xiyuyang19@fudan.edu.cn>,
        Alistair Popple <apopple@nvidia.com>,
        Yang Shi <shy828301@gmail.com>,
        Shakeel Butt <shakeelb@google.com>,
        Hugh Dickins <hughd@google.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>, yuanxzhang@fudan.edu.cn,
        Xin Tan <tanxin.ctf@gmail.com>,
        Will Deacon <will.deacon@arm.com>,
        David Howells <dhowells@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Aug 19, 2021 at 8:21 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> If we can skip the OF... we can do something like this:

Honestly, I think a lot of the refcount code is questionable. It was
absolutely written with no care for performance AT ALL.

I'm not sure it helps to then add arch-specific code for it without
thinking it through a _lot_ first.

It might be better to just have a "atomic_t with overflow handling" in
general, exactly because the refcount_t was designed and written
without any regard for code that cares about performance.

> static inline bool refcount_dec_and_test(refcount_t *r)
> {
>         asm_volatile_goto (LOCK_PREFIX "decl %[var]\n\t"
>                            "jz %l[cc_zero]\n\t"
>                            "jns 1f\n\t"

I think you can use "jl" for the bad case.

You want to fail if the old value was negative, or if the old value
was less than what you subtracted. That's literally the "less than"
operator.

I think it's better to handle that case out-of-line than play games
with UD, though - this is going to be the rare case, the likelihood
that we get the fixup wrong is just too big. Once it's out-of-line
it's not as critical any more, even if it does add to the size of the
code.

So if we do this, I think it should be something like

   static inline __must_check bool refcount_dec_and_test(refcount_t *r)
   {
        asm_volatile_goto (LOCK_PREFIX "decl %[var]\n\t"
                "jz %l[cc_zero]\n\t"
                "jl %l[cc_error]"
                : : [var] "m" (r->refs.counter)
                : "memory" : cc_zero, cc_error);

        return false;

   cc_zero:
        return true;
   cc_error:
        refcount_warn_saturate(r, REFCOUNT_SUB_UAF);
        return false;
   }

and we can discuss whether we could improve on the
refcount_warn_saturate() separately.

But see above: maybe just make this a separate "careful atomic_t",
with the option to panic-on-overflow. So then we could get rid of
refcount_warn_saturate() enmtirely above, and instead just have a
(compile-time option) BUG() case, with the non-careful version just
being our existing atomic_dec_and_test.

Giving people who want a smaller kernel the option to do so, while
giving people who want checking the option too.

                Linus