On Tue 30-04-19 15:10:30, Linus Torvalds wrote: > 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. Hum, you're right. The WRITE_ONCE(mark->connector, conn) still is not enough. It needs to have a barrier so that the connector initialization is guaranteed to be visible by RCU reader. READ_ONCE(iter_info->marks[type]->connector) is safe as is already contains smp_read_barrier_depends() which is all that should be needed once we have write barrier before WRITE_ONCE(). Since I don't think this is a practical problem, I'll just queue the fix for the merge window. Thanks for spotting this! Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
