Re: Patch "fanotify: Release SRCU lock when waiting for userspace response" has been added to the 4.9-stable tree

[Greg KH <greg@kroah.com>]

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@vger.kernel.org> 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@canonical.com/
> >> 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