| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6869aee5-0093-489d-bac7-fa718bdcb362@nvidia.com>
Date: Fri, 14 Nov 2025 12:24:51 -0800
From: John Hubbard <jhubbard@...dia.com>
To: peng8420.li@...il.com, linux-mm@...ck.org, akpm@...ux-foundation.org
Cc: david@...hat.com, osalvador@...e.de, jgg@...pe.ca, peterx@...hat.com,
linux-kernel@...r.kernel.org, dan.j.williams@...el.com
Subject: Re: [PATCH v2 1/2] Remove the "FOLL_TOUCH" test code from gup_test.c.
Hi,
The diffs look good, but the commit log and your replies have many minor
issues that add up. So you'll want to avoid them in the future, and with
that in mind, I've listed all of them, along with suggested alternatives.
On 11/14/25 8:11 AM, peng8420.li@...il.com wrote:
> From: "peng8420.li" <peng8420.li@...il.com>
This should be your real name, such as (my first guess)
"Peng Li" <peng8420.li@...il.com>.
Also, if you are sending from the same email address as your "From:",
then the "From:" item shouldn't normally show up (it's harmless here,
but could be cleaner without it). Check your .gitconfig send-email
options and you can make it go away.
Also, when replying, your email client is apparently using HTML, please
fix it to use text only!
Please read the patch submitter guidelines, too:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submit-checklist.rst
> Ever since commit 0f20bba1688b ("mm/gup: explicitly define and check
> internal GUP flags, disallow FOLL_TOUCH") we marked FOLL_TOUCH as a GUP-internal flag.
Not "ever since", but simply:
commit 0f20bba1688b ("mm/gup: explicitly define and check internal GUP
flags, disallow FOLL_TOUCH") marked FOLL_TOUCH as a GUP-internal flag.
Also, it's best to keep the line length within about 72 columns for
commit messages.
>
> Therefore, remove the "FOLL_TOUCH" test code from gup_test.c;
Trailing semicolon. Should be a period.
>
> Otherwise, executing the test command "./gup_test -L -r 100 -z" will report the following warning log:
> TAP version 13
> 1..1
> ENCHMARK: Time: [ 39.363371] WARNING: CPU: 1 PID: 117 at mm/gup.c:2512 is_valid_gup_args+0x66/0x8c
> get:2818 put:46 [ 39.364043] Modules linked in:
> us# , truncated [ 39.364311] CPU: 1 UID: 0 PID: 117 Comm: gup_test Not tainted 6.18.0-rc5-00324-gd09eaf415c87 #29 NONE
> (size: 0)#
> [ 39.364434] Hardware name: riscv-virtio,qemu (DT)
> [ 39.364546] epc : is_valid_gup_args+0x66/0x8c
> [ 39.364596] ra : pin_user_pages+0x38/0x78
> [ 39.364630] epc : ffffffff802079e6 ra : ffffffff8020c214 sp : ff2000000041bd20
> [ 39.364650] gp : ffffffff81a26068 tp : ff60000080b3b000 t0 : ff2000000041bdf8
> [ 39.364678] t1 : 000000000000001e t2 : 0000000000000000 s0 : ff2000000041bd30
> [ 39.364697] s1 : ff60000081300000 a0 : ff60000081300000 a1 : 0000000000000000
> [ 39.364714] a2 : ff2000000041bd3c a3 : 0000000000080000 a4 : 0000000000000001
> [ 39.364731] a5 : 0000000000010101 a6 : 0000000000000001 a7 : 0000000000000000
> [ 39.364747] s2 : 00007fff7eeed000 s3 : 0000000000000001 s4 : 00007fff7eeee000
> [ 39.364761] s5 : 00007fff7eeeb838 s6 : 0000000000000000 s7 : 00007fff7eeed000
> [ 39.364825] s8 : ff60000081300000 s9 : 0000000000000000 s10: 0000000000000002
> [ 39.364842] s11: 000000092869bfdc t3 : 2152ffffffffffc0 t4 : 00000000001fffff
> [ 39.364855] t5 : ffffffffffffffff t6 : 0000000000000000
> [ 39.364867] status: 0000000200000120 badaddr: ffffffff802079e6 cause: 0000000000000003
> [ 39.364949] [<ffffffff802079e6>] is_valid_gup_args+0x66/0x8c
> [ 39.365036] [<ffffffff8020c214>] pin_user_pages+0x38/0x78
> [ 39.365049] [<ffffffff80278f78>] gup_test_ioctl+0x2b4/0xc08
> [ 39.365060] [<ffffffff80297da6>] __riscv_sys_ioctl+0xba/0xc4
> [ 39.365072] [<ffffffff80b93f96>] do_trap_ecall_u+0x296/0x370
> [ 39.365093] [<ffffffff80b9ef06>] handle_exception+0x146/0x152
> [ 39.365220] ---[ end trace 0000000000000000 ]---
>
We don't need all of this output for such a simple case. Just the
warning will suffice, and even that is optional for this patch.
> Signed-off-by: peng8420.li <peng8420.li@...il.com>
Same rules for the Signed-off-by: real name needed here.
Combining all of that, your commit message would look like this:
From: "Peng Li" <peng8420.li@...il.com>
commit 0f20bba1688b ("mm/gup: explicitly define and check internal GUP
flags, disallow FOLL_TOUCH") marked FOLL_TOUCH as a GUP-internal flag.
This causes a warning to fire when running gup_test, for example:
$ ./gup_test -L -r 100 -z
dmesg:
WARNING: CPU: 1 PID: 117 at mm/gup.c:2512 is_valid_gup_args+0x66/0x8c
Therefore, remove the "FOLL_TOUCH" test code from gup_test.c.
Signed-off-by: Peng Li <peng8420.li@...il.com>
With the commit log fixed up, please feel free to add:
Reviewed-by: John Hubbard <jhubbard@...dia.com>
thanks,
--
John Hubbard
> ---
> tools/testing/selftests/mm/gup_test.c | 22 ++++------------------
> 1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/gup_test.c b/tools/testing/selftests/mm/gup_test.c
> index 8900b840c17a..75f7134d529d 100644
> --- a/tools/testing/selftests/mm/gup_test.c
> +++ b/tools/testing/selftests/mm/gup_test.c
> @@ -19,7 +19,6 @@
>
> /* Just the flags we need, copied from mm.h: */
> #define FOLL_WRITE 0x01 /* check pte is writable */
> -#define FOLL_TOUCH 0x02 /* mark page accessed */
>
> #define GUP_TEST_FILE "/sys/kernel/debug/gup_test"
>
> @@ -93,7 +92,7 @@ int main(int argc, char **argv)
> {
> struct gup_test gup = { 0 };
> int filed, i, opt, nr_pages = 1, thp = -1, write = 1, nthreads = 1, ret;
> - int flags = MAP_PRIVATE, touch = 0;
> + int flags = MAP_PRIVATE;
> char *file = "/dev/zero";
> pthread_t *tid;
> char *p;
> @@ -170,10 +169,6 @@ int main(int argc, char **argv)
> case 'H':
> flags |= (MAP_HUGETLB | MAP_ANONYMOUS);
> break;
> - case 'z':
> - /* fault pages in gup, do not fault in userland */
> - touch = 1;
> - break;
> default:
> ksft_exit_fail_msg("Wrong argument\n");
> }
> @@ -244,18 +239,9 @@ int main(int argc, char **argv)
> else if (thp == 0)
> madvise(p, size, MADV_NOHUGEPAGE);
>
> - /*
> - * FOLL_TOUCH, in gup_test, is used as an either/or case: either
> - * fault pages in from the kernel via FOLL_TOUCH, or fault them
> - * in here, from user space. This allows comparison of performance
> - * between those two cases.
> - */
> - if (touch) {
> - gup.gup_flags |= FOLL_TOUCH;
> - } else {
> - for (; (unsigned long)p < gup.addr + size; p += psize())
> - p[0] = 0;
> - }
> + /* Fault them in here, from user space. */
> + for (; (unsigned long)p < gup.addr + size; p += psize())
> + p[0] = 0;
>
> tid = malloc(sizeof(pthread_t) * nthreads);
> assert(tid);
Powered by blists - more mailing lists