[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEXv5_hXQxr=n_CPVKiqVjVwXU5FdHNhNfMDeygFOb30BcuWGw@mail.gmail.com>
Date: Thu, 19 Jan 2017 10:20:41 -0500
From: David Windsor <dwindsor@...il.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Eric Biggers <ebiggers3@...il.com>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>,
"keescook@...omium.org" <keescook@...omium.org>, "arnd@...db.de" <arnd@...db.de>,
"tglx@...utronix.de" <tglx@...utronix.de>, "mingo@...hat.com" <mingo@...hat.com>,
"h.peter.anvin@...el.com" <h.peter.anvin@...el.com>, "will.deacon@....com" <will.deacon@....com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>, Elena Reshetova <elena.reshetova@...el.com>
Subject: Re: [RFCv2 PATCH 00/18] refcount_t API + usage
On Thursday, January 19, 2017, Peter Zijlstra <peterz@...radead.org> wrote:
> On Wed, Jan 18, 2017 at 01:52:47PM -0800, Eric Biggers wrote:
>> There seems to be a lot of focus on converting things to use refcount_t
but much
>> less focus on providing a refcount_t implementation that actually meets
the
>> performance and security goals of the feature.
>
> And here you go again... :-(
>
> The refcount_t implementation does meet the security goals afaict, it
> has full saturation semantics, which means an overflow bug gets turned
> into a resource leak.
>
> That covers the entirely of the security goal. If there is more, you'll
> need to spell it out.
>
> As for performance, you didn't reply to my earlier email on the subject.
>
>> Notably, the proposed patchset
>> provides no information about why the proposed implementation was chosen
over
>> the PaX implementation (note that I'm talking about the actual
implementation of
>> safe reference counts, not the atomic_t/atomic_unchecked_t division)
which as
>> I've already mentioned is much more efficient (less bloated and faster)
while
>> still meeting the security goal.
>
> You again failed to reply to my last email on the subject. The initial
> PaX thing was broken as heck, only later did you mention it got fixed. I
> told you we could change to that for x86 if it could be proven to be
> equivalent.
>
> If you want to expedite matters, provide said proof.
>
> The scheme does not make sense for LL/SC based architectures though, so
> its not something that belongs in generic code.
>
>> I'm especially worried that people will be put
>> in a position where they need to take performance concerns into account
when
>> deciding whether to use refcount_t or not.
>
> First show a place where refcounting is performance critical, then we
> can see how much effort is required.
>
>> And the patch even still includes
>> the "don't allow incrementing a zero refcount" check which AFAICS is
bogus from
>> a security perspective.
>
> Because use-after-free isn't a security problem, right? Reference
> counting semantics are fairly clear that 0 means it is, or is going to
> be, free()'ed. How does allowing to increment at that point make any
> sense?
>
>> Even if you and Peter disagree with the comments that I and also PaX
Team have
>> made, the patch must at least explain the design decisions made.
>
> It was constructed as a generic atomic with saturation semantics because
> what was said PaX had was broken as hell (note, I have myself never
> looked at PaX code and have only seen what was sent me as derived code).
> If that later got fixed, or the derived code was buggy or whatever, your
> earlier email was the first I heard of that, and that was well after I
> wrote refcount_t.
>
> So the design decision was broken vs not-broken.
>
> Also, refcount_t is written using generic primitives (not arch
> specific), to avoid arch dependencies and provide a common
> implementation to determine semantics. That does not mean architectures
> cannot implement their own later on (matching semantics).
>
I think it's important to emphasize this: the generic refcount_t
implementation you provided is the default, arch-independent
implementation. The original PaX code had a similar system in which
default, arch-independent implementations were provided (defined in
include/asm-generic/atomic*.h, if memory serves). Individual architectures
overrode those definitions with their own, possibly optimized, versions.
Note that figuring out how this system of overridden implementations works
was pretty difficult, so we may want to investigate whether or not there's
an easier way to do what we want, or at least document where/how these
types' definitions are resolved. I have notes on this, and I bet Elena and
Hans do as well.
> Also, I agree GCC does a very poor job generating code from it. But
> again, I've not had a report where refcounting is performance critical.
> I've also been very busy with other work and haven't spend much if any
> time on this since your last email.
>
> If you want something done, contribute.
>
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
