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.2 required=3.0 tests=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 77CA2C7618B for ; Fri, 26 Jul 2019 13:38:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 55A5E2238C for ; Fri, 26 Jul 2019 13:38:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726402AbfGZNin (ORCPT ); Fri, 26 Jul 2019 09:38:43 -0400 Received: from mga11.intel.com ([192.55.52.93]:26492 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726364AbfGZNim (ORCPT ); Fri, 26 Jul 2019 09:38:42 -0400 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Jul 2019 06:38:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,311,1559545200"; d="scan'208";a="198321941" Received: from soegtrop-mobl1.ger.corp.intel.com (HELO [10.252.37.234]) ([10.252.37.234]) by fmsmga002.fm.intel.com with ESMTP; 26 Jul 2019 06:38:40 -0700 Subject: Re: [Intel-gfx] [PATCH 1/5] drm/i915/userptr: Beware recursive lock_page() To: Tvrtko Ursulin , Chris Wilson , intel-gfx@lists.freedesktop.org Cc: stable@vger.kernel.org References: <20190716124931.5870-1-chris@chris-wilson.co.uk> <156329142200.9436.8651620549785965913@skylake-alporthouse-com> <156336944635.4375.7269371478914847980@skylake-alporthouse-com> <6038b21f-c052-36c5-2d56-72ddeb069097@linux.intel.com> <156337053617.4375.13675276970408492219@skylake-alporthouse-com> <951e2751-15d7-9ca8-ef6f-299ba59c47a6@linux.intel.com> <156337241401.4375.2377981562987470090@skylake-alporthouse-com> From: Lionel Landwerlin Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ Message-ID: <4a90e8f9-694c-8dea-45b6-e5ea5677df64@intel.com> Date: Fri, 26 Jul 2019 16:38:40 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org On 17/07/2019 21:09, Tvrtko Ursulin wrote: > > On 17/07/2019 15:06, Chris Wilson wrote: >> Quoting Tvrtko Ursulin (2019-07-17 14:46:15) >>> >>> On 17/07/2019 14:35, Chris Wilson wrote: >>>> Quoting Tvrtko Ursulin (2019-07-17 14:23:55) >>>>> >>>>> On 17/07/2019 14:17, Chris Wilson wrote: >>>>>> Quoting Tvrtko Ursulin (2019-07-17 14:09:00) >>>>>>> >>>>>>> On 16/07/2019 16:37, Chris Wilson wrote: >>>>>>>> Quoting Tvrtko Ursulin (2019-07-16 16:25:22) >>>>>>>>> >>>>>>>>> On 16/07/2019 13:49, Chris Wilson wrote: >>>>>>>>>> Following a try_to_unmap() we may want to remove the userptr >>>>>>>>>> and so call >>>>>>>>>> put_pages(). However, try_to_unmap() acquires the page lock >>>>>>>>>> and so we >>>>>>>>>> must avoid recursively locking the pages ourselves -- which >>>>>>>>>> means that >>>>>>>>>> we cannot safely acquire the lock around set_page_dirty(). >>>>>>>>>> Since we >>>>>>>>>> can't be sure of the lock, we have to risk skip dirtying the >>>>>>>>>> page, or >>>>>>>>>> else risk calling set_page_dirty() without a lock and so risk fs >>>>>>>>>> corruption. >>>>>>>>> >>>>>>>>> So if trylock randomly fail we get data corruption in whatever >>>>>>>>> data set >>>>>>>>> application is working on, which is what the original patch >>>>>>>>> was trying >>>>>>>>> to avoid? Are we able to detect the backing store type so at >>>>>>>>> least we >>>>>>>>> don't risk skipping set_page_dirty with anonymous/shmemfs? >>>>>>>> >>>>>>>> page->mapping??? >>>>>>> >>>>>>> Would page->mapping work? What is it telling us? >>>>>> >>>>>> It basically tells us if there is a fs around; anything that is >>>>>> the most >>>>>> basic of malloc (even tmpfs/shmemfs has page->mapping). >>>>> >>>>> Normal malloc so anonymous pages? Or you meant everything _apart_ >>>>> from >>>>> the most basic malloc? >>>> >>>> Aye missed the not. >>>> >>>>>>>> We still have the issue that if there is a mapping we should be >>>>>>>> taking >>>>>>>> the lock, and we may have both a mapping and be inside >>>>>>>> try_to_unmap(). >>>>>>> >>>>>>> Is this a problem? On a path with mappings we trylock and so >>>>>>> solve the >>>>>>> set_dirty_locked and recursive deadlock issues, and with no >>>>>>> mappings >>>>>>> with always dirty the page and avoid data corruption. >>>>>> >>>>>> The problem as I see it is !page->mapping are likely an >>>>>> insignificant >>>>>> minority of userptr; as I think even memfd are essentially >>>>>> shmemfs (or >>>>>> hugetlbfs) and so have mappings. >>>>> >>>>> Better then nothing, no? If easy to do.. >>>> >>>> Actually, I erring on the opposite side. Peeking at mm/ internals does >>>> not bode confidence and feels indefensible. I'd much rather throw my >>>> hands up and say "this is the best we can do with the API provided, >>>> please tell us what we should have done." To which the answer is >>>> probably to not have used gup in the first place :| >>> >>> """ >>> /* >>>   * set_page_dirty() is racy if the caller has no reference against >>>   * page->mapping->host, and if the page is unlocked. This is >>> because another >>>   * CPU could truncate the page off the mapping and then free the >>> mapping. >>>   * >>>   * Usually, the page _is_ locked, or the caller is a user-space >>> process which >>>   * holds a reference on the inode by having an open file. >>>   * >>>   * In other cases, the page should be locked before running >>> set_page_dirty(). >>>   */ >>> int set_page_dirty_lock(struct page *page) >>> """ >>> >>> Could we hold a reference to page->mapping->host while having pages >>> and then would be okay to call plain set_page_dirty? >> >> We would then be hitting the warnings in ext4 for unlocked pages again. > > Ah true.. > >> Essentially the argument is whether or not that warn is valid, to >> which I >> think requires inner knowledge of vfs + ext4. To hold a reference on the >> host would require us tracking page->mapping (reasonable since we >> already hooked into mmu and so will get an invalidate + fresh gup on >> any changes), plus iterating over all to acquire the extra reference if >> applicable -- and I have no idea what the side-effects of that would be. >> Could well be positive side-effects. Just feels like wandering even >> further off the beaten path without a map. Good news hmm is just around >> the corner (which will probably prohibit this use-case) :| > > ... can we reach out to someone more knowledgeable in mm matters to > recommend us what to do? > > Regards, > > Tvrtko Just a reminder to not let this slip. We run into userptr bugs in CI quite regularly. Thanks, -Lionel