On Fri, Oct 20, 2017 at 1:37 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > On Thu, Oct 19, 2017 at 4:58 PM, Miklos Szeredi <mszeredi@xxxxxxxxxx> wrote: >> We may fail to pin one of the marks in fsnotify_prepare_user_wait() when >> dropping the srcu read lock, resulting in use after free at the next >> iteration. >> >> Solution is to store both marks in iter_info instead of just the one we'll >> be sending the event for, as well as pinning the group for both (if they >> have the same group: fine, pinnig it twice doesn't hurt). I was thinking about this one and actually pinning only the single group for the used mark was correct, since the other, unused mark won't have its group dereferenced while pinned. Only there doesn't appear to be a way to make this work without adding complexity :( >> >> Also blind increment of group's user_waits is not enough, we could be far >> enough in the group's destruction that it isn't taken into account. >> Instead we need to check (under lock) that the mark is still attached to >> the group after having obtained a ref to the mark. If not, skip it. >> >> In the process of fixing the above, clean up fsnotify_prepare_user_wait()/ >> fsnotify_finish_user_wait(). >> > > Change looks correct, but it was so hard to follow this patch. > The moving of helpers around created a completely garbled diff > where it is impossible to see the logic changed. > > Suggest to separate to 3 patches: create helpers (move them around), > pass the 2 marks in iter_info and fix increment of group's user_waits. Yeah, I'll do that. Thanks, Miklos
