From: Andrew Morton <akpm@linux-foundation.org>
To: Dongli Zhang <dongli.zhang@oracle.com>
Cc: linux-mm@kvack.org, cl@linux.com, penberg@kernel.org,
rientjes@google.com, iamjoonsoo.kim@lge.com,
linux-kernel@vger.kernel.org, joe.jin@oracle.com
Subject: Re: [PATCH 1/1] mm: slub: fix corrupted freechain in deactivate_slab()
Date: Fri, 17 Apr 2020 18:12:19 -0700 [thread overview]
Message-ID: <20200417181219.bef9b2f9ade92bf3798e3622@linux-foundation.org> (raw)
In-Reply-To: <20200331031450.12182-1-dongli.zhang@oracle.com>
On Mon, 30 Mar 2020 20:14:50 -0700 Dongli Zhang <dongli.zhang@oracle.com> wrote:
> The slub_debug is able to fix the corrupted slab freelist/page. However,
> alloc_debug_processing() only checks the validity of current and next
> freepointer during allocation path. As a result, once some objects have
> their freepointers corrupted, deactivate_slab() may lead to page fault.
>
> Below is from a test kernel module when
> 'slub_debug=PUF,kmalloc-128 slub_nomerge'. The test kernel corrupts the
> freepointer of one free object on purpose. Unfortunately, deactivate_slab()
> does not detect it when iterating the freechain.
>
> ...
>
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2082,6 +2082,20 @@ static void deactivate_slab(struct kmem_cache *s, struct page *page,
> void *prior;
> unsigned long counters;
>
> + if ((s->flags & SLAB_CONSISTENCY_CHECKS) &&
> + !check_valid_pointer(s, page, nextfree)) {
> + /*
> + * If 'nextfree' is invalid, it is possible that
> + * the object at 'freelist' is already corrupted.
> + * Therefore, all objects starting at 'freelist'
> + * are isolated.
> + */
> + object_err(s, page, freelist, "Freechain corrupt");
> + freelist = NULL;
> + slab_fix(s, "Isolate corrupted freechain");
> + break;
> + }
> +
> do {
> prior = page->freelist;
> counters = page->counters;
We could do it this way:
--- a/mm/slub.c~mm-slub-fix-corrupted-freechain-in-deactivate_slab-fix
+++ a/mm/slub.c
@@ -2083,6 +2083,7 @@ static void deactivate_slab(struct kmem_
void *prior;
unsigned long counters;
+#ifdef CONFIG_SLAB_DEBUG
if ((s->flags & SLAB_CONSISTENCY_CHECKS) &&
!check_valid_pointer(s, page, nextfree)) {
/*
@@ -2096,6 +2097,7 @@ static void deactivate_slab(struct kmem_
slab_fix(s, "Isolate corrupted freechain");
break;
}
+#endif
do {
prior = page->freelist;
But it's a bit ugly. How about this?
--- a/mm/slub.c~mm-slub-fix-corrupted-freechain-in-deactivate_slab-fix
+++ a/mm/slub.c
@@ -650,6 +650,20 @@ static void slab_bug(struct kmem_cache *
va_end(args);
}
+static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
+ void *freelist, void *nextfree)
+{
+ if ((s->flags & SLAB_CONSISTENCY_CHECKS) &&
+ !check_valid_pointer(s, page, nextfree)) {
+ object_err(s, page, freelist, "Freechain corrupt");
+ freelist = NULL;
+ slab_fix(s, "Isolate corrupted freechain");
+ return true;
+ }
+
+ return false;
+}
+
static void slab_fix(struct kmem_cache *s, char *fmt, ...)
{
struct va_format vaf;
@@ -1400,6 +1414,11 @@ static inline void inc_slabs_node(struct
static inline void dec_slabs_node(struct kmem_cache *s, int node,
int objects) {}
+static bool freelist_corrupted(struct kmem_cache *s, struct page *page,
+ void *freelist, void *nextfree)
+{
+ return false;
+}
#endif /* CONFIG_SLUB_DEBUG */
/*
@@ -2083,19 +2102,13 @@ static void deactivate_slab(struct kmem_
void *prior;
unsigned long counters;
- if ((s->flags & SLAB_CONSISTENCY_CHECKS) &&
- !check_valid_pointer(s, page, nextfree)) {
- /*
- * If 'nextfree' is invalid, it is possible that
- * the object at 'freelist' is already corrupted.
- * Therefore, all objects starting at 'freelist'
- * are isolated.
- */
- object_err(s, page, freelist, "Freechain corrupt");
- freelist = NULL;
- slab_fix(s, "Isolate corrupted freechain");
+ /*
+ * If 'nextfree' is invalid, it is possible that the object at
+ * 'freelist' is already corrupted. So isolate all objects
+ * starting at 'freelist'.
+ */
+ if (freelist_corrupted(s, page, freelist, nextfree))
break;
- }
do {
prior = page->freelist;
_
next prev parent reply other threads:[~2020-04-18 1:12 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-31 3:14 [PATCH 1/1] mm: slub: fix corrupted freechain in deactivate_slab() Dongli Zhang
2020-04-12 22:20 ` Dongli Zhang
2020-04-18 1:12 ` Andrew Morton [this message]
2020-04-18 1:56 ` Dongli Zhang
2020-04-18 20:04 ` Andrew Morton
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=20200417181219.bef9b2f9ade92bf3798e3622@linux-foundation.org \
--to=akpm@linux-foundation.org \
--cc=cl@linux.com \
--cc=dongli.zhang@oracle.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=joe.jin@oracle.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
/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).