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-V9ni1zntYdes8adQKf2XXAUpqdaW... [2] https://lore.kernel.org/lkml/YzSSl1ItVlARDvG3@ZenIV
To: Sumit Garg sumit.garg@linaro.org To: Andrew Morton akpm@linux-foundation.org Cc: "Al Viro" viro@zeniv.linux.org.uk Cc: "Christoph Hellwig" hch@lst.de Cc: linux-kernel@vger.kernel.org Cc: op-tee@lists.trustedfirmware.org Cc: linux-mm@kvack.org Cc: Jens Wiklander jens.wiklander@linaro.org Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Signed-off-by: Ira Weiny ira.weiny@intel.com
--- 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/20221002002326.946620-1-ira.weiny@intel.com
--- 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,
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 willy@infradead.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Christoph Hellwig hch@lst.de Cc: Andrew Morton akpm@linux-foundation.org Signed-off-by: Ira Weiny ira.weiny@intel.com --- 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 */
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?
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, 03 Feb 2023 20:06:32 -0800 Ira Weiny ira.weiny@intel.com 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 akpm@linux-foudation.org
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 willy@infradead.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Christoph Hellwig hch@lst.de Cc: Andrew Morton akpm@linux-foundation.org Signed-off-by: Ira Weiny ira.weiny@intel.com
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...
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
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 viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Linus Torvalds torvalds@linux-foundation.org Reviewed-by: Jens Wiklander jens.wiklander@linaro.org Signed-off-by: Ira Weiny ira.weiny@intel.com --- 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; }
Looks good:
Reviewed-by: Christoph Hellwig hch@lst.de
On Sat, 4 Feb 2023 at 09:36, Ira Weiny ira.weiny@intel.com 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 viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Linus Torvalds torvalds@linux-foundation.org Reviewed-by: Jens Wiklander jens.wiklander@linaro.org Signed-off-by: Ira Weiny ira.weiny@intel.com
drivers/tee/tee_shm.c | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-)
Reviewed-by: Sumit Garg sumit.garg@linaro.org
-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 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 viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Linus Torvalds torvalds@linux-foundation.org Reviewed-by: Jens Wiklander jens.wiklander@linaro.org Signed-off-by: Ira Weiny ira.weiny@intel.com
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...
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
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 jens.wiklander@linaro.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Ira Weiny ira.weiny@intel.com
--- 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)
Looks good:
Reviewed-by: Christoph Hellwig hch@lst.de
On Sat, 4 Feb 2023 at 09:36, Ira Weiny ira.weiny@intel.com 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 jens.wiklander@linaro.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Ira Weiny ira.weiny@intel.com
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 sumit.garg@linaro.org
-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 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 jens.wiklander@linaro.org Cc: Al Viro viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Linus Torvalds torvalds@linux-foundation.org Signed-off-by: Ira Weiny ira.weiny@intel.com
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...
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)))
- 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 mgorman@suse.de Cc: Al Viro viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Andrew Morton akpm@linux-foundation.org Acked-by: John Hubbard jhubbard@nvidia.com Signed-off-by: Ira Weiny ira.weiny@intel.com --- 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)
Looks good:
Reviewed-by: Christoph Hellwig hch@lst.de
On Sat, 4 Feb 2023 at 09:36, Ira Weiny ira.weiny@intel.com 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 mgorman@suse.de Cc: Al Viro viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Andrew Morton akpm@linux-foundation.org Acked-by: John Hubbard jhubbard@nvidia.com Signed-off-by: Ira Weiny ira.weiny@intel.com
include/linux/mm.h | 2 -- mm/swap.c | 30 ------------------------------ 2 files changed, 32 deletions(-)
Reviewed-by: Sumit Garg sumit.garg@linaro.org
-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 Fri, 03 Feb 2023 20:06:35 -0800 Ira Weiny ira.weiny@intel.com 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 akpm@linux-foudation.org
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 mgorman@suse.de Cc: Al Viro viro@zeniv.linux.org.uk Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Cc: Christoph Hellwig hch@lst.de Cc: Andrew Morton akpm@linux-foundation.org Acked-by: John Hubbard jhubbard@nvidia.com Signed-off-by: Ira Weiny ira.weiny@intel.com
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...
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
On Fri, Feb 3, 2023 at 8:06 PM Ira Weiny ira.weiny@intel.com 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 ira.weiny@intel.com 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-V9ni1zntYdes8adQKf2XXAUpqdaW... [2] https://lore.kernel.org/lkml/YzSSl1ItVlARDvG3@ZenIV
To: Sumit Garg sumit.garg@linaro.org To: Andrew Morton akpm@linux-foundation.org Cc: "Al Viro" viro@zeniv.linux.org.uk Cc: "Christoph Hellwig" hch@lst.de Cc: linux-kernel@vger.kernel.org Cc: op-tee@lists.trustedfirmware.org Cc: linux-mm@kvack.org Cc: Jens Wiklander jens.wiklander@linaro.org Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Signed-off-by: Ira Weiny ira.weiny@intel.com
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/20221002002326.946620-1-ira.weiny@intel.com
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 ira.weiny@intel.com
Sumit Garg wrote:
On Sat, 4 Feb 2023 at 09:36, Ira Weiny ira.weiny@intel.com 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-V9ni1zntYdes8adQKf2XXAUpqdaW... [2] https://lore.kernel.org/lkml/YzSSl1ItVlARDvG3@ZenIV
To: Sumit Garg sumit.garg@linaro.org To: Andrew Morton akpm@linux-foundation.org Cc: "Al Viro" viro@zeniv.linux.org.uk Cc: "Christoph Hellwig" hch@lst.de Cc: linux-kernel@vger.kernel.org Cc: op-tee@lists.trustedfirmware.org Cc: linux-mm@kvack.org Cc: Jens Wiklander jens.wiklander@linaro.org Cc: "Fabio M. De Francesco" fmdefrancesco@gmail.com Signed-off-by: Ira Weiny ira.weiny@intel.com
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/20221002002326.946620-1-ira.weiny@intel.com
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 ira.weiny@intel.com
Ira Weiny wrote:
Sumit Garg wrote:
On Sat, 4 Feb 2023 at 09:36, Ira Weiny ira.weiny@intel.com 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
Hi Ira,
On Fri, Feb 10, 2023 at 9:28 PM Ira Weiny ira.weiny@intel.com wrote:
Ira Weiny wrote:
Sumit Garg wrote:
On Sat, 4 Feb 2023 at 09:36, Ira Weiny ira.weiny@intel.com 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
Jens Wiklander wrote:
Hi Ira,
On Fri, Feb 10, 2023 at 9:28 PM Ira Weiny ira.weiny@intel.com wrote:
Ira Weiny wrote:
Sumit Garg wrote:
On Sat, 4 Feb 2023 at 09:36, Ira Weiny ira.weiny@intel.com 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 jens.wiklander@linaro.org 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 jens.wiklander@linaro.org 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
op-tee@lists.trustedfirmware.org
-
Andrew Morton -
Christoph Hellwig -
Ira Weiny -
Jens Wiklander -
Linus Torvalds -
Sumit Garg