From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw0-f198.google.com (mail-yw0-f198.google.com [209.85.161.198]) by kanga.kvack.org (Postfix) with ESMTP id 3FA5A6B0069 for ; Fri, 20 Jan 2017 01:49:48 -0500 (EST) Received: by mail-yw0-f198.google.com with SMTP id l19so87752545ywc.5 for ; Thu, 19 Jan 2017 22:49:48 -0800 (PST) Received: from mail-yw0-x243.google.com (mail-yw0-x243.google.com. [2607:f8b0:4002:c05::243]) by mx.google.com with ESMTPS id x9si1738048ybd.71.2017.01.19.22.49.47 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 19 Jan 2017 22:49:47 -0800 (PST) Received: by mail-yw0-x243.google.com with SMTP id 17so8823128ywk.2 for ; Thu, 19 Jan 2017 22:49:47 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20170119155701.GA24654@leverpostej> References: <20170119145114.GA19772@pjb1027-Latitude-E5410> <20170119155701.GA24654@leverpostej> From: park jinbum Date: Fri, 20 Jan 2017 15:49:46 +0900 Message-ID: Subject: Re: [PATCH] mm: add arch-independent testcases for RODATA Content-Type: multipart/alternative; boundary=001a114112183e359f0546810ee3 Sender: owner-linux-mm@kvack.org List-ID: To: Mark Rutland Cc: hpa@zytor.com, x86@kernel.org, akpm@linuxfoundation.org, keescook@chromium.org, linux-mm@kvack.org, arjan@linux.intel.com, mingo@redhat.com, tglx@linutronix.de, linux@armlinux.org.uk, kernel-janitors@vger.kernel.org, kernel-hardening@lists.openwall.com, labbott@redhat.com, linux-kernel@vger.kernel.org --001a114112183e359f0546810ee3 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Where is the best place for common test file in general?? kernel/rodata_test.c include/rodata_test.h =3D> Is it fine?? I can't see common file about rodata. So I'm confused where the best place is. 2017. 1. 20. =EC=98=A4=EC=A0=84 12:58=EC=97=90 "Mark Rutland" =EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1: > On Thu, Jan 19, 2017 at 11:51:14PM +0900, Jinbum Park wrote: > > This patch adds arch-independent testcases for RODATA. > > Both x86 and x86_64 already have testcases for RODATA, > > But they are arch-specific because using inline assembly directly. > > > > and cacheflush.h is not suitable location for rodata-test related thing= s. > > Since they were in cacheflush.h, > > If someone change the state of CONFIG_DEBUG_RODATA_TEST, > > It cause overhead of kernel build. > > > > To solve above issue, > > write arch-independent testcases and move it to shared location. (main.= c) > > This is clearly a rework and move of the existing x86 test, and not the > addition of a completely new test (see Arjan's comment about his credit > being removed...). > > I would recommend that you turn this into a series that makes the x86 > code generic, then moves it out into a common location where it can be > used by others. e.g. > > 1) make the test use put_user() > 2) move the rodata_test() call and the prototype to a common location > 3) move the test out to mm/ (with no changes to the file itself) > > Otherwise, comments below. > > > diff --git a/mm/rodata_test.c b/mm/rodata_test.c > > new file mode 100644 > > index 0000000..d5b0504 > > --- /dev/null > > +++ b/mm/rodata_test.c > > @@ -0,0 +1,63 @@ > > +/* > > + * rodata_test.c: functional test for mark_rodata_ro function > > + * > > + * (C) Copyright 2017 Jinbum Park > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License > > + * as published by the Free Software Foundation; version 2 > > + * of the License. > > + */ > > +#include > > +#include > > + > > +const int rodata_test_data =3D 0xC3; > > +EXPORT_SYMBOL_GPL(rodata_test_data); > > + > > +void rodata_test(void) > > +{ > > + unsigned long start, end, rodata_addr; > > + int zero =3D 0; > > + > > + /* prepare test */ > > + rodata_addr =3D ((unsigned long)&rodata_test_data); > > + > > + /* test 1: read the value */ > > + /* If this test fails, some previous testrun has clobbered the > state */ > > + if (!rodata_test_data) { > > + pr_err("rodata_test: test 1 fails (start data)\n"); > > + return; > > + } > > + > > + /* test 2: write to the variable; this should fault */ > > + /* > > + * This must be written in assembly to be able to catch the > > + * exception that is supposed to happen in the correct case. > > + * > > + * So that put_user macro is used to write arch-independent > assembly. > > + */ > > + if (!put_user(zero, (int *)rodata_addr)) { > > + pr_err("rodata_test: test data was not read only\n"); > > + return; > > + } > > As I mentioned in the original posting, you need to change to KERNEL_DS > for the put_user. > > Russell's suggestion to use probe_kernel_write() is strictly better; > please do that instead. > > Thanks, > Mark. > --001a114112183e359f0546810ee3 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Where is the best place for common test file in general??

=C2=A0kernel/r= odata_test.c=C2=A0
=C2=A0include/rodata_test.h =3D> Is it fine??

I can't se= e common file about rodata.
So I'm confused where the best place is.
<= /div>

2017. 1. 20.= =EC=98=A4=EC=A0=84 12:58=EC=97=90 "Mark Rutland" <mark.rutland@arm.com>=EB=8B=98=EC=9D=B4= =EC=9E=91=EC=84=B1:
On Thu, Jan 19, 2017 at 11:51:14PM +0900, Jinbum Park wrote:
> This patch adds arch-independent testcases for RODATA.
> Both x86 and x86_64 already have testcases for RODATA,
> But they are arch-specific because using inline assembly directly.
>
> and cacheflush.h is not suitable location for rodata-test related thin= gs.
> Since they were in cacheflush.h,
> If someone change the state of CONFIG_DEBUG_RODATA_TEST,
> It cause overhead of kernel build.
>
> To solve above issue,
> write arch-independent testcases and move it to shared location. (main= .c)

This is clearly a rework and move of the existing x86 test, and not the
addition of a completely new test (see Arjan's comment about his credit=
being removed...).

I would recommend that you turn this into a series that makes the x86
code generic, then moves it out into a common location where it can be
used by others. e.g.

1) make the test use put_user()
2) move the rodata_test() call and the prototype to a common location
3) move the test out to mm/ (with no changes to the file itself)

Otherwise, comments below.

> diff --git a/mm/rodata_test.c b/mm/rodata_test.c
> new file mode 100644
> index 0000000..d5b0504
> --- /dev/null
> +++ b/mm/rodata_test.c
> @@ -0,0 +1,63 @@
> +/*
> + * rodata_test.c: functional test for mark_rodata_ro function
> + *
> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +#include <asm/uaccess.h>
> +#include <asm/sections.h>
> +
> +const int rodata_test_data =3D 0xC3;
> +EXPORT_SYMBOL_GPL(rodata_test_data);
> +
> +void rodata_test(void)
> +{
> +=C2=A0 =C2=A0 =C2=A0unsigned long start, end, rodata_addr;
> +=C2=A0 =C2=A0 =C2=A0int zero =3D 0;
> +
> +=C2=A0 =C2=A0 =C2=A0/* prepare test */
> +=C2=A0 =C2=A0 =C2=A0rodata_addr =3D ((unsigned long)&rodata_test_= data);
> +
> +=C2=A0 =C2=A0 =C2=A0/* test 1: read the value */
> +=C2=A0 =C2=A0 =C2=A0/* If this test fails, some previous testrun has = clobbered the state */
> +=C2=A0 =C2=A0 =C2=A0if (!rodata_test_data) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pr_err("rodata_t= est: test 1 fails (start data)\n");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0/* test 2: write to the variable; this should fau= lt */
> +=C2=A0 =C2=A0 =C2=A0/*
> +=C2=A0 =C2=A0 =C2=A0 * This must be written in assembly to be able to= catch the
> +=C2=A0 =C2=A0 =C2=A0 * exception that is supposed to happen in the co= rrect case.
> +=C2=A0 =C2=A0 =C2=A0 *
> +=C2=A0 =C2=A0 =C2=A0 * So that put_user macro is used to write arch-i= ndependent assembly.
> +=C2=A0 =C2=A0 =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0if (!put_user(zero, (int *)rodata_addr)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pr_err("rodata_t= est: test data was not read only\n");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
> +=C2=A0 =C2=A0 =C2=A0}

As I mentioned in the original posting, you need to change to KERNEL_DS
for the put_user.

Russell's suggestion to use probe_kernel_write() is strictly better; please do that instead.

Thanks,
Mark.
--001a114112183e359f0546810ee3-- -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org