linux-kernel - Re: [PATCH] block: switch to atomic

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: <20211208184416.GY16608@worktop.programming.kicks-ass.net>
Date:   Wed, 8 Dec 2021 19:44:16 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Jens Axboe <axboe@...nel.dk>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] block: switch to atomic_t for request references

On Wed, Dec 08, 2021 at 10:00:04AM -0800, Linus Torvalds wrote:
> On Wed, Dec 8, 2021 at 9:07 AM Peter Zijlstra <peterz@...radead.org> wrote:
> >
> > IOW, the effective range becomes: [1..INT_MIN], which is a bit
> > counter-intuitive, but then so is most of this stuff.
> 
> I'd suggest not codifying it too strictly, because the exact range at
> the upper end might depend on what is convenient for an architecture
> to do.
> 
> For x86, 'xadd' has odd semantics in that the flags register is about
> the *new* state, but the returned value is about the *old* state.

>From testing xadd had different flags from add; I've not yet looked at
the SDM to see what it said on the matter.

> That means that on x86, some things are cheaper to test based on the
> pre-inc/dec values, and other things are cheaper to test based on the
> post-inc/dec ones.
> 
> It's also why for "page->_mapcount" we have the "free" value being -1,
> not 0, and the refcount is "off by one". It makes the special cases of
> "increment from zero" and "decrement to zero" be very easy and
> straightforward to test for.
> 
> That might be an option for an "atomic_ref" type - with our existing
> "page_mapcount()" code being the thing we'd convert first, and make be
> the example for it.
> 
> I think it should also make the error cases be very easy to check for
> without extra tests. If you make "decrement from zero" be the "ok, now
> it's free", then that shows in the carry flag. But otherwise, if SF or
> OF is set, it's an error.  That means we can use the regular atomics
> and flags (although not "dec" and "inc", since we'd care about CF).
> 
> So on x86, I think "atomic_dec_ref()" could be
> 
>         lock subl $1,ptr
>         jc now_its_free
>         jl this_is_an_error
> 
> if we end up having that "off by one" model.
> 
> And importantly, "atomic_inc_ref()" would be just
> 
>         lock incl ptr
>         jle this_is_an_error
> 
> and this avoids us having to have the value in a register and test it
> separately.
> 
> So your suggestion is _close_, but note how you can't do the "inc_ofl"
> without that "off-by-one" model.
> 
> And again - I might have gotten the exact flag test instructions
> wrong. That's what you get for not actually doing serious assembly
> language for a couple of decades.

Yeah; I don't have it all in-cache either; I'll go through it tomorrow
or something to see what I can make of it.

Meanwhile I did send out what I had.