On Wed 01-05-19 16:52:28, Jan Kara wrote: > 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! And looking some more into this. I don't think the issue can happen at all. The thing is that the "connector" gets allocated, initialized, and attached to inode / mntpoint / sb using cmpxchg() which provides the barrier. Then mark gets added to connector's list and mark->connector is set. So *mark* changes happening in fsnotify_add_mark_list() can get reordered (but there's just list addition there) but *connector* changes are safely visible. But this certainly deserves a comment as even I got confused and it was me who wrote this all ;) Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
