Now that all other kernel callers of get_user_pages(FOLL_LONGTERM)
have been converted to pin_longterm_pages(), lock it down:
1) Add an assertion to get_user_pages(), preventing callers from
passing FOLL_LONGTERM (in addition to the existing assertion that
prevents FOLL_PIN).
2) Remove the associated GUP_LONGTERM_BENCHMARK test.
Signed-off-by: John Hubbard <[email protected]>
---
mm/gup.c | 8 ++++----
mm/gup_benchmark.c | 9 +--------
tools/testing/selftests/vm/gup_benchmark.c | 7 ++-----
3 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 82e7e4ce5027..90f5f95ee7ac 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1756,11 +1756,11 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,
struct vm_area_struct **vmas)
{
/*
- * FOLL_PIN must only be set internally by the pin_user_page*() and
- * pin_longterm_*() APIs, never directly by the caller, so enforce that
- * with an assertion:
+ * FOLL_PIN and FOLL_LONGTERM must only be set internally by the
+ * pin_user_page*() and pin_longterm_*() APIs, never directly by the
+ * caller, so enforce that with an assertion:
*/
- if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
+ if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_LONGTERM)))
return -EINVAL;
return __gup_longterm_locked(current, current->mm, start, nr_pages,
diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
index 8f980d91dbf5..679f0e6a0adb 100644
--- a/mm/gup_benchmark.c
+++ b/mm/gup_benchmark.c
@@ -6,7 +6,7 @@
#include <linux/debugfs.h>
#define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
-#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
+/* Command 2 has been deleted. */
#define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)
#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
#define PIN_LONGTERM_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
@@ -28,7 +28,6 @@ static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)
switch (cmd) {
case GUP_FAST_BENCHMARK:
- case GUP_LONGTERM_BENCHMARK:
case GUP_BENCHMARK:
for (i = 0; i < nr_pages; i++)
put_page(pages[i]);
@@ -97,11 +96,6 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
nr = get_user_pages_fast(addr, nr, gup->flags,
pages + i);
break;
- case GUP_LONGTERM_BENCHMARK:
- nr = get_user_pages(addr, nr,
- gup->flags | FOLL_LONGTERM,
- pages + i, NULL);
- break;
case GUP_BENCHMARK:
nr = get_user_pages(addr, nr, gup->flags, pages + i,
NULL);
@@ -159,7 +153,6 @@ static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
switch (cmd) {
case GUP_FAST_BENCHMARK:
- case GUP_LONGTERM_BENCHMARK:
case GUP_BENCHMARK:
case PIN_FAST_BENCHMARK:
case PIN_LONGTERM_BENCHMARK:
diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
index 03928e47a86f..836b7082db13 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -15,7 +15,7 @@
#define PAGE_SIZE sysconf(_SC_PAGESIZE)
#define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
-#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
+/* Command 2 has been deleted. */
#define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)
/*
@@ -49,7 +49,7 @@ int main(int argc, char **argv)
char *file = "/dev/zero";
char *p;
- while ((opt = getopt(argc, argv, "m:r:n:f:abctTLUuwSH")) != -1) {
+ while ((opt = getopt(argc, argv, "m:r:n:f:abctTUuwSH")) != -1) {
switch (opt) {
case 'a':
cmd = PIN_FAST_BENCHMARK;
@@ -75,9 +75,6 @@ int main(int argc, char **argv)
case 'T':
thp = 0;
break;
- case 'L':
- cmd = GUP_LONGTERM_BENCHMARK;
- break;
case 'U':
cmd = GUP_BENCHMARK;
break;
--
2.24.0
On Tue, Nov 12, 2019 at 08:27:10PM -0800, John Hubbard wrote:
> Now that all other kernel callers of get_user_pages(FOLL_LONGTERM)
> have been converted to pin_longterm_pages(), lock it down:
>
> 1) Add an assertion to get_user_pages(), preventing callers from
> passing FOLL_LONGTERM (in addition to the existing assertion that
> prevents FOLL_PIN).
>
> 2) Remove the associated GUP_LONGTERM_BENCHMARK test.
>
> Signed-off-by: John Hubbard <[email protected]>
> ---
> mm/gup.c | 8 ++++----
> mm/gup_benchmark.c | 9 +--------
> tools/testing/selftests/vm/gup_benchmark.c | 7 ++-----
> 3 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 82e7e4ce5027..90f5f95ee7ac 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1756,11 +1756,11 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,
> struct vm_area_struct **vmas)
> {
> /*
> - * FOLL_PIN must only be set internally by the pin_user_page*() and
> - * pin_longterm_*() APIs, never directly by the caller, so enforce that
> - * with an assertion:
> + * FOLL_PIN and FOLL_LONGTERM must only be set internally by the
> + * pin_user_page*() and pin_longterm_*() APIs, never directly by the
> + * caller, so enforce that with an assertion:
> */
> - if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
> + if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_LONGTERM)))
Don't we want to block FOLL_LONGTERM in get_user_pages_fast() as well after all
this?
Ira
> return -EINVAL;
>
> return __gup_longterm_locked(current, current->mm, start, nr_pages,
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index 8f980d91dbf5..679f0e6a0adb 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -6,7 +6,7 @@
> #include <linux/debugfs.h>
>
> #define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
> -#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
> +/* Command 2 has been deleted. */
> #define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)
> #define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
> #define PIN_LONGTERM_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
> @@ -28,7 +28,6 @@ static void put_back_pages(int cmd, struct page **pages, unsigned long nr_pages)
>
> switch (cmd) {
> case GUP_FAST_BENCHMARK:
> - case GUP_LONGTERM_BENCHMARK:
> case GUP_BENCHMARK:
> for (i = 0; i < nr_pages; i++)
> put_page(pages[i]);
> @@ -97,11 +96,6 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> nr = get_user_pages_fast(addr, nr, gup->flags,
> pages + i);
> break;
> - case GUP_LONGTERM_BENCHMARK:
> - nr = get_user_pages(addr, nr,
> - gup->flags | FOLL_LONGTERM,
> - pages + i, NULL);
> - break;
> case GUP_BENCHMARK:
> nr = get_user_pages(addr, nr, gup->flags, pages + i,
> NULL);
> @@ -159,7 +153,6 @@ static long gup_benchmark_ioctl(struct file *filep, unsigned int cmd,
>
> switch (cmd) {
> case GUP_FAST_BENCHMARK:
> - case GUP_LONGTERM_BENCHMARK:
> case GUP_BENCHMARK:
> case PIN_FAST_BENCHMARK:
> case PIN_LONGTERM_BENCHMARK:
> diff --git a/tools/testing/selftests/vm/gup_benchmark.c b/tools/testing/selftests/vm/gup_benchmark.c
> index 03928e47a86f..836b7082db13 100644
> --- a/tools/testing/selftests/vm/gup_benchmark.c
> +++ b/tools/testing/selftests/vm/gup_benchmark.c
> @@ -15,7 +15,7 @@
> #define PAGE_SIZE sysconf(_SC_PAGESIZE)
>
> #define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
> -#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
> +/* Command 2 has been deleted. */
> #define GUP_BENCHMARK _IOWR('g', 3, struct gup_benchmark)
>
> /*
> @@ -49,7 +49,7 @@ int main(int argc, char **argv)
> char *file = "/dev/zero";
> char *p;
>
> - while ((opt = getopt(argc, argv, "m:r:n:f:abctTLUuwSH")) != -1) {
> + while ((opt = getopt(argc, argv, "m:r:n:f:abctTUuwSH")) != -1) {
> switch (opt) {
> case 'a':
> cmd = PIN_FAST_BENCHMARK;
> @@ -75,9 +75,6 @@ int main(int argc, char **argv)
> case 'T':
> thp = 0;
> break;
> - case 'L':
> - cmd = GUP_LONGTERM_BENCHMARK;
> - break;
> case 'U':
> cmd = GUP_BENCHMARK;
> break;
> --
> 2.24.0
>
On 11/13/19 11:09 AM, Ira Weiny wrote:
...
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 82e7e4ce5027..90f5f95ee7ac 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -1756,11 +1756,11 @@ long get_user_pages(unsigned long start, unsigned long nr_pages,
>> struct vm_area_struct **vmas)
>> {
>> /*
>> - * FOLL_PIN must only be set internally by the pin_user_page*() and
>> - * pin_longterm_*() APIs, never directly by the caller, so enforce that
>> - * with an assertion:
>> + * FOLL_PIN and FOLL_LONGTERM must only be set internally by the
>> + * pin_user_page*() and pin_longterm_*() APIs, never directly by the
>> + * caller, so enforce that with an assertion:
>> */
>> - if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
>> + if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_LONGTERM)))
>
> Don't we want to block FOLL_LONGTERM in get_user_pages_fast() as well after all
> this?
>
Yes. But with the latest idea to restore FOLL_LONGTERM to its original glory,
that won't be an issue in the next version. heh.
thanks,
--
John Hubbard
NVIDIA