diff mbox series

[v4] mm/ksm: Fix NULL pointer dereference when KSM zero page is enabled

Message ID 20200416025034.29780-1-songmuchun@bytedance.com (mailing list archive)
State New, archived
Headers show
Series [v4] mm/ksm: Fix NULL pointer dereference when KSM zero page is enabled | expand

Commit Message

Muchun Song April 16, 2020, 2:50 a.m. UTC 
The find_mergeable_vma can return NULL. In this case, it leads
to a crash when we access vm_mm(its offset is 0x40) later in
write_protect_page. And this case did happen on our server. The
following call trace is captured in kernel 4.19 with the following
patch applied and KSM zero page enabled on our server.

  commit e86c59b1b12d ("mm/ksm: improve deduplication of zero pages with colouring")

So add a vma check to fix it.

--------------------------------------------------------------------------
  BUG: unable to handle kernel NULL pointer dereference at 0000000000000040
  Oops: 0000 [#1] SMP NOPTI
  CPU: 9 PID: 510 Comm: ksmd Kdump: loaded Tainted: G OE 4.19.36.bsk.9-amd64 #4.19.36.bsk.9
  RIP: 0010:try_to_merge_one_page+0xc7/0x760
  Code: 24 58 65 48 33 34 25 28 00 00 00 89 e8 0f 85 a3 06 00 00 48 83 c4
        60 5b 5d 41 5c 41 5d 41 5e 41 5f c3 48 8b 46 08 a8 01 75 b8 <49>
        8b 44 24 40 4c 8d 7c 24 20 b9 07 00 00 00 4c 89 e6 4c 89 ff 48
  RSP: 0018:ffffadbdd9fffdb0 EFLAGS: 00010246
  RAX: ffffda83ffd4be08 RBX: ffffda83ffd4be40 RCX: 0000002c6e800000
  RDX: 0000000000000000 RSI: ffffda83ffd4be40 RDI: 0000000000000000
  RBP: ffffa11939f02ec0 R08: 0000000094e1a447 R09: 00000000abe76577
  R10: 0000000000000962 R11: 0000000000004e6a R12: 0000000000000000
  R13: ffffda83b1e06380 R14: ffffa18f31f072c0 R15: ffffda83ffd4be40
  FS: 0000000000000000(0000) GS:ffffa0da43b80000(0000) knlGS:0000000000000000
  CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000000000000040 CR3: 0000002c77c0a003 CR4: 00000000007626e0
  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  PKRU: 55555554
  Call Trace:
    ? follow_page_pte+0x36d/0x5e0
    ksm_scan_thread+0x115e/0x1960
    ? remove_wait_queue+0x60/0x60
    kthread+0xf5/0x130
    ? try_to_merge_with_ksm_page+0x90/0x90
    ? kthread_create_worker_on_cpu+0x70/0x70
    ret_from_fork+0x1f/0x30
--------------------------------------------------------------------------

Fixes: e86c59b1b12d ("mm/ksm: improve deduplication of zero pages with colouring")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Co-developed-by: Xiongchun Duan <duanxiongchun@bytedance.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---

Change in v4:
    1. Update commit message.
    2. If the vma is out of date, just exit.

Change in v3:
    1. Update "Signed-off-by" to "Co-developed-by"
    2. Update commit message

Change in v2:
    1. Update commit message.
    2. Update patch subject from:
         "mm/ksm: Fix kernel NULL pointer dereference at 0000000000000040"
       to:
         "mm/ksm: Fix NULL pointer dereference when KSM zero page is enabled"

 mm/ksm.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

--
2.11.0

Comments

Andrew Morton April 16, 2020, 2:58 a.m. UTC | #1 
On Thu, 16 Apr 2020 10:50:34 +0800 Muchun Song <songmuchun@bytedance.com> wrote:

> The find_mergeable_vma can return NULL. In this case, it leads
> to a crash when we access vm_mm(its offset is 0x40) later in
> write_protect_page. And this case did happen on our server. The
> following call trace is captured in kernel 4.19 with the following
> patch applied and KSM zero page enabled on our server.
> 
>   commit e86c59b1b12d ("mm/ksm: improve deduplication of zero pages with colouring")
> 
> So add a vma check to fix it.
>
> ...
>
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2112,8 +2112,15 @@ static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
> 
>  		down_read(&mm->mmap_sem);
>  		vma = find_mergeable_vma(mm, rmap_item->address);
> -		err = try_to_merge_one_page(vma, page,
> -					    ZERO_PAGE(rmap_item->address));
> +		if (vma)
> +			err = try_to_merge_one_page(vma, page,
> +					ZERO_PAGE(rmap_item->address));
> +		else
> +			/**
> +			 * If the vma is out of date, we do not need to
> +			 * continue.
> +			 */
> +			err = 0;
>  		up_read(&mm->mmap_sem);
>  		/*
>  		 * In case of failure, the page was not really empty, so we

Thanks.

It's conventional to put braces around multi-line blocks such as this.

Also, /** is specifically used to introduce kerneldoc comments.  This
comment is not a kerneldoc one so use /*.

--- a/mm/ksm.c~mm-ksm-fix-null-pointer-dereference-when-ksm-zero-page-is-enabled-v4-fix
+++ a/mm/ksm.c
@@ -2112,15 +2112,16 @@ static void cmp_and_merge_page(struct pa
 
 		down_read(&mm->mmap_sem);
 		vma = find_mergeable_vma(mm, rmap_item->address);
-		if (vma)
+		if (vma) {
 			err = try_to_merge_one_page(vma, page,
 					ZERO_PAGE(rmap_item->address));
-		else
-			/**
+		} else {
+			/*
 			 * If the vma is out of date, we do not need to
 			 * continue.
 			 */
 			err = 0;
+		}
 		up_read(&mm->mmap_sem);
 		/*
 		 * In case of failure, the page was not really empty, so we
Markus Elfring April 16, 2020, 6 a.m. UTC | #2 
> to a crash when we access vm_mm(its offset is 0x40) later in

Would the text variant “… vm_mm (its …” be a bit nicer?


…
> +++ b/mm/ksm.c
> @@ -2112,8 +2112,15 @@ static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
…
> +		if (vma)
> +			err = try_to_merge_one_page(vma, page,
> +					ZERO_PAGE(rmap_item->address));

Can the parameter alignment trigger further software development considerations
for such a function call?

Regards,
Markus
Markus Elfring April 16, 2020, 6:14 a.m. UTC | #3 
…
>> +++ b/mm/ksm.c
>> @@ -2112,8 +2112,15 @@ static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
…
>> +		if (vma)
>> +			err = try_to_merge_one_page(vma, page,
>> +					ZERO_PAGE(rmap_item->address));
>> +		else
>> +			/**
>> +			 * If the vma is out of date, we do not need to
>> +			 * continue.
>> +			 */
>> +			err = 0;
>>  		up_read(&mm->mmap_sem);
…
> It's conventional to put braces around multi-line blocks such as this.

