[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEXv5_jdQM0X1XNtxNVJi4Ftf8w4P4WN6sy_MK64=0Gmo8oWKQ@mail.gmail.com>
Date: Mon, 3 Oct 2016 18:49:01 -0400
From: David Windsor <dwindsor@...il.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: kernel-hardening@...ts.openwall.com, Kees Cook <keescook@...omium.org>,
Elena Reshetova <elena.reshetova@...el.com>, Hans Liljestrand <ishkamiel@...il.com>
Subject: Re: [RFC PATCH 12/13] x86: x86 implementation for HARDENED_ATOMIC
On Mon, Oct 3, 2016 at 3:27 PM, Dave Hansen <dave.hansen@...el.com> wrote:
> On 10/02/2016 11:41 PM, Elena Reshetova wrote:
>> static __always_inline void atomic_add(int i, atomic_t *v)
>> {
>> - asm volatile(LOCK_PREFIX "addl %1,%0"
>> + asm volatile(LOCK_PREFIX "addl %1,%0\n"
>> +
>> +#ifdef CONFIG_HARDENED_ATOMIC
>> + "jno 0f\n"
>> + LOCK_PREFIX "subl %1,%0\n"
>> + "int $4\n0:\n"
>> + _ASM_EXTABLE(0b, 0b)
>> +#endif
>> +
>> + : "+m" (v->counter)
>> + : "ir" (i));
>> +}
>
> Rather than doing all this assembly and exception stuff, could we just do:
>
> static __always_inline void atomic_add(int i, atomic_t *v)
> {
> if (atomic_add_unless(v, a, INT_MAX))
> BUG_ON_OVERFLOW_FOO()...
> }
>
> That way, there's also no transient state where somebody can have
> observed the overflow before it is fixed up. Granted, this
> cmpxchg-based operation _is_ more expensive than the fast-path locked addl.
I'm not opposed to this, as this would allow us to eliminate the race
on x86 and this doesn't really change how things work on a fundamental
level here. The overflow detection mechanism essentially remains the
same: __atomic_add_unless still would use exception dispatching as the
means of signaling that an overflow has occurred:
static __always_inline int __atomic_add_unless(atomic_t *v, int a, int u)
{
int c, old, new;
c = atomic_read(v);
for (;;) {
if (unlikely(c == (u)))
break;
asm volatile("addl %2,%0\n"
#ifdef CONFIG_HARDENED_ATOMIC
"jno 0f\n"
"subl %2,%0\n"
"int $4\n0:\n"
_ASM_EXTABLE(0b, 0b)
#endif
: "=r" (new)
: "0" (c), "ir" (a));
old = atomic_cmpxchg((v), c, new);
if (likely(old == c))
break;
c = old;
}
return c;
}
I'm unsure about the performance implications of cmpxchg, though, so I
agree with Kees: we should subject this feature (and this series as a
whole) to benchmarking.
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.
