On Tue, Apr 16, 2019 at 05:11:08PM +1200, Matthew Ruffell wrote: > On 15/04/19 10:42 PM, Greg KH wrote: > > > 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 > Jan, thank you for your detailed explanation and reasoning why just backporting > those three commits is a bad idea. > > I think I got a bit of tunnel vision trying to resolve the issue the customer > was having, and when trying to stay within the -stable rules of keeping things > small and to the point, I seemed to have accepted that those three commits were > the answer, and did not consider the other commits on either side of those three > commits, and the impact of not having them. > > You have taught me that I need to take a moment and consider the whole picture > and read around the commits on either side of the fix before creating patches, > testing them and finding that the system no longer hangs, and accepting them as > the answer. > > If anyone is interested, I went and looked into the list of commits Jan > mentioned, and if they need backporting for 4.9: > > [ Fixup series] > f37650f1c7c71cf5180b43229d13b421d81e7170 - cherry pick > 9a31d7ad997f55768c687974ce36b759065b49e5 - cherry pick > 0d6ec079d6aaa098b978d6395973bb027c752a03 - cherry pick > 24c20305c7fc8959836211cb8c50aab93ae0e54f - cherry pick > > [ Core series on dropping SRCU lock] > 05f0e38724e8449184acd8fbf0473ee5a07adc6c - cherry pick > 9385a84d7e1f658bb2d96ab798393e4b16268aaa - backport > abc77577a669f424c5d0c185b9994f2621c52aa4 - cherry pick > > [Mark lifetime series] > f09b04a03e0239f65bd964a1de758e53cf6349e8 - cherry pick > 6b3f05d24d355f50f3d9814304650fcab0efb482 - backport > 11375145a70d69e871dd5b8fcadd5d1ee4162e7c - cherry pick > e7253760587e8523fe1e8ede092a620f1403f2e8 - cherry pick > 08991e83b7286635167bab40927665a90fb00d81 - cherry pick > 04662cab59fc3e8421fd7a0539d304d51d2750a4 - cherry pick > 2629718dd26f89e064dcdec6c8e5b9713502e1f8 - cherry pick > 73cd3c33ab793325ebaae27fa58b4f713c16f12c - cherry pick > 8212a6097a720896b4cdbe516487ad47f4296599 - cherry pick > a03e2e4f078365428bb4317989cb5d1d6563cfe9 - cherry pick > f06fd98759451876f51607f60abd74c89b141610 - cherry pick > a242677bb1e6faa9bd82bd33afb2621071258231 - cherry pick > 0810b4f9f207910d90aee56d312d25f334796363 - cherry pick > 755b5bc681eb46de7bfaec196f85e30efd95bd9f - cherry pick > e911d8af87dba7642138f4320ca3db80629989f2 - cherry pick > 86ffe245c430f07f95d5d28d3b694ea72f4492e7 - backport > 9dd813c15b2c101168808d4f5941a29985758973 - backport > > [ Necessary infrastructure ] > c97476400d3b73376fc055e828d7388d6b9ea99a - cherry pick > 25c829afbd74fb9594d2351d9e41be05bacb9903 - cherry pick > 5198adf649a0b7b0f9ddb98b264e57b41516116b - cherry pick > e3ba730702af370563f66cb610b71aa0ca67955e - cherry pick > > Now, this is starting to be a lot of commits, and 4.4 requires even more. > > I think we might accept that this is far too major of a change for 4.4, 4.9 and > there is probably a little too much risk of regression. I agree. > Greg, thanks for dropping the patches from 4.4.y and 4.9.y trees, I was starting > to worry I might break some machines if they got through. > > If anyone is still interested in a new patch series, I have some spare time > which means I can keep working on this if needed, but for now, I think I will > tell my customer to upgrade to the Ubuntu 4.15 LTS kernel, and move on. That is a good idea. thanks, greg k-h
