On Fri 09-09-16 15:39:44, Miklos Szeredi wrote: > On Thu, Sep 8, 2016 at 4:25 PM, Jan Kara <jack@xxxxxxx> wrote: > > fanotify_get_response() calls fsnotify_remove_event() when it finds that > > group is being released from fanotify_release() (bypass_perm is set). > > However the event it removes need not be only in the group's notification > > queue but it can have already moved to access_list (userspace read the > > event before closing the fanotify instance fd) which is protected by a > > different lock. Thus when fsnotify_remove_event() races with > > fanotify_release() operating on access_list, the list can get corrupted. > > > > Fix the problem by moving all the logic removing permission events from > > the lists to one place - fanotify_release(). > > Reviewed-by: Miklos Szeredi <mszeredi@xxxxxxxxxx> > > This should fix the reported issue, and it's cleaner than my solution. > I'll backport and ask QA to test. I haven't tried reproducing it > myself. OK, thanks! > However the theoretical memory access ordering issue (which might > possibly be impossible to trigger) is still there AFAICS: > > CPU1: > list_del_init(&event->fae.fse.list); > event->response = FAN_ALLOW; > > CPU2: > wait_event(group->fanotify_data.access_waitq, event->response); > ... > WARN_ON(!list_empty(&event->list)); > > So I think we still need a separate patch adding smp_wmb() before > setting event->response and smp_rmb() after the wait_event(). Ugh, this is really theoretical ;) But I agree nothing really prevents it. The core reason for this seems to be the lockless check of event->list. I dislike adding barriers just to accommodate that WARN_ON() (although that is a useful one so I wouldn't like to lose it either). I'll think about it. Thanks for spotting this. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
