On Tue, Apr 30, 2019 at 2:41 PM Jan Kara <jack@xxxxxxx> wrote:
>
> to get a fix of user trigerable NULL pointer dereference syzbot has
> recently spotted. The problem has been introduced in rc1 so no CC stable is
> needed.
Hmm. Pulled, but I thin kthe use of READ_ONCE/WRITE_ONCE is suspicious.
If we're reading a pointer like this locklessly, the proper sequence
is almost always something like "smp_store_release()" to write the
pointer, and "smp_load_acquire()" to read it.
Because that not only does the "access once" semantics, but it also
guarantees that when we actually look _through_ the pointer, we see
the data that was written to it. In contrast, code like this (from the
fix)
+ WRITE_ONCE(mark->connector, conn);
...
+ conn = READ_ONCE(iter_info->marks[type]->connector);
+ /* Mark is just getting destroyed or created? */
+ if (!conn)
+ continue;
+ fsid = conn->fsid;
is rather suspicious, because there's no obvious guarantee that tjhe
"conn->fsid" part was written on one CPU before we read it on another.
There may well be barriers in place there that end up guaranteeing it
in practice, but I wanted to point out that the READ/WRITE_ONCE()
pattern tends to be a bit dodgy unless you have some other explicit
synchronization (and if that synchronization is a lock, then you
obviously shouldn't be using READ/WRITE_ONCE() at all).
Linus
