Messages in this thread | | | From | Dmitry Vyukov <> | Date | Tue, 21 Mar 2017 19:06:14 +0100 | Subject | Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations |
| |
On Tue, Mar 21, 2017 at 11:41 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Tue, Mar 21, 2017 at 12:25:06PM +0300, Andrey Ryabinin wrote: >> On 03/20/2017 08:17 PM, Mark Rutland wrote: >> > Hi, >> > >> > On Tue, Mar 14, 2017 at 08:24:13PM +0100, Dmitry Vyukov wrote: >> >> /** >> >> - * atomic_read - read atomic variable >> >> + * arch_atomic_read - read atomic variable >> >> * @v: pointer of type atomic_t >> >> * >> >> * Atomically reads the value of @v. >> >> */ >> >> -static __always_inline int atomic_read(const atomic_t *v) >> >> +static __always_inline int arch_atomic_read(const atomic_t *v) >> >> { >> >> - return READ_ONCE((v)->counter); >> >> + /* >> >> + * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN >> >> + * instrumentation. Double instrumentation is unnecessary. >> >> + */ >> >> + return READ_ONCE_NOCHECK((v)->counter); >> >> } >> > >> > Just to check, we do this to avoid duplicate reports, right? >> > >> > If so, double instrumentation isn't solely "unnecessary"; it has a >> > functional difference, and we should explicitly describe that in the >> > comment. >> > >> > ... or are duplicate reports supressed somehow? >> >> They are not suppressed yet. But I think we should just switch kasan >> to single shot mode, i.e. report only the first error. Single bug >> quite often has multiple invalid memory accesses causing storm in >> dmesg. Also write OOB might corrupt metadata so the next report will >> print bogus alloc/free stacktraces. >> In most cases we need to look only at the first report, so reporting >> anything after the first is just counterproductive. > > FWIW, that sounds sane to me. > > Given that, I agree with your comment regarding READ_ONCE{,_NOCHECK}(). > > If anyone really wants all the reports, we could have a boot-time option > to do that.
I don't mind changing READ_ONCE_NOCHECK to READ_ONCE. But I don't have strong preference either way.
We could do: #define arch_atomic_read_is_already_instrumented 1 and then skip instrumentation in asm-generic if it's defined. But I don't think it's worth it.
There is no functional difference, it's only an optimization (now somewhat questionable). As Andrey said, one can get a splash of reports anyway, and it's the first one that is important. We use KASAN with panic_on_warn=1 so we don't even see the rest.
| |