From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B4A3EC10F13 for ; Tue, 16 Apr 2019 09:01:25 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6D7D320868 for ; Tue, 16 Apr 2019 09:01:25 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=kroah.com header.i=@kroah.com header.b="az+211ep"; dkim=pass (2048-bit key) header.d=messagingengine.com header.i=@messagingengine.com header.b="0afamTd7" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727852AbfDPJBY (ORCPT ); Tue, 16 Apr 2019 05:01:24 -0400 Received: from out5-smtp.messagingengine.com ([66.111.4.29]:59875 "EHLO out5-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726576AbfDPJBY (ORCPT ); Tue, 16 Apr 2019 05:01:24 -0400 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 5C75821FFD; Tue, 16 Apr 2019 05:01:22 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Tue, 16 Apr 2019 05:01:22 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kroah.com; h= date:from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm3; bh=1iCMzNDgKeLnbqNhDv7+X6xAOTh Okq/Pl69WvOSk6XE=; b=az+211epbxc/YvzPZFOYx8UD7RhMutEi11Uq0wd+Cnm 82cGLu73r3S4urhdsafeQ0BG0fu/IqHb+8V+ibEgkrwHdnl7vqGeUFQg08d9wsAI qgavHvbsTInMwDfdHd09yv7bPKCmhHTv1zcPQAvM1h/+23wxzjmFdzO/rR3u/c8u RwjxNDoWqTFKYU85qK1Y3XBvxH/6SLz+wpdr/lz15ukC8zj0cmCCcraEp2iKQrLq 8oImpdUOhfxtsBJlyMNjs3XzXHl4oDKkhcy4ToSMX1AxRjL+y8Y7IuPBoiI/ZFsV 0G6g+5HtWiLu4cPBai5qIe7aZcU/dpaqGW/nOqBD5yw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=1iCMzN DgKeLnbqNhDv7+X6xAOThOkq/Pl69WvOSk6XE=; b=0afamTd7PbkGcxf3h2nk/a eFz7C2e3sYmJXOUR2Po8zj8Z/d4+oUHaWJxVoxGeYEs6/r3YSMBvsCvoVqBOZK7i XC4JN5NFYi06xmRYzXoi1R+L1eAzqgo4Ova0jCIEE9fIAG9lyB7o5uRaBdc0gQfv Y5K0+d2VwpBS+p6fBw8XOyrk8BCbi6UzfEL8wedNF1qZ8OssSPfrAmJXLQ+Jbq6S Ad6qd6nn0fWxGIHE+7SiPzMosP+h+G920B7A/4umtjeXUk8fBasDDklWCsFhHp3z yo59B6L0YAj8tEFiihVwovrIyx4dVdnF7BZo7e7tJUesXNnhJL36PjIsezzK+ONA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduuddrfedugdduudcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvuffkfhggtggujggfsehttdertddtredvnecuhfhrohhmpefirhgvghcu mffjuceoghhrvghgsehkrhhorghhrdgtohhmqeenucffohhmrghinhepkhgvrhhnvghlrd horhhgnecukfhppeekfedrkeeirdekledruddtjeenucfrrghrrghmpehmrghilhhfrhho mhepghhrvghgsehkrhhorghhrdgtohhmnecuvehluhhsthgvrhfuihiivgeptd X-ME-Proxy: Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) by mail.messagingengine.com (Postfix) with ESMTPA id 169E4E415C; Tue, 16 Apr 2019 05:01:20 -0400 (EDT) Date: Tue, 16 Apr 2019 11:01:19 +0200 From: Greg KH To: Matthew Ruffell Cc: Jan Kara , Sasha Levin , stable-commits@vger.kernel.org, stable@vger.kernel.org Subject: Re: Patch "fanotify: Release SRCU lock when waiting for userspace response" has been added to the 4.9-stable tree Message-ID: <20190416090119.GE4209@kroah.com> References: <20190411152628.8E2682173C@mail.kernel.org> <20190412084412.GB29850@quack2.suse.cz> <20190412144344.GR11568@sasha-vm> <20190415090825.GA3031@quack2.suse.cz> <20190415104228.GA9397@kroah.com> <8b1eb951-498e-45f2-138b-9fa4290268e1@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8b1eb951-498e-45f2-138b-9fa4290268e1@canonical.com> User-Agent: Mutt/1.11.4 (2019-03-13) Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org 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 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