On Mon, Apr 15, 2019 at 11:08:25AM +0200, Jan Kara wrote: > On Fri 12-04-19 10:43:44, Sasha Levin wrote: > > On Fri, Apr 12, 2019 at 10:44:12AM +0200, Jan Kara wrote: > > > On Thu 11-04-19 11:26:27, Sasha Levin wrote: > > > > This is a note to let you know that I've just added the patch titled > > > > > > > > fanotify: Release SRCU lock when waiting for userspace response > > > > > > > > to the 4.9-stable tree which can be found at: > > > > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary > > > > > > > > The filename of the patch is: > > > > fanotify-release-srcu-lock-when-waiting-for-userspac.patch > > > > and it can be found in the queue-4.9 subdirectory. > > > > > > > > If you, or anyone else, feels it should not be added to the stable tree, > > > > please let <stable@xxxxxxxxxxxxxxx> know about it. > > > > > > I'd be careful with this series. You're missing at least the fixup series > > > from Miklos culminating in f37650f1c7c7 "fanotify: fix > > > fsnotify_prepare_user_wait() failure". And you seem to be missing also > > > quite some prerequisites reworking lifetime of fsnotify marks (series > > > culminating with f09b04a03e0 "fsnotify: Remove special handling of mark > > > destruction on group shutdown"). So you're just introducing subtle > > > use-after-free issues to fanotify code. Overall I think the chances for > > > regressions here are much bigger than the problem you'll be fixing unless > > > you'll go for something like wholesale update of fs/notify/* to state in > > > f37650f1c7c7. > > > > I've pulled this series based on the request here: > > https://lore.kernel.org/stable/20190411032430.17353-1-matthew.ruffell@xxxxxxxxxxxxx/ > > Thanks for reference! I've added Matthew into CC so that he's aware of the > potential problems with the patches they backported. > > > There are a few reasons why I'd prefer to keep it in: > > > > 1. It fixes a very real bug which has affected quite a few of our > > customers as well, so (at least for me) this is more than a minor > > bugfix. > > I have my reservations about it being a kernel bug :) Primarily, it is a > problem in userspace not responding to fanotify permission events properly. > With these patches, the misbehaving application will take down just the > filesystem it is working on (processes blocked in D state), without them it > will take down the whole machine. So sure the patches improve the situation > but more often than not you have to reboot anyway. > > And yes, it is bad that misbehaving userspace can take the kernel down > rather easily but that's the problem with how fanotify permission events > API has been designed and generally with the idea of AV vendors that they > need to intercept and approve all writes / opens with their AV solution. > > > 2. It came with a straightforward testcase. > > > > 3. Given that Canonical pulled it in as well, it (hopefully) received > > more testing than some other random patches. > > Sure, seeing the reference I don't blame you that you've included the > patches. > > > If there are missing patches here I'd be happy to take them in and > > re-test the kernel, but I'd really like to avoid *not* taking these > > patches just because we fear a regression but can't show it. > > So the three patches as you took them are definitely wrong because they > introduce use after free issues. Ubuntu guys have backported the part that > takes care of dropping SRCU when waiting for response but didn't backport > the part that makes sure fanotify marks, inodes, and mounts stay allocated > while we are waiting. This could be even exploitable as attacker can force > inode eviction via rm(1). So please don't include them as they are into > -stable. > > Matthew, if you really want to backport the patches changing how fanotify > uses SRCU (and honestly I'm not convinced you have to since without fixing > the AV applications the system will not work good anyway), you have to also > backport the series 5198adf649a0 "fsnotify: Remove unnecessary tests when > showing fdinfo" .. f09b04a03e0 "fsnotify: Remove special handling of mark > destruction on group shutdown") - yes, it is big and it completely reworks > lifetime of notification marks and their inode / mount references in the > kernel. And then as a cherry on top you also need to backport followup > fixes 24c20305c7f "fsnotify: clean up fsnotify_prepare/finish_user_wait()" > .. f37650f1c7c7 "fanotify: fix fsnotify_prepare_user_wait() failure". And > as a warning these are only the prerequisites I'm aware of. Given the > amount of patches, I might have easily forgotten about something. Thank you for the detailed review and for catching this. I've now dropped both of these series from the 4.4.y and 4.9.y trees. Matthew, if you can fix these up properly, feel free to resend them. thanks, greg k-h