Are there different views to consider around the usage of single statements
together with curly brackets in if branches?

Regards,
Markus
Kirill Tkhai April 16, 2020, 11:20 a.m. UTC | #4 
On 16.04.2020 09:14, Markus Elfring wrote:
> …
>>> +++ b/mm/ksm.c
>>> @@ -2112,8 +2112,15 @@ static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
> …
>>> +		if (vma)
>>> +			err = try_to_merge_one_page(vma, page,
>>> +					ZERO_PAGE(rmap_item->address));
>>> +		else
>>> +			/**
>>> +			 * If the vma is out of date, we do not need to
>>> +			 * continue.
>>> +			 */
>>> +			err = 0;
>>>  		up_read(&mm->mmap_sem);
> …
>> It's conventional to put braces around multi-line blocks such as this.
> 
> Are there different views to consider around the usage of single statements
> together with curly brackets in if branches?

For me Andrew's conversion is the best readable. I try to comment the code
the same way myself. I even thought it's kernel default style :)

Kirill
Matthew Wilcox April 16, 2020, 11:21 a.m. UTC | #5 
On Thu, Apr 16, 2020 at 10:50:34AM +0800, Muchun Song wrote:
> +			/**
> +			 * If the vma is out of date, we do not need to
> +			 * continue.
> +			 */

This comment is not in kernel-doc format and should not start with '/**'.
Joe Perches April 16, 2020, 8:05 p.m. UTC | #6 
On Thu, 2020-04-16 at 14:20 +0300, Kirill Tkhai wrote:
> On 16.04.2020 09:14, Markus Elfring wrote:
> > …
> > > > +++ b/mm/ksm.c
> > > > @@ -2112,8 +2112,15 @@ static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)
> > …
> > > > +		if (vma)
> > > > +			err = try_to_merge_one_page(vma, page,
> > > > +					ZERO_PAGE(rmap_item->address));
> > > > +		else
> > > > +			/**
> > > > +			 * If the vma is out of date, we do not need to
> > > > +			 * continue.

trivia:

It's generally better to not use "/**" as that's used for kernel-doc
and this could be a single line like

+			/* If the vma is out of date, no need to continue */

> > > It's conventional to put braces around multi-line blocks such as this.

true

> > Are there different views to consider around the usage of single statements
> > together with curly brackets in if branches?

no
diff mbox series

Patch

diff --git a/mm/ksm.c b/mm/ksm.c
index a558da9e71770..15339538da299 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2112,8 +2112,15 @@  static void cmp_and_merge_page(struct page *page, struct rmap_item *rmap_item)

 		down_read(&mm->mmap_sem);
 		vma = find_mergeable_vma(mm, rmap_item->address);
-		err = try_to_merge_one_page(vma, page,
-					    ZERO_PAGE(rmap_item->address));
+		if (vma)
+			err = try_to_merge_one_page(vma, page,
+					ZERO_PAGE(rmap_item->address));
+		else
+			/**
+			 * If the vma is out of date, we do not need to
+			 * continue.
+			 */
+			err = 0;
 		up_read(&mm->mmap_sem);
 		/*
 		 * In case of failure, the page was not really empty, so we