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=-5.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no 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 7208FC56202 for ; Fri, 13 Nov 2020 00:42:27 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id CD08B21D79 for ; Fri, 13 Nov 2020 00:42:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CD08B21D79 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 0B1186B005C; Thu, 12 Nov 2020 19:42:26 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 062856B005D; Thu, 12 Nov 2020 19:42:25 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E93AA6B0068; Thu, 12 Nov 2020 19:42:25 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0037.hostedemail.com [216.40.44.37]) by kanga.kvack.org (Postfix) with ESMTP id B9CDD6B005C for ; Thu, 12 Nov 2020 19:42:25 -0500 (EST) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 5F3948249980 for ; Fri, 13 Nov 2020 00:42:25 +0000 (UTC) X-FDA: 77477543850.14.oil35_25110992730a Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin14.hostedemail.com (Postfix) with ESMTP id 3F90B1822987A for ; Fri, 13 Nov 2020 00:42:25 +0000 (UTC) X-HE-Tag: oil35_25110992730a X-Filterd-Recvd-Size: 5265 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by imf32.hostedemail.com (Postfix) with ESMTP for ; Fri, 13 Nov 2020 00:42:23 +0000 (UTC) IronPort-SDR: h/G4el9wKAPPkzupBTiSgepXEelHplcGSfCUYBcD6iy5yY46OY0Hyqwjmf3HhD+eCmoul0YfSs s2WqcgxuRq6Q== X-IronPort-AV: E=McAfee;i="6000,8403,9803"; a="234565641" X-IronPort-AV: E=Sophos;i="5.77,473,1596524400"; d="scan'208";a="234565641" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Nov 2020 16:42:21 -0800 IronPort-SDR: rsU/NrbilkyfFShHgrIPrigKB4wbPEpm3KVR7kB5TFbqJFukcWIHXPPPCUDOa9RfFmQPUTza1M Z1mxeWgTeHLA== X-IronPort-AV: E=Sophos;i="5.77,473,1596524400"; d="scan'208";a="542464450" Received: from iweiny-desk2.sc.intel.com (HELO localhost) ([10.3.52.147]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Nov 2020 16:42:21 -0800 Date: Thu, 12 Nov 2020 16:42:21 -0800 From: Ira Weiny To: Jason Gunthorpe Cc: Dennis Dalessandro , dledford@redhat.com, Jann Horn , linux-rdma@vger.kernel.org, Mike Marciniszyn , linux-mm@kvack.org Subject: Re: [PATCH for-rc v2] IB/hfi1: Move cached value of mm into handler Message-ID: <20201113004221.GX3976735@iweiny-DESK2.sc.intel.com> References: <20201112025837.24440.6767.stgit@awfm-01.aw.intel.com> <20201112171439.GT3976735@iweiny-DESK2.sc.intel.com> <20201113000136.GW244516@ziepe.ca> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201113000136.GW244516@ziepe.ca> User-Agent: Mutt/1.11.1 (2018-12-01) X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Nov 12, 2020 at 08:01:36PM -0400, Jason Gunthorpe wrote: > On Thu, Nov 12, 2020 at 05:06:30PM -0500, Dennis Dalessandro wrote: > > On 11/12/2020 12:14 PM, Ira Weiny wrote: > > > On Wed, Nov 11, 2020 at 09:58:37PM -0500, Dennis Dalessandro wrote: > > > > Two earlier bug fixes have created a security problem in the hfi1 > > > > driver. One fix aimed to solve an issue where current->mm was not valid > > > > when closing the hfi1 cdev. It attempted to do this by saving a cached > > > > value of the current->mm pointer at file open time. This is a problem if > > > > another process with access to the FD calls in via write() or ioctl() to > > > > pin pages via the hfi driver. The other fix tried to solve a use after > > > > free by taking a reference on the mm. This was just wrong because its > > > > possible for a race condition between one process with an mm that opened > > > > the cdev if it was accessing via an IOCTL, and another process > > > > attempting to close the cdev with a different current->mm. > > > > > > Again I'm still not seeing the race here. It is entirely possible that the fix > > > I was trying to do way back was mistaken too... ;-) I would just delete the > > > last 2 sentences... and/or reference the commit of those fixes and help > > > explain this more. > > > > I was attempting to refer to [1], the email that started all of this. > > > > > > > > > > To fix this correctly we move the cached value of the mm into the mmu > > > > handler struct for the driver. > > > > > > Looking at this closer I don't think you need the mm member of mmu_rb_handler > > > any longer. See below. > > > > We went back and forth on this as well. We thought it better to rely on our > > own pointer vs looking into the notifier to get the mm. Same reasoning for > > doing our own referecne counting. Question is what is the preferred way > > here. Functionally it makes no difference and I'm fine going either way. > > Use the mm pointer in the notifier if you have a notifier registered, > it is clearer as to the lifetime and matches what other places do I agree. IOW, if you are storing a pointer to something you should take a reference to it. Here you store 2 pointers but only take 1 reference and the user has to 'know' the lifetime is the same... > > > That's the question. It does make sense to do that if we are sticking iwth > > the notifier's reference vs our own explicit one. I'm not 100% sold that we > > should not be doing the ref counting and keeping our own pointer. To me we > > shoudln't be looking inside the notifer struct and instead honestly there > > should probably be an API/helper call to get the mm from it. I'm open to > > either approach. > > The notifier is there to support users of the notifier, and nearly all > notifier users require the mm at various points. > > Adding get accessors is a bit of a kernel anti-pattern, this isn't Java.. I'm not 100% on board that a helper call is an anti-pattern, it can help to ID that users are 'reaching into' the notifier 'object'... That said, in this case I would not bother. Also you defined other helper calls which reach into the notifier... so FWIW you were not consistent in this patch. That flagged my attention in the first place... ;-) Ira