Sumit,
I did not see a follow up on this series per your last email.[1] I'd like to
move forward with getting rid of kmap_to_page(). So Hopefully this can land
and you can build on this rather than the other way around?
All,
Al Viro found[2] that kmap_to_page() is broken. But not only is it broken, it
presents confusion over how highmem should be used because kmap() and friends
should not be used for 'long term' mappings.
get_kernel_pages() is a caller of kmap_to_page(). It only has one caller
[shm_get_kernel_pages()] which does not need the functionality.
Alter shm_get_kernel_pages() to no longer call get_kernel_pages() and remove
get_kernel_pages(). Along the way it was noted that shm_get_kernel_pages()
does not have any need to support vmalloc'ed addresses either. Remove that
functionality to clean up the logic.
This series also fixes is_kmap_addr() and uses it to ensure no kmap addresses
slip in later.
[1] https://lore.kernel.org/all/CAFA6WYMqEVDVW-ifoh-V9ni1zntYdes8adQKf2XXAUpqdaW53w@mail.gmail.com/
[2] https://lore.kernel.org/lkml/YzSSl1ItVlARDvG3@ZenIV
To: Sumit Garg <[email protected]>
To: Andrew Morton <[email protected]>
Cc: "Al Viro" <[email protected]>
Cc: "Christoph Hellwig" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Jens Wiklander <[email protected]>
Cc: "Fabio M. De Francesco" <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
Changes in v2:
- Al Viro: Avoid allocating the kiov.
- Sumit: Update cover letter to clarify the motivation behind removing
get_kernel_pages()
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Ira Weiny (4):
highmem: Enhance is_kmap_addr() to check kmap_local_page() mappings
tee: Remove vmalloc page support
tee: Remove call to get_kernel_pages()
mm: Remove get_kernel_pages()
drivers/tee/tee_shm.c | 37 ++++++++++---------------------------
include/linux/highmem-internal.h | 5 ++++-
include/linux/mm.h | 2 --
mm/swap.c | 30 ------------------------------
4 files changed, 14 insertions(+), 60 deletions(-)
---
base-commit: 0136d86b78522bbd5755f8194c97a987f0586ba5
change-id: 20230203-get_kernel_pages-199342cfba79
Best regards,
--
Ira Weiny <[email protected]>
is_kmap_addr() is only looking at the kmap() address range which may
cause check_heap_object() to miss checking an overflow on a
kmap_local_page() page.
Add a check for the kmap_local_page() address range to is_kmap_addr().
Cc: Matthew Wilcox <[email protected]>
Cc: Al Viro <[email protected]>
Cc: "Fabio M. De Francesco" <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
include/linux/highmem-internal.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index e098f38422af..a3028e400a9c 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -152,7 +152,10 @@ static inline void totalhigh_pages_add(long count)
static inline bool is_kmap_addr(const void *x)
{
unsigned long addr = (unsigned long)x;
- return addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP);
+
+ return (addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP)) ||
+ (addr >= __fix_to_virt(FIX_KMAP_END) &&
+ addr < __fix_to_virt(FIX_KMAP_BEGIN));
}
#else /* CONFIG_HIGHMEM */
--
2.39.1
The kernel pages used by shm_get_kernel_pages() are allocated using
GFP_KERNEL through the following call stack:
trusted_instantiate()
trusted_payload_alloc() -> GFP_KERNEL
<trusted key op>
tee_shm_register_kernel_buf()
register_shm_helper()
shm_get_kernel_pages()
Where <trusted key op> is one of:
trusted_key_unseal()
trusted_key_get_random()
trusted_key_seal()
Remove the vmalloc page support from shm_get_kernel_pages(). Replace
with a warn on once.
Cc: Al Viro <[email protected]>
Cc: "Fabio M. De Francesco" <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Linus Torvalds <[email protected]>
Reviewed-by: Jens Wiklander <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
drivers/tee/tee_shm.c | 36 ++++++++++++------------------------
1 file changed, 12 insertions(+), 24 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 27295bda3e0b..527a6eabc03e 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -24,37 +24,25 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count)
static int shm_get_kernel_pages(unsigned long start, size_t page_count,
struct page **pages)
{
+ struct kvec *kiov;
size_t n;
int rc;
- if (is_vmalloc_addr((void *)start)) {
- struct page *page;
-
- for (n = 0; n < page_count; n++) {
- page = vmalloc_to_page((void *)(start + PAGE_SIZE * n));
- if (!page)
- return -ENOMEM;
-
- get_page(page);
- pages[n] = page;
- }
- rc = page_count;
- } else {
- struct kvec *kiov;
-
- kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
- if (!kiov)
- return -ENOMEM;
+ if (WARN_ON_ONCE(is_vmalloc_addr((void *)start)))
+ return -EINVAL;
- for (n = 0; n < page_count; n++) {
- kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
- kiov[n].iov_len = PAGE_SIZE;
- }
+ kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
+ if (!kiov)
+ return -ENOMEM;
- rc = get_kernel_pages(kiov, page_count, 0, pages);
- kfree(kiov);
+ for (n = 0; n < page_count; n++) {
+ kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
+ kiov[n].iov_len = PAGE_SIZE;
}
+ rc = get_kernel_pages(kiov, page_count, 0, pages);
+ kfree(kiov);
+
return rc;
}
--
2.39.1
The kernel pages used by shm_get_kernel_pages() are allocated using
GFP_KERNEL through the following call stack:
trusted_instantiate()
trusted_payload_alloc() -> GFP_KERNEL
<trusted key op>
tee_shm_register_kernel_buf()
register_shm_helper()
shm_get_kernel_pages()
Where <trusted key op> is one of:
trusted_key_unseal()
trusted_key_get_random()
trusted_key_seal()
Because the pages can't be from highmem get_kernel_pages() boils down to
a get_page() call.
Remove the get_kernel_pages() call and open code the get_page().
In case a highmem page does slip through warn on once for a kmap'ed
address.
Cc: Jens Wiklander <[email protected]>
Cc: Al Viro <[email protected]>
Cc: "Fabio M. De Francesco" <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Linus Torvalds <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
Changes from v1:
Al/Christoph: Remove kiov altogether
---
drivers/tee/tee_shm.c | 21 ++++++++-------------
1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 527a6eabc03e..b1c6231defad 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -11,6 +11,7 @@
#include <linux/tee_drv.h>
#include <linux/uaccess.h>
#include <linux/uio.h>
+#include <linux/highmem.h>
#include "tee_private.h"
static void shm_put_kernel_pages(struct page **pages, size_t page_count)
@@ -24,26 +25,20 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count)
static int shm_get_kernel_pages(unsigned long start, size_t page_count,
struct page **pages)
{
- struct kvec *kiov;
+ struct page *page;
size_t n;
- int rc;
- if (WARN_ON_ONCE(is_vmalloc_addr((void *)start)))
+ if (WARN_ON_ONCE(is_vmalloc_addr((void *)start) ||
+ is_kmap_addr((void *)start)))
return -EINVAL;
- kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
- if (!kiov)
- return -ENOMEM;
-
+ page = virt_to_page(start);
for (n = 0; n < page_count; n++) {
- kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
- kiov[n].iov_len = PAGE_SIZE;
+ pages[n] = page + n;
+ get_page(pages[n]);
}
- rc = get_kernel_pages(kiov, page_count, 0, pages);
- kfree(kiov);
-
- return rc;
+ return page_count;
}
static void release_registered_pages(struct tee_shm *shm)
--
2.39.1
The only caller to get_kernel_pages() [shm_get_kernel_pages()] has been
updated to not need it.
Remove get_kernel_pages().
Cc: Mel Gorman <[email protected]>
Cc: Al Viro <[email protected]>
Cc: "Fabio M. De Francesco" <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Andrew Morton <[email protected]>
Acked-by: John Hubbard <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
include/linux/mm.h | 2 --
mm/swap.c | 30 ------------------------------
2 files changed, 32 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 8f857163ac89..2041e6d4fa27 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2095,8 +2095,6 @@ int __account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc,
struct task_struct *task, bool bypass_rlim);
struct kvec;
-int get_kernel_pages(const struct kvec *iov, int nr_pages, int write,
- struct page **pages);
struct page *get_dump_page(unsigned long addr);
bool folio_mark_dirty(struct folio *folio);
diff --git a/mm/swap.c b/mm/swap.c
index 70e2063ef43a..4c03ecab698e 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -158,36 +158,6 @@ void put_pages_list(struct list_head *pages)
}
EXPORT_SYMBOL(put_pages_list);
-/*
- * get_kernel_pages() - pin kernel pages in memory
- * @kiov: An array of struct kvec structures
- * @nr_segs: number of segments to pin
- * @write: pinning for read/write, currently ignored
- * @pages: array that receives pointers to the pages pinned.
- * Should be at least nr_segs long.
- *
- * Returns number of pages pinned. This may be fewer than the number requested.
- * If nr_segs is 0 or negative, returns 0. If no pages were pinned, returns 0.
- * Each page returned must be released with a put_page() call when it is
- * finished with.
- */
-int get_kernel_pages(const struct kvec *kiov, int nr_segs, int write,
- struct page **pages)
-{
- int seg;
-
- for (seg = 0; seg < nr_segs; seg++) {
- if (WARN_ON(kiov[seg].iov_len != PAGE_SIZE))
- return seg;
-
- pages[seg] = kmap_to_page(kiov[seg].iov_base);
- get_page(pages[seg]);
- }
-
- return seg;
-}
-EXPORT_SYMBOL_GPL(get_kernel_pages);
-
typedef void (*move_fn_t)(struct lruvec *lruvec, struct folio *folio);
static void lru_add_fn(struct lruvec *lruvec, struct folio *folio)
--
2.39.1
On Fri, Feb 03, 2023 at 08:06:32PM -0800, Ira Weiny wrote:
> - return addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP);
> +
> + return (addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP)) ||
> + (addr >= __fix_to_virt(FIX_KMAP_END) &&
> + addr < __fix_to_virt(FIX_KMAP_BEGIN));
Isn't the second check inverted?
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
Christoph Hellwig wrote:
> On Fri, Feb 03, 2023 at 08:06:32PM -0800, Ira Weiny wrote:
> > - return addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP);
> > +
> > + return (addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP)) ||
> > + (addr >= __fix_to_virt(FIX_KMAP_END) &&
> > + addr < __fix_to_virt(FIX_KMAP_BEGIN));
>
> Isn't the second check inverted?
>
The enum map runs from top down. So I believe this is correct. I tested
it with a different series and it worked.
Ira
On Fri, Feb 3, 2023 at 8:06 PM Ira Weiny <[email protected]> wrote:
>
> This series also fixes is_kmap_addr() and uses it to ensure no kmap addresses
> slip in later.
Ack. Please make it so.
That said...
I wasn't cc'd on all the patches, but checked them on the mailing
list, and that first is_kmap_addr() patch makes me a bit unhappy.
Right now that 'is_kmap_addr()' is only used for user copy addresses,
for debugging purposes, and I'm not exactly thrilled about extending
it this way.
I get the feeling that we should just have a name for that "kmap _or_
kmap_local" range instead of making it two ranges.
But admittedly I can't come up with anything better, and it looks like
different architectures may do different things. I just don't like
it.
Linus
On Sat, 4 Feb 2023 at 09:36, Ira Weiny <[email protected]> wrote:
>
> The kernel pages used by shm_get_kernel_pages() are allocated using
> GFP_KERNEL through the following call stack:
>
> trusted_instantiate()
> trusted_payload_alloc() -> GFP_KERNEL
> <trusted key op>
> tee_shm_register_kernel_buf()
> register_shm_helper()
> shm_get_kernel_pages()
>
> Where <trusted key op> is one of:
>
> trusted_key_unseal()
> trusted_key_get_random()
> trusted_key_seal()
>
> Remove the vmalloc page support from shm_get_kernel_pages(). Replace
> with a warn on once.
>
> Cc: Al Viro <[email protected]>
> Cc: "Fabio M. De Francesco" <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Reviewed-by: Jens Wiklander <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
> ---
> drivers/tee/tee_shm.c | 36 ++++++++++++------------------------
> 1 file changed, 12 insertions(+), 24 deletions(-)
>
Reviewed-by: Sumit Garg <[email protected]>
-Sumit
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 27295bda3e0b..527a6eabc03e 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -24,37 +24,25 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count)
> static int shm_get_kernel_pages(unsigned long start, size_t page_count,
> struct page **pages)
> {
> + struct kvec *kiov;
> size_t n;
> int rc;
>
> - if (is_vmalloc_addr((void *)start)) {
> - struct page *page;
> -
> - for (n = 0; n < page_count; n++) {
> - page = vmalloc_to_page((void *)(start + PAGE_SIZE * n));
> - if (!page)
> - return -ENOMEM;
> -
> - get_page(page);
> - pages[n] = page;
> - }
> - rc = page_count;
> - } else {
> - struct kvec *kiov;
> -
> - kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> - if (!kiov)
> - return -ENOMEM;
> + if (WARN_ON_ONCE(is_vmalloc_addr((void *)start)))
> + return -EINVAL;
>
> - for (n = 0; n < page_count; n++) {
> - kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> - kiov[n].iov_len = PAGE_SIZE;
> - }
> + kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> + if (!kiov)
> + return -ENOMEM;
>
> - rc = get_kernel_pages(kiov, page_count, 0, pages);
> - kfree(kiov);
> + for (n = 0; n < page_count; n++) {
> + kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> + kiov[n].iov_len = PAGE_SIZE;
> }
>
> + rc = get_kernel_pages(kiov, page_count, 0, pages);
> + kfree(kiov);
> +
> return rc;
> }
>
>
> --
> 2.39.1
On Sat, 4 Feb 2023 at 09:36, Ira Weiny <[email protected]> wrote:
>
> The kernel pages used by shm_get_kernel_pages() are allocated using
> GFP_KERNEL through the following call stack:
>
> trusted_instantiate()
> trusted_payload_alloc() -> GFP_KERNEL
> <trusted key op>
> tee_shm_register_kernel_buf()
> register_shm_helper()
> shm_get_kernel_pages()
>
> Where <trusted key op> is one of:
>
> trusted_key_unseal()
> trusted_key_get_random()
> trusted_key_seal()
>
> Because the pages can't be from highmem get_kernel_pages() boils down to
> a get_page() call.
>
> Remove the get_kernel_pages() call and open code the get_page().
>
> In case a highmem page does slip through warn on once for a kmap'ed
> address.
>
> Cc: Jens Wiklander <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: "Fabio M. De Francesco" <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
>
> ---
> Changes from v1:
> Al/Christoph: Remove kiov altogether
> ---
> drivers/tee/tee_shm.c | 21 ++++++++-------------
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
Reviewed-by: Sumit Garg <[email protected]>
-Sumit
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 527a6eabc03e..b1c6231defad 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -11,6 +11,7 @@
> #include <linux/tee_drv.h>
> #include <linux/uaccess.h>
> #include <linux/uio.h>
> +#include <linux/highmem.h>
> #include "tee_private.h"
>
> static void shm_put_kernel_pages(struct page **pages, size_t page_count)
> @@ -24,26 +25,20 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count)
> static int shm_get_kernel_pages(unsigned long start, size_t page_count,
> struct page **pages)
> {
> - struct kvec *kiov;
> + struct page *page;
> size_t n;
> - int rc;
>
> - if (WARN_ON_ONCE(is_vmalloc_addr((void *)start)))
> + if (WARN_ON_ONCE(is_vmalloc_addr((void *)start) ||
> + is_kmap_addr((void *)start)))
> return -EINVAL;
>
> - kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> - if (!kiov)
> - return -ENOMEM;
> -
> + page = virt_to_page(start);
> for (n = 0; n < page_count; n++) {
> - kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> - kiov[n].iov_len = PAGE_SIZE;
> + pages[n] = page + n;
> + get_page(pages[n]);
> }
>
> - rc = get_kernel_pages(kiov, page_count, 0, pages);
> - kfree(kiov);
> -
> - return rc;
> + return page_count;
> }
>
> static void release_registered_pages(struct tee_shm *shm)
>
> --
> 2.39.1
On Sat, 4 Feb 2023 at 09:36, Ira Weiny <[email protected]> wrote:
>
> The only caller to get_kernel_pages() [shm_get_kernel_pages()] has been
> updated to not need it.
>
> Remove get_kernel_pages().
>
> Cc: Mel Gorman <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: "Fabio M. De Francesco" <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Acked-by: John Hubbard <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
> ---
> include/linux/mm.h | 2 --
> mm/swap.c | 30 ------------------------------
> 2 files changed, 32 deletions(-)
>
Reviewed-by: Sumit Garg <[email protected]>
-Sumit
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8f857163ac89..2041e6d4fa27 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2095,8 +2095,6 @@ int __account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc,
> struct task_struct *task, bool bypass_rlim);
>
> struct kvec;
> -int get_kernel_pages(const struct kvec *iov, int nr_pages, int write,
> - struct page **pages);
> struct page *get_dump_page(unsigned long addr);
>
> bool folio_mark_dirty(struct folio *folio);
> diff --git a/mm/swap.c b/mm/swap.c
> index 70e2063ef43a..4c03ecab698e 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -158,36 +158,6 @@ void put_pages_list(struct list_head *pages)
> }
> EXPORT_SYMBOL(put_pages_list);
>
> -/*
> - * get_kernel_pages() - pin kernel pages in memory
> - * @kiov: An array of struct kvec structures
> - * @nr_segs: number of segments to pin
> - * @write: pinning for read/write, currently ignored
> - * @pages: array that receives pointers to the pages pinned.
> - * Should be at least nr_segs long.
> - *
> - * Returns number of pages pinned. This may be fewer than the number requested.
> - * If nr_segs is 0 or negative, returns 0. If no pages were pinned, returns 0.
> - * Each page returned must be released with a put_page() call when it is
> - * finished with.
> - */
> -int get_kernel_pages(const struct kvec *kiov, int nr_segs, int write,
> - struct page **pages)
> -{
> - int seg;
> -
> - for (seg = 0; seg < nr_segs; seg++) {
> - if (WARN_ON(kiov[seg].iov_len != PAGE_SIZE))
> - return seg;
> -
> - pages[seg] = kmap_to_page(kiov[seg].iov_base);
> - get_page(pages[seg]);
> - }
> -
> - return seg;
> -}
> -EXPORT_SYMBOL_GPL(get_kernel_pages);
> -
> typedef void (*move_fn_t)(struct lruvec *lruvec, struct folio *folio);
>
> static void lru_add_fn(struct lruvec *lruvec, struct folio *folio)
>
> --
> 2.39.1
On Sat, 4 Feb 2023 at 09:36, Ira Weiny <[email protected]> wrote:
>
> Sumit,
>
> I did not see a follow up on this series per your last email.[1] I'd like to
> move forward with getting rid of kmap_to_page(). So Hopefully this can land
> and you can build on this rather than the other way around?
Apologies Ira for keeping you waiting. Actually I was fully involved
with other high priority work with my upstream review backlog
increasing. So I wasn't able to devote time to this work. Sure I will
rebase my work on top of your changes.
-Sumit
>
> All,
>
> Al Viro found[2] that kmap_to_page() is broken. But not only is it broken, it
> presents confusion over how highmem should be used because kmap() and friends
> should not be used for 'long term' mappings.
>
> get_kernel_pages() is a caller of kmap_to_page(). It only has one caller
> [shm_get_kernel_pages()] which does not need the functionality.
>
> Alter shm_get_kernel_pages() to no longer call get_kernel_pages() and remove
> get_kernel_pages(). Along the way it was noted that shm_get_kernel_pages()
> does not have any need to support vmalloc'ed addresses either. Remove that
> functionality to clean up the logic.
>
> This series also fixes is_kmap_addr() and uses it to ensure no kmap addresses
> slip in later.
>
> [1] https://lore.kernel.org/all/CAFA6WYMqEVDVW-ifoh-V9ni1zntYdes8adQKf2XXAUpqdaW53w@mail.gmail.com/
> [2] https://lore.kernel.org/lkml/YzSSl1ItVlARDvG3@ZenIV
>
> To: Sumit Garg <[email protected]>
> To: Andrew Morton <[email protected]>
> Cc: "Al Viro" <[email protected]>
> Cc: "Christoph Hellwig" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Jens Wiklander <[email protected]>
> Cc: "Fabio M. De Francesco" <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
>
> ---
> Changes in v2:
> - Al Viro: Avoid allocating the kiov.
> - Sumit: Update cover letter to clarify the motivation behind removing
> get_kernel_pages()
> - Link to v1: https://lore.kernel.org/r/[email protected]
>
> ---
> Ira Weiny (4):
> highmem: Enhance is_kmap_addr() to check kmap_local_page() mappings
> tee: Remove vmalloc page support
> tee: Remove call to get_kernel_pages()
> mm: Remove get_kernel_pages()
>
> drivers/tee/tee_shm.c | 37 ++++++++++---------------------------
> include/linux/highmem-internal.h | 5 ++++-
> include/linux/mm.h | 2 --
> mm/swap.c | 30 ------------------------------
> 4 files changed, 14 insertions(+), 60 deletions(-)
> ---
> base-commit: 0136d86b78522bbd5755f8194c97a987f0586ba5
> change-id: 20230203-get_kernel_pages-199342cfba79
>
> Best regards,
> --
> Ira Weiny <[email protected]>
Sumit Garg wrote:
> On Sat, 4 Feb 2023 at 09:36, Ira Weiny <[email protected]> wrote:
> >
> > Sumit,
> >
> > I did not see a follow up on this series per your last email.[1] I'd like to
> > move forward with getting rid of kmap_to_page(). So Hopefully this can land
> > and you can build on this rather than the other way around?
>
> Apologies Ira for keeping you waiting. Actually I was fully involved
> with other high priority work with my upstream review backlog
> increasing. So I wasn't able to devote time to this work. Sure I will
> rebase my work on top of your changes.
No problem on my end. I just wanted to ensure that I did not miss
something.
Thanks for the reviews!
Ira
>
> -Sumit
>
> >
> > All,
> >
> > Al Viro found[2] that kmap_to_page() is broken. But not only is it broken, it
> > presents confusion over how highmem should be used because kmap() and friends
> > should not be used for 'long term' mappings.
> >
> > get_kernel_pages() is a caller of kmap_to_page(). It only has one caller
> > [shm_get_kernel_pages()] which does not need the functionality.
> >
> > Alter shm_get_kernel_pages() to no longer call get_kernel_pages() and remove
> > get_kernel_pages(). Along the way it was noted that shm_get_kernel_pages()
> > does not have any need to support vmalloc'ed addresses either. Remove that
> > functionality to clean up the logic.
> >
> > This series also fixes is_kmap_addr() and uses it to ensure no kmap addresses
> > slip in later.
> >
> > [1] https://lore.kernel.org/all/CAFA6WYMqEVDVW-ifoh-V9ni1zntYdes8adQKf2XXAUpqdaW53w@mail.gmail.com/
> > [2] https://lore.kernel.org/lkml/YzSSl1ItVlARDvG3@ZenIV
> >
> > To: Sumit Garg <[email protected]>
> > To: Andrew Morton <[email protected]>
> > Cc: "Al Viro" <[email protected]>
> > Cc: "Christoph Hellwig" <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: Jens Wiklander <[email protected]>
> > Cc: "Fabio M. De Francesco" <[email protected]>
> > Signed-off-by: Ira Weiny <[email protected]>
> >
> > ---
> > Changes in v2:
> > - Al Viro: Avoid allocating the kiov.
> > - Sumit: Update cover letter to clarify the motivation behind removing
> > get_kernel_pages()
> > - Link to v1: https://lore.kernel.org/r/[email protected]
> >
> > ---
> > Ira Weiny (4):
> > highmem: Enhance is_kmap_addr() to check kmap_local_page() mappings
> > tee: Remove vmalloc page support
> > tee: Remove call to get_kernel_pages()
> > mm: Remove get_kernel_pages()
> >
> > drivers/tee/tee_shm.c | 37 ++++++++++---------------------------
> > include/linux/highmem-internal.h | 5 ++++-
> > include/linux/mm.h | 2 --
> > mm/swap.c | 30 ------------------------------
> > 4 files changed, 14 insertions(+), 60 deletions(-)
> > ---
> > base-commit: 0136d86b78522bbd5755f8194c97a987f0586ba5
> > change-id: 20230203-get_kernel_pages-199342cfba79
> >
> > Best regards,
> > --
> > Ira Weiny <[email protected]>
Ira Weiny wrote:
> Sumit Garg wrote:
> > On Sat, 4 Feb 2023 at 09:36, Ira Weiny <[email protected]> wrote:
> > >
> > > Sumit,
> > >
> > > I did not see a follow up on this series per your last email.[1] I'd like to
> > > move forward with getting rid of kmap_to_page(). So Hopefully this can land
> > > and you can build on this rather than the other way around?
> >
> > Apologies Ira for keeping you waiting. Actually I was fully involved
> > with other high priority work with my upstream review backlog
> > increasing. So I wasn't able to devote time to this work. Sure I will
> > rebase my work on top of your changes.
>
> No problem on my end. I just wanted to ensure that I did not miss
> something.
Andrew, can I get an ack on patches 1 and 4 for this series? I realized
that perhaps I was not clear on my expectations of this series. I was
thinking this would be easiest to go through the tee subsystem tree.
Sumit or Jens, is that ok with you all?
Thanks,
Ira
On Fri, 03 Feb 2023 20:06:32 -0800 Ira Weiny <[email protected]> wrote:
> is_kmap_addr() is only looking at the kmap() address range which may
> cause check_heap_object() to miss checking an overflow on a
> kmap_local_page() page.
>
> Add a check for the kmap_local_page() address range to is_kmap_addr().
Acked-by: Andrew Morton <[email protected]>
On Fri, 03 Feb 2023 20:06:35 -0800 Ira Weiny <[email protected]> wrote:
> The only caller to get_kernel_pages() [shm_get_kernel_pages()] has been
> updated to not need it.
>
> Remove get_kernel_pages().
Acked-by: Andrew Morton <[email protected]>
Hi Ira,
On Fri, Feb 10, 2023 at 9:28 PM Ira Weiny <[email protected]> wrote:
>
> Ira Weiny wrote:
> > Sumit Garg wrote:
> > > On Sat, 4 Feb 2023 at 09:36, Ira Weiny <[email protected]> wrote:
> > > >
> > > > Sumit,
> > > >
> > > > I did not see a follow up on this series per your last email.[1] I'd like to
> > > > move forward with getting rid of kmap_to_page(). So Hopefully this can land
> > > > and you can build on this rather than the other way around?
> > >
> > > Apologies Ira for keeping you waiting. Actually I was fully involved
> > > with other high priority work with my upstream review backlog
> > > increasing. So I wasn't able to devote time to this work. Sure I will
> > > rebase my work on top of your changes.
> >
> > No problem on my end. I just wanted to ensure that I did not miss
> > something.
>
> Andrew, can I get an ack on patches 1 and 4 for this series? I realized
> that perhaps I was not clear on my expectations of this series. I was
> thinking this would be easiest to go through the tee subsystem tree.
>
> Sumit or Jens, is that ok with you all?
Sure, I'll take it. The timing is a bit unfortunate, it's likely too
close to the merge window to be included there. However, I'll pick it
up and add it to linux-next so it's ready for the 6.4 merge window.
Thanks,
Jens
On Fri, Feb 03, 2023 at 08:06:32PM -0800, Ira Weiny wrote:
> is_kmap_addr() is only looking at the kmap() address range which may
> cause check_heap_object() to miss checking an overflow on a
> kmap_local_page() page.
>
> Add a check for the kmap_local_page() address range to is_kmap_addr().
>
> Cc: Matthew Wilcox <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: "Fabio M. De Francesco" <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
> ---
> include/linux/highmem-internal.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
Added to https://git.linaro.org/people/jens.wiklander/linux-tee.git/log/?h=get_kernel_pages-for-v6.4
Thanks,
Jens
> diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
> index e098f38422af..a3028e400a9c 100644
> --- a/include/linux/highmem-internal.h
> +++ b/include/linux/highmem-internal.h
> @@ -152,7 +152,10 @@ static inline void totalhigh_pages_add(long count)
> static inline bool is_kmap_addr(const void *x)
> {
> unsigned long addr = (unsigned long)x;
> - return addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP);
> +
> + return (addr >= PKMAP_ADDR(0) && addr < PKMAP_ADDR(LAST_PKMAP)) ||
> + (addr >= __fix_to_virt(FIX_KMAP_END) &&
> + addr < __fix_to_virt(FIX_KMAP_BEGIN));
> }
> #else /* CONFIG_HIGHMEM */
>
>
> --
> 2.39.1
On Fri, Feb 03, 2023 at 08:06:33PM -0800, Ira Weiny wrote:
> The kernel pages used by shm_get_kernel_pages() are allocated using
> GFP_KERNEL through the following call stack:
>
> trusted_instantiate()
> trusted_payload_alloc() -> GFP_KERNEL
> <trusted key op>
> tee_shm_register_kernel_buf()
> register_shm_helper()
> shm_get_kernel_pages()
>
> Where <trusted key op> is one of:
>
> trusted_key_unseal()
> trusted_key_get_random()
> trusted_key_seal()
>
> Remove the vmalloc page support from shm_get_kernel_pages(). Replace
> with a warn on once.
>
> Cc: Al Viro <[email protected]>
> Cc: "Fabio M. De Francesco" <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Reviewed-by: Jens Wiklander <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
> ---
> drivers/tee/tee_shm.c | 36 ++++++++++++------------------------
> 1 file changed, 12 insertions(+), 24 deletions(-)
Added to https://git.linaro.org/people/jens.wiklander/linux-tee.git/log/?h=get_kernel_pages-for-v6.4
Thanks,
Jens
>
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 27295bda3e0b..527a6eabc03e 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -24,37 +24,25 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count)
> static int shm_get_kernel_pages(unsigned long start, size_t page_count,
> struct page **pages)
> {
> + struct kvec *kiov;
> size_t n;
> int rc;
>
> - if (is_vmalloc_addr((void *)start)) {
> - struct page *page;
> -
> - for (n = 0; n < page_count; n++) {
> - page = vmalloc_to_page((void *)(start + PAGE_SIZE * n));
> - if (!page)
> - return -ENOMEM;
> -
> - get_page(page);
> - pages[n] = page;
> - }
> - rc = page_count;
> - } else {
> - struct kvec *kiov;
> -
> - kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> - if (!kiov)
> - return -ENOMEM;
> + if (WARN_ON_ONCE(is_vmalloc_addr((void *)start)))
> + return -EINVAL;
>
> - for (n = 0; n < page_count; n++) {
> - kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> - kiov[n].iov_len = PAGE_SIZE;
> - }
> + kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> + if (!kiov)
> + return -ENOMEM;
>
> - rc = get_kernel_pages(kiov, page_count, 0, pages);
> - kfree(kiov);
> + for (n = 0; n < page_count; n++) {
> + kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> + kiov[n].iov_len = PAGE_SIZE;
> }
>
> + rc = get_kernel_pages(kiov, page_count, 0, pages);
> + kfree(kiov);
> +
> return rc;
> }
>
>
> --
> 2.39.1
On Fri, Feb 03, 2023 at 08:06:34PM -0800, Ira Weiny wrote:
> The kernel pages used by shm_get_kernel_pages() are allocated using
> GFP_KERNEL through the following call stack:
>
> trusted_instantiate()
> trusted_payload_alloc() -> GFP_KERNEL
> <trusted key op>
> tee_shm_register_kernel_buf()
> register_shm_helper()
> shm_get_kernel_pages()
>
> Where <trusted key op> is one of:
>
> trusted_key_unseal()
> trusted_key_get_random()
> trusted_key_seal()
>
> Because the pages can't be from highmem get_kernel_pages() boils down to
> a get_page() call.
>
> Remove the get_kernel_pages() call and open code the get_page().
>
> In case a highmem page does slip through warn on once for a kmap'ed
> address.
>
> Cc: Jens Wiklander <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: "Fabio M. De Francesco" <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Linus Torvalds <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
>
> ---
> Changes from v1:
> Al/Christoph: Remove kiov altogether
> ---
> drivers/tee/tee_shm.c | 21 ++++++++-------------
> 1 file changed, 8 insertions(+), 13 deletions(-)
Added to https://git.linaro.org/people/jens.wiklander/linux-tee.git/log/?h=get_kernel_pages-for-v6.4
Thanks,
Jens
>
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 527a6eabc03e..b1c6231defad 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -11,6 +11,7 @@
> #include <linux/tee_drv.h>
> #include <linux/uaccess.h>
> #include <linux/uio.h>
> +#include <linux/highmem.h>
> #include "tee_private.h"
>
> static void shm_put_kernel_pages(struct page **pages, size_t page_count)
> @@ -24,26 +25,20 @@ static void shm_put_kernel_pages(struct page **pages, size_t page_count)
> static int shm_get_kernel_pages(unsigned long start, size_t page_count,
> struct page **pages)
> {
> - struct kvec *kiov;
> + struct page *page;
> size_t n;
> - int rc;
>
> - if (WARN_ON_ONCE(is_vmalloc_addr((void *)start)))
> + if (WARN_ON_ONCE(is_vmalloc_addr((void *)start) ||
> + is_kmap_addr((void *)start)))
> return -EINVAL;
>
> - kiov = kcalloc(page_count, sizeof(*kiov), GFP_KERNEL);
> - if (!kiov)
> - return -ENOMEM;
> -
> + page = virt_to_page(start);
> for (n = 0; n < page_count; n++) {
> - kiov[n].iov_base = (void *)(start + n * PAGE_SIZE);
> - kiov[n].iov_len = PAGE_SIZE;
> + pages[n] = page + n;
> + get_page(pages[n]);
> }
>
> - rc = get_kernel_pages(kiov, page_count, 0, pages);
> - kfree(kiov);
> -
> - return rc;
> + return page_count;
> }
>
> static void release_registered_pages(struct tee_shm *shm)
>
> --
> 2.39.1
On Fri, Feb 03, 2023 at 08:06:35PM -0800, Ira Weiny wrote:
> The only caller to get_kernel_pages() [shm_get_kernel_pages()] has been
> updated to not need it.
>
> Remove get_kernel_pages().
>
> Cc: Mel Gorman <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: "Fabio M. De Francesco" <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Acked-by: John Hubbard <[email protected]>
> Signed-off-by: Ira Weiny <[email protected]>
> ---
> include/linux/mm.h | 2 --
> mm/swap.c | 30 ------------------------------
> 2 files changed, 32 deletions(-)
Added to https://git.linaro.org/people/jens.wiklander/linux-tee.git/log/?h=get_kernel_pages-for-v6.4
Thanks,
Jens
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8f857163ac89..2041e6d4fa27 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2095,8 +2095,6 @@ int __account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc,
> struct task_struct *task, bool bypass_rlim);
>
> struct kvec;
> -int get_kernel_pages(const struct kvec *iov, int nr_pages, int write,
> - struct page **pages);
> struct page *get_dump_page(unsigned long addr);
>
> bool folio_mark_dirty(struct folio *folio);
> diff --git a/mm/swap.c b/mm/swap.c
> index 70e2063ef43a..4c03ecab698e 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -158,36 +158,6 @@ void put_pages_list(struct list_head *pages)
> }
> EXPORT_SYMBOL(put_pages_list);
>
> -/*
> - * get_kernel_pages() - pin kernel pages in memory
> - * @kiov: An array of struct kvec structures
> - * @nr_segs: number of segments to pin
> - * @write: pinning for read/write, currently ignored
> - * @pages: array that receives pointers to the pages pinned.
> - * Should be at least nr_segs long.
> - *
> - * Returns number of pages pinned. This may be fewer than the number requested.
> - * If nr_segs is 0 or negative, returns 0. If no pages were pinned, returns 0.
> - * Each page returned must be released with a put_page() call when it is
> - * finished with.
> - */
> -int get_kernel_pages(const struct kvec *kiov, int nr_segs, int write,
> - struct page **pages)
> -{
> - int seg;
> -
> - for (seg = 0; seg < nr_segs; seg++) {
> - if (WARN_ON(kiov[seg].iov_len != PAGE_SIZE))
> - return seg;
> -
> - pages[seg] = kmap_to_page(kiov[seg].iov_base);
> - get_page(pages[seg]);
> - }
> -
> - return seg;
> -}
> -EXPORT_SYMBOL_GPL(get_kernel_pages);
> -
> typedef void (*move_fn_t)(struct lruvec *lruvec, struct folio *folio);
>
> static void lru_add_fn(struct lruvec *lruvec, struct folio *folio)
>
> --
> 2.39.1
Jens Wiklander wrote:
> Hi Ira,
>
> On Fri, Feb 10, 2023 at 9:28 PM Ira Weiny <[email protected]> wrote:
> >
> > Ira Weiny wrote:
> > > Sumit Garg wrote:
> > > > On Sat, 4 Feb 2023 at 09:36, Ira Weiny <[email protected]> wrote:
> > > > >
> > > > > Sumit,
> > > > >
> > > > > I did not see a follow up on this series per your last email.[1] I'd like to
> > > > > move forward with getting rid of kmap_to_page(). So Hopefully this can land
> > > > > and you can build on this rather than the other way around?
> > > >
> > > > Apologies Ira for keeping you waiting. Actually I was fully involved
> > > > with other high priority work with my upstream review backlog
> > > > increasing. So I wasn't able to devote time to this work. Sure I will
> > > > rebase my work on top of your changes.
> > >
> > > No problem on my end. I just wanted to ensure that I did not miss
> > > something.
> >
> > Andrew, can I get an ack on patches 1 and 4 for this series? I realized
> > that perhaps I was not clear on my expectations of this series. I was
> > thinking this would be easiest to go through the tee subsystem tree.
> >
> > Sumit or Jens, is that ok with you all?
>
> Sure, I'll take it. The timing is a bit unfortunate, it's likely too
> close to the merge window to be included there. However, I'll pick it
> up and add it to linux-next so it's ready for the 6.4 merge window.
6.4 is fine with me.
Thanks everyone!
Ira
On Mon, Feb 13, 2023 at 7:02 AM Jens Wiklander
<[email protected]> wrote:
>
> Sure, I'll take it. The timing is a bit unfortunate, it's likely too
> close to the merge window to be included there. However, I'll pick it
> up and add it to linux-next so it's ready for the 6.4 merge window.
With this boeing almost all code removal, I'm perfectly fine taking it
in the upcoming merge window.
Linus
On Mon, Feb 13, 2023 at 11:02:52AM -0800, Linus Torvalds wrote:
> On Mon, Feb 13, 2023 at 7:02 AM Jens Wiklander
> <[email protected]> wrote:
> >
> > Sure, I'll take it. The timing is a bit unfortunate, it's likely too
> > close to the merge window to be included there. However, I'll pick it
> > up and add it to linux-next so it's ready for the 6.4 merge window.
>
> With this boeing almost all code removal, I'm perfectly fine taking it
> in the upcoming merge window.
OK, thank you.
Cheers,
Jens