lkml.org 
[lkml]   [2020]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -next] lib: disable KCSAN for XArray
On Fri, Mar 06, 2020 at 08:53:00AM -0800, Matthew Wilcox wrote:
> On Fri, Mar 06, 2020 at 02:38:39PM +0100, Marco Elver wrote:
> > On Thu, 5 Mar 2020 at 22:39, Paul E. McKenney <paulmck@kernel.org> wrote:
> > > On Thu, Mar 05, 2020 at 07:18:31AM -0800, Matthew Wilcox wrote:
> > > > I have found three locations where we use the ->marks array:
> > > >
> > > > 1.
> > > > unsigned long data = *addr & (~0UL << offset);
> > > > if (data)
> > > > return __ffs(data);
> > > >
> > > > 2.
> > > > return find_next_bit(addr, XA_CHUNK_SIZE, offset);
> > > > 3.
> > > > return test_bit(offset, node_marks(node, mark));
> > > >
> > > > The modifications -- all done with the spinlock held -- use the non-atomic
> > > > bitops:
> > > > return __test_and_set_bit(offset, node_marks(node, mark));
> > > > return __test_and_clear_bit(offset, node_marks(node, mark));
> > > > bitmap_fill(node_marks(node, mark), XA_CHUNK_SIZE);
> > > > (that last one doesn't really count -- it's done prior to placing the node
> > > > in the tree)
> > > >
> > > > The first read seems straightforward; I can place a READ_ONCE around
> > > > *addr. The second & third reads are rather less straightforward.
> > > > find_next_bit() and test_bit() are common code and use plain loads today.
> > >
> > > Yes, those last two are a bit annoying, aren't they? I guess the first
> > > thing would be placing READ_ONCE() inside them, and if that results in
> > > regressions, have an alternative API for concurrent access?
> >
> > FWIW test_bit() is an "atomic" bitop (per atomic_bitops.txt), and
> > KCSAN treats it as such. On x86 arch_test_bit() is not instrumented,
> > and then in asm-generic/bitops/instrumented-non-atomic.h test_bit() is
> > instrumented with instrument_atomic_read(). So on x86, things should
> > already be fine for test_bit(). Not sure about other architectures.
>
> Hum. It may well be documented as atomic, but is it? Here's the
> generic implementation:
>
> static inline int test_bit(int nr, const volatile unsigned long *addr)
> {
> return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
> }
>
> arch_test_bit is only used by the instrumented variants:
>
> $ git grep arch_test_bit include
> include/asm-generic/bitops/instrumented-non-atomic.h: return arch_test_bit(nr, addr);
>
> As far as I can tell, the generic version is what's used on x86. Does
> the 'volatile' qualifier save us here?
>
> find_next_bit() doesn't have the 'volatile' qualifier, so may still be
> a problem?

One approach would be to add the needed READ_ONCE().

Another, if someone is crazy enough to do the work, would be to verify
that the code output is as if there was a READ_ONCE().

Thoughts?

Thanx, Paul

\
 
 \ /
  Last update: 2020-03-06 18:03    [W:0.063 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site