linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Cremmer <laurent@oss.volkswagen.com>
To: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Mina Almasry <almasrymina@google.com>,
	David Rientjes <rientjes@google.com>,
	linux-mm@kvack.org, Shuah Khan <skhan@linuxfoundation.org>,
	Laurent Cremmer <laurent@oss.volkswagen.com>
Subject: [PATCH] hugetlb: fix locking in region_add,region_cgh,allocate_file_region_entries
Date: Fri, 23 Oct 2020 09:47:59 +0200	[thread overview]
Message-ID: <20201023074759.46605-1-laurent@oss.volkswagen.com> (raw)

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 <laurent@oss.volkswagen.com>
Reviewed-by: Oliver Hartkopp <hartkopp@oss.volkswagen.com>
---
 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);
 			if (!trg)
-				goto out_of_memory;
+				goto out_of_memory_unlocked;
 			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;
 		}
 
 		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



             reply	other threads:[~2020-10-23  7:58 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-23  7:47 Laurent Cremmer [this message]
2020-10-23 10:12 ` [PATCH] hugetlb: fix locking in region_add,region_cgh,allocate_file_region_entries David Hildenbrand
2020-10-23 11:25   ` Laurent Cremmer
2020-10-23 11:39     ` David Hildenbrand
2020-10-23 11:02 ` Oscar Salvador
2020-10-23 11:32   ` Laurent Cremmer
2020-10-23 12:11     ` Oscar Salvador
2020-10-23 16:47       ` Mike Kravetz
2020-10-27  7:34         ` Laurent Cremmer
2020-10-28 19:18 ` Mina Almasry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201023074759.46605-1-laurent@oss.volkswagen.com \
    --to=laurent@oss.volkswagen.com \
    --cc=akpm@linux-foundation.org \
    --cc=almasrymina@google.com \
    --cc=linux-mm@kvack.org \
    --cc=mike.kravetz@oracle.com \
    --cc=rientjes@google.com \
    --cc=skhan@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).