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=-9.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 15BE6C55178 for ; Fri, 23 Oct 2020 11:25:28 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 335F92225F for ; Fri, 23 Oct 2020 11:25:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=oss-volkswagen-com.20150623.gappssmtp.com header.i=@oss-volkswagen-com.20150623.gappssmtp.com header.b="NwQ8G5DI" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 335F92225F Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=oss.volkswagen.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 530796B0068; Fri, 23 Oct 2020 07:25:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4DE1B6B006C; Fri, 23 Oct 2020 07:25:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 37FDC6B006E; Fri, 23 Oct 2020 07:25:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0252.hostedemail.com [216.40.44.252]) by kanga.kvack.org (Postfix) with ESMTP id 082856B0068 for ; Fri, 23 Oct 2020 07:25:25 -0400 (EDT) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id A4CEB1EE6 for ; Fri, 23 Oct 2020 11:25:25 +0000 (UTC) X-FDA: 77402959410.28.grass87_31056a327259 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin28.hostedemail.com (Postfix) with ESMTP id 66C456C05 for ; Fri, 23 Oct 2020 11:25:25 +0000 (UTC) X-HE-Tag: grass87_31056a327259 X-Filterd-Recvd-Size: 8602 Received: from mail-lj1-f195.google.com (mail-lj1-f195.google.com [209.85.208.195]) by imf18.hostedemail.com (Postfix) with ESMTP for ; Fri, 23 Oct 2020 11:25:24 +0000 (UTC) Received: by mail-lj1-f195.google.com with SMTP id d24so1151829ljg.10 for ; Fri, 23 Oct 2020 04:25:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oss-volkswagen-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TZYVay0d411dVl1G8VAGv9a/ZNcXlg2PCXbI7WPCh4E=; b=NwQ8G5DIkHGdMbyPp6KENhe/Pj0nxGUCg7N66eKiwFW/XEPRmspjmS6d9mkSCb52NV D0cA8KcXGlNRK1ciIuxQ9bwJk3S9U6F5N2oF1AemlOvjaNpyXJzSuOHaTO50MVgp3ajy 4iGZlpY8ZXUxjLmPJY4XFTU1iY/JoYTJcnSLe/I1ktAXs1+ZcTB3S9ilI6e2MpdGe5Cb MyxlIYr2TpnKtPHUJcfmHI5yFs0pYvk/9qcGQxmH1NE3uujBIuFDvdNYVwgtoWvsk0Ib zBCHoGEKBdD2PVrfo8eJMsaLNmnKQz8CtsqKaF3C2qp218ilxy9mCXRTjFK6UL/4Ys8f xRTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TZYVay0d411dVl1G8VAGv9a/ZNcXlg2PCXbI7WPCh4E=; b=V01st42jvhzGESkOe5pqCEeiMgeTR7EL1dkv28by3e+l/XLeoQ3Wg5PrQpKTX6/aQn aEFPBrxmWz/OhcwKaAf4dFnxcUqMThhVlxT8I5pCX4Pvcdi1rGiKwPSVL4Zr546wdusI MJDfrITKWGYdcsnR3P8V8Hfil5cVYo4JWmM4KMaZp96sfnMg59Uat2fCGHyob/dXdUZZ 3rhA+gCnguII9zGCufKXFCRDGKYZUL1BMGdCzcra2Rxipzr6YlXo3PvtN8Pd7csVrPG/ LxFEF/DFTLxFqDlbtSyqV2+ntEJtSudl0WfQfairADi6FWUYAtaxlsu5r+0X3ovjGxIh cNJQ== X-Gm-Message-State: AOAM533qmlA2yCAepDWqKuMpS0SU/H9dd0rAwAE0JMb7DLxiArabFFnz 0X0YeO0L71XkvVfFJGPDKqAx4TQcKpyY10cg8tcqsQ== X-Google-Smtp-Source: ABdhPJxyR3fSAY9kesdHzhY7lXNT2V7I9G64mPEE6V562MrDmnf3s0fAd4JoPuGz/B0HY4Wx6NgJMYYgdd5hF0+/O04= X-Received: by 2002:a2e:b1c2:: with SMTP id e2mr721034lja.282.1603452322901; Fri, 23 Oct 2020 04:25:22 -0700 (PDT) MIME-Version: 1.0 References: <20201023074759.46605-1-laurent@oss.volkswagen.com> In-Reply-To: From: Laurent Cremmer Date: Fri, 23 Oct 2020 13:25:11 +0200 Message-ID: Subject: Re: [PATCH] hugetlb: fix locking in region_add,region_cgh,allocate_file_region_entries To: David Hildenbrand Cc: Mike Kravetz , Andrew Morton , Mina Almasry , David Rientjes , linux-mm@kvack.org, Shuah Khan Content-Type: text/plain; charset="UTF-8" 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 23.10.20 09:47, Laurent Cremmer wrote: > > commit 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing") > > introduced issues with regards to locking: > > > > - Missing spin_unlock in hugetlb.c:region_add and hugetlb.c:region_cgh > > when returning after an error. > > - Missing spin lock in hugetlb.c:allocate_file_region_entries > > when returning after an error. > > > > The first two errors were spotted using the coccinelle static code > > analysis tool while focusing on the mini_lock.cocci script, > > whose goal is to find missing unlocks. > > > > make coccicheck mode=REPORT m=mm/hugetlb.c > > > > mm/hugetlb.c:514:3-9: preceding lock on line 487 > > mm/hugetlb.c:564:2-8: preceding lock on line 554 > > > > The third instance spotted by manual examination. > > > > In static long region_add(...) and static long region_cgh(...) , releasing > > the acquired lock when exiting via their error path was removed. > > This will cause these functions to return with a lock held if they do not > > succeed. > > > > This patch reintroduces the original error path making sure the lock is > > properly released on all exits. > > > > A a new function allocate_file_region_entries was also introduced that > > must be called with a lock held and returned with the lock held. > > However the lock is temporarily released during processing but will not > > be reacquired on error. > > > > This patch ensures that the lock will be reacquired in the error path also. > > > > Fixes: 0db9d74ed884 ("hugetlb: disable region_add file_region coalescing") > > Link: https://lists.elisa.tech/g/development-process/message/289 > > Signed-off-by: Laurent Cremmer > > Reviewed-by: Oliver Hartkopp > > --- > > mm/hugetlb.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index fe76f8fd5a73..92bea6f77361 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -438,7 +438,7 @@ static int allocate_file_region_entries(struct resv_map *resv, > > for (i = 0; i < to_allocate; i++) { > > trg = kmalloc(sizeof(*trg), GFP_KERNEL); > > So we're allocating memory with GFP_KERNEL while holding a spinlock? > > If my memory isn't completely wrong, that's broken, no? this would > require GFP_ATOMIC? > > But I might be messing up things :) > Ah, the beauty of patches sometimes not telling the whole story :-) The lock is released in line 437, prior attempting to allocate with GFP_KERNEL, so GFP_ATOMIC is not required in this context. > > if (!trg) > > - goto out_of_memory; > > + goto out_of_memory_unlocked; The problem fixed here is that in this particular situation (i.e. releasing the lock, attempting to kmalloc(..., GFP_KERNEL) ) if the allocation failed, the lock would not be reacquired in the error path, and the function would return without the lock being held on error and with the lock being held on success, which doesn't feel right. > > list_add(&trg->link, &allocated_regions); > > } > > > > @@ -450,7 +450,8 @@ static int allocate_file_region_entries(struct resv_map *resv, > > > > return 0; > > > > -out_of_memory: > > +out_of_memory_unlocked: However, in light of your remarks, I would question re-acquiring the lock before cleaning up using kfree and would defer it to after the clean up as ended. Agree? > > + spin_lock(&resv->lock); > > list_for_each_entry_safe(rg, trg, &allocated_regions, link) { > > list_del(&rg->link); > > kfree(rg); > > @@ -508,7 +509,8 @@ static long region_add(struct resv_map *resv, long f, long t, > > > > if (allocate_file_region_entries( > > resv, actual_regions_needed - in_regions_needed)) { > > - return -ENOMEM; > > + add = -ENOMEM; > > dito, does this handle atomic context? > IMHO it does, since allocate_file_region_entries will drop the lock when attempting to allocate entries and reacquires it on exit. > > > + goto out_locked; > > } > > > > goto retry; > > @@ -517,7 +519,7 @@ static long region_add(struct resv_map *resv, long f, long t, > > add = add_reservation_in_range(resv, f, t, h_cg, h, NULL); > > > > resv->adds_in_progress -= in_regions_needed; > > - > > +out_locked: > > spin_unlock(&resv->lock); > > VM_BUG_ON(add < 0); > > return add; > > @@ -557,11 +559,14 @@ static long region_chg(struct resv_map *resv, long f, long t, > > if (*out_regions_needed == 0) > > *out_regions_needed = 1; > > > > - if (allocate_file_region_entries(resv, *out_regions_needed)) > > - return -ENOMEM; > > + if (allocate_file_region_entries(resv, *out_regions_needed)) { > > + chg = -ENOMEM; > > + goto out_locked; > > + } > > dito > See above, allocate_file_region_entries will return the lock held. Now, in defence of the original implementation what "looks" like a bug at first sight might be an attempt to "optimize-out" a pair of lock/unlock operations on the error path of region_add, and region_chg by relying on the implicit knowledegfe that allocate_file_region_entries would return with the lock not being held on error. Unfortunately it hinders the readability (and IMHO the maintainability) of the code. > > -- > Thanks, > > David / dhildenb >