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=-17.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_IN_DEF_DKIM_WL 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 EA641C4363A for ; Wed, 28 Oct 2020 19:18:28 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 21FDC24814 for ; Wed, 28 Oct 2020 19:18:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="h0aj5ArE" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 21FDC24814 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 344CD6B005C; Wed, 28 Oct 2020 15:18:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2F4BF6B0062; Wed, 28 Oct 2020 15:18:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1BD696B0068; Wed, 28 Oct 2020 15:18:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0120.hostedemail.com [216.40.44.120]) by kanga.kvack.org (Postfix) with ESMTP id E132C6B005C for ; Wed, 28 Oct 2020 15:18:26 -0400 (EDT) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 607733624 for ; Wed, 28 Oct 2020 19:18:26 +0000 (UTC) X-FDA: 77422295412.24.scarf30_37125ee27287 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin24.hostedemail.com (Postfix) with ESMTP id 4367E1A4A5 for ; Wed, 28 Oct 2020 19:18:26 +0000 (UTC) X-HE-Tag: scarf30_37125ee27287 X-Filterd-Recvd-Size: 7741 Received: from mail-ej1-f67.google.com (mail-ej1-f67.google.com [209.85.218.67]) by imf50.hostedemail.com (Postfix) with ESMTP for ; Wed, 28 Oct 2020 19:18:25 +0000 (UTC) Received: by mail-ej1-f67.google.com with SMTP id k3so428935ejj.10 for ; Wed, 28 Oct 2020 12:18:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=9niM4JKwNk5r53jpuP1fceCavRx/okOpY0nXS/kZ1qA=; b=h0aj5ArEW4iwe9zBAk/pb4zrX7F1/Cb28y3Fo0I2jNOCIPgrJacC9+fXGhF0A2FrvO V1gmrH3JDySWoCeMolkcjEeggwg5Sd9LgVMQEskyMcSHJ+kHeXHK4i1NYZat3uts1+ia gyySo16vmI/9NNle5R6RLC9P0fAgakc6AuskAlBjFdob+9uelw17VU8XWsToeSeVOHYO fAJT3zZPb2iTKVmAVPHb5y/FDFWX8cRXniAmX2sh34gMWM8Cqfn+xpKQrrXfjHi5AZj5 0QwciGO0Vo98qkPh86CmFgDzFbniaTbNpPLEtb6q5RYTrLXrwbx3OYploFvzpexPApWS WUXA== 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=9niM4JKwNk5r53jpuP1fceCavRx/okOpY0nXS/kZ1qA=; b=bWrPYvA7A0F2CU3VKUvF+3dqSOCgT/TLnh/7v47uDOp5iJ1R16TbOWTIXroKdmArXD jSRRtV9T6yQ6F++fwh8Y3rog6qX04M569glVKxdbH40gc20SaxtphHZN/4sQJeo4pCWs D1OUfLHxQgqLOMwiaWXSNxoogNRzpo5Vmlj+znnl8LIJi8SGucZN9k/2v2/CE5BKIbEM +k15qU58tuutEPYel/8vc1N5GrACjaTbB319SCm2rP1gHZ6KGUYTAups90PLCKldkFn1 DFnMoJthp4dOQj1oLLFZ5njp1oeYYvgHiZmnpdmMiGjfRzdGcd2yooiWyIrZJRfaoBHI w4Kw== X-Gm-Message-State: AOAM530qa5pEGB2XQOSHO7LwUlFegYoT9VpLlMTQQH15tpL5jMAHTC1u NDMysQMF4pRQ1EcMwW8J/DxH6ZIUNoGiXz5vkgBasw== X-Google-Smtp-Source: ABdhPJwjEii6vC+g+wPjJkMkauH+Qr0S6pWHA8Kfu3vv1OgzCBfyqcoQ/cJyqA45uNH5PyqcOq1GM05DGB+elZ/K3LI= X-Received: by 2002:a17:906:48b:: with SMTP id f11mr579070eja.293.1603912704115; Wed, 28 Oct 2020 12:18:24 -0700 (PDT) MIME-Version: 1.0 References: <20201023074759.46605-1-laurent@oss.volkswagen.com> In-Reply-To: <20201023074759.46605-1-laurent@oss.volkswagen.com> From: Mina Almasry Date: Wed, 28 Oct 2020 12:18:13 -0700 Message-ID: Subject: Re: [PATCH] hugetlb: fix locking in region_add,region_cgh,allocate_file_region_entries To: Laurent Cremmer Cc: Mike Kravetz , Andrew Morton , David Rientjes , Linux-MM , 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 Fri, Oct 23, 2020 at 12:58 AM 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(-) > Sorry for the late reply, I've been out a few days. I'm catching up to this now. It looks like I missed some unlocks/locks on error paths. I don't recall this being an intentional optimization; likely just an oversight. I think adding the proper locking is good for readability and shouldn't impact perf too much since it's just error paths, but I'll defer to Mike here. I'm adding some minor comments in case Mike decides to go through with this. > 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); > if (!trg) > - goto out_of_memory; > + goto out_of_memory_unlocked; nit: I would not rename this. IMO specifying _unlocked is only useful if we had another label called 'out_of_memory'. > 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: > + 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; > + goto out_locked; I don't think this works, since we VM_BUG_ON(add < 0). I think it's acceptable to just unlock and return here. Or, for better readability, add a variable called err = 0 at the top, set that variable here, and jump to a label that unlocks and returns err. > } > > 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; > + } > > resv->adds_in_progress += *out_regions_needed; > > +out_locked: > spin_unlock(&resv->lock); > return chg; > } > -- > 2.17.1 >