The use of kmap() and kmap_atomic() are being deprecated in favor of
kmap_local_page().
With kmap_local_page(), the mappings are per thread, CPU local and not
globally visible. Furthermore, the mappings can be acquired from any
context (including interrupts).
Therefore, use kmap_local_page() in exec.c because these mappings are per
thread, CPU local, and not globally visible.
Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with
HIGHMEM64GB enabled.
Suggested-by: Ira Weiny <[email protected]>
Signed-off-by: Fabio M. De Francesco <[email protected]>
---
fs/exec.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 0989fb8472a1..4a2129c0d422 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -583,11 +583,11 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
if (kmapped_page) {
flush_dcache_page(kmapped_page);
- kunmap(kmapped_page);
+ kunmap_local(kaddr);
put_arg_page(kmapped_page);
}
kmapped_page = page;
- kaddr = kmap(kmapped_page);
+ kaddr = kmap_local_page(kmapped_page);
kpos = pos & PAGE_MASK;
flush_arg_page(bprm, kpos, kmapped_page);
}
@@ -601,7 +601,7 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
out:
if (kmapped_page) {
flush_dcache_page(kmapped_page);
- kunmap(kmapped_page);
+ kunmap_local(kaddr);
put_arg_page(kmapped_page);
}
return ret;
@@ -883,11 +883,11 @@ int transfer_args_to_stack(struct linux_binprm *bprm,
for (index = MAX_ARG_PAGES - 1; index >= stop; index--) {
unsigned int offset = index == stop ? bprm->p & ~PAGE_MASK : 0;
- char *src = kmap(bprm->page[index]) + offset;
+ char *src = kmap_local_page(bprm->page[index]) + offset;
sp -= PAGE_SIZE - offset;
if (copy_to_user((void *) sp, src, PAGE_SIZE - offset) != 0)
ret = -EFAULT;
- kunmap(bprm->page[index]);
+ kunmap_local(src);
if (ret)
goto out;
}
@@ -1680,13 +1680,13 @@ int remove_arg_zero(struct linux_binprm *bprm)
ret = -EFAULT;
goto out;
}
- kaddr = kmap_atomic(page);
+ kaddr = kmap_local_page(page);
for (; offset < PAGE_SIZE && kaddr[offset];
offset++, bprm->p++)
;
- kunmap_atomic(kaddr);
+ kunmap_local(kaddr);
put_arg_page(page);
} while (offset == PAGE_SIZE);
--
2.36.1
"Fabio M. De Francesco" <[email protected]> writes:
> The use of kmap() and kmap_atomic() are being deprecated in favor of
> kmap_local_page().
>
> With kmap_local_page(), the mappings are per thread, CPU local and not
> globally visible. Furthermore, the mappings can be acquired from any
> context (including interrupts).
>
> Therefore, use kmap_local_page() in exec.c because these mappings are per
> thread, CPU local, and not globally visible.
>
> Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with
> HIGHMEM64GB enabled.
Can someone please refresh my memory on what is going on.
I remember there were limitations that kmap_atomic had that are hard to
meet so something I think it was kmap_local was invented and created
to be the kmap_atomic replacement.
What are the requirements on kmap_local? In copy_strings
kmap is called in contexts that can sleep in page faults so any
nearly any requirement except a thread local use is invalidated.
As you have described kmap_local above it does not sound like kmap_local
is safe in this context, but that could just be a problem in description
that my poor memory does is not recalling the necessary details to
correct.
Eric
> Suggested-by: Ira Weiny <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> fs/exec.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 0989fb8472a1..4a2129c0d422 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -583,11 +583,11 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
>
> if (kmapped_page) {
> flush_dcache_page(kmapped_page);
> - kunmap(kmapped_page);
> + kunmap_local(kaddr);
> put_arg_page(kmapped_page);
> }
> kmapped_page = page;
> - kaddr = kmap(kmapped_page);
> + kaddr = kmap_local_page(kmapped_page);
> kpos = pos & PAGE_MASK;
> flush_arg_page(bprm, kpos, kmapped_page);
> }
> @@ -601,7 +601,7 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
> out:
> if (kmapped_page) {
> flush_dcache_page(kmapped_page);
> - kunmap(kmapped_page);
> + kunmap_local(kaddr);
> put_arg_page(kmapped_page);
> }
> return ret;
> @@ -883,11 +883,11 @@ int transfer_args_to_stack(struct linux_binprm *bprm,
>
> for (index = MAX_ARG_PAGES - 1; index >= stop; index--) {
> unsigned int offset = index == stop ? bprm->p & ~PAGE_MASK : 0;
> - char *src = kmap(bprm->page[index]) + offset;
> + char *src = kmap_local_page(bprm->page[index]) + offset;
> sp -= PAGE_SIZE - offset;
> if (copy_to_user((void *) sp, src, PAGE_SIZE - offset) != 0)
> ret = -EFAULT;
> - kunmap(bprm->page[index]);
> + kunmap_local(src);
> if (ret)
> goto out;
> }
> @@ -1680,13 +1680,13 @@ int remove_arg_zero(struct linux_binprm *bprm)
> ret = -EFAULT;
> goto out;
> }
> - kaddr = kmap_atomic(page);
> + kaddr = kmap_local_page(page);
>
> for (; offset < PAGE_SIZE && kaddr[offset];
> offset++, bprm->p++)
> ;
>
> - kunmap_atomic(kaddr);
> + kunmap_local(kaddr);
> put_arg_page(page);
> } while (offset == PAGE_SIZE);
On giovedì 30 giugno 2022 19:38:08 CEST Eric W. Biederman wrote:
> "Fabio M. De Francesco" <[email protected]> writes:
>
> > The use of kmap() and kmap_atomic() are being deprecated in favor of
> > kmap_local_page().
> >
> > With kmap_local_page(), the mappings are per thread, CPU local and not
> > globally visible. Furthermore, the mappings can be acquired from any
> > context (including interrupts).
> >
> > Therefore, use kmap_local_page() in exec.c because these mappings are
per
> > thread, CPU local, and not globally visible.
> >
> > Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with
> > HIGHMEM64GB enabled.
>
> Can someone please refresh my memory on what is going on.
>
> I remember there were limitations that kmap_atomic had that are hard to
> meet so something I think it was kmap_local was invented and created
> to be the kmap_atomic replacement.
Please read highmem.rst. I've updated that document weeks ago:
https://docs.kernel.org/vm/highmem.html?highlight=highmem
Currently it contains many more information I can ever place here in order
to answer your questions.
Believe me, this is not by any means a way to elude your questions. I'm
pretty sure that by reading that document you'll have a clear vision on
what is going on :-)
>
> What are the requirements on kmap_local? In copy_strings
> kmap is called in contexts that can sleep in page faults
No problems with kmap_local_page() with regard to page faults (again,
please read the above-mentioned document).
From that document...
"It’s valid to take pagefaults in a local kmap region []".
"Each call of kmap_atomic() in the kernel creates a non-preemptible section
and disable pagefaults. This could be a source of unwanted latency.
Therefore users should prefer kmap_local_page() instead of kmap_atomic().".
> so any
> nearly any requirement except a thread local use is invalidated.
>
> As you have described kmap_local above it does not sound like kmap_local
> is safe in this context,
Sorry, probably I should add that taking page faults is allowed. Would you
prefer I send a v2 and add this information?
Thanks,
Fabio
> but that could just be a problem in description
> that my poor memory does is not recalling the necessary details to
> correct.
>
> Eric
>
> > Suggested-by: Ira Weiny <[email protected]>
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > ---
> > fs/exec.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 0989fb8472a1..4a2129c0d422 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -583,11 +583,11 @@ static int copy_strings(int argc, struct
user_arg_ptr argv,
> >
> > if (kmapped_page) {
> >
flush_dcache_page(kmapped_page);
> > - kunmap(kmapped_page);
> > + kunmap_local(kaddr);
> >
put_arg_page(kmapped_page);
> > }
> > kmapped_page = page;
> > - kaddr = kmap(kmapped_page);
> > + kaddr =
kmap_local_page(kmapped_page);
> > kpos = pos & PAGE_MASK;
> > flush_arg_page(bprm, kpos,
kmapped_page);
> > }
> > @@ -601,7 +601,7 @@ static int copy_strings(int argc, struct
user_arg_ptr argv,
> > out:
> > if (kmapped_page) {
> > flush_dcache_page(kmapped_page);
> > - kunmap(kmapped_page);
> > + kunmap_local(kaddr);
> > put_arg_page(kmapped_page);
> > }
> > return ret;
> > @@ -883,11 +883,11 @@ int transfer_args_to_stack(struct linux_binprm
*bprm,
> >
> > for (index = MAX_ARG_PAGES - 1; index >= stop; index--) {
> > unsigned int offset = index == stop ? bprm->p &
~PAGE_MASK : 0;
> > - char *src = kmap(bprm->page[index]) + offset;
> > + char *src = kmap_local_page(bprm->page[index]) +
offset;
> > sp -= PAGE_SIZE - offset;
> > if (copy_to_user((void *) sp, src, PAGE_SIZE - offset)
!= 0)
> > ret = -EFAULT;
> > - kunmap(bprm->page[index]);
> > + kunmap_local(src);
> > if (ret)
> > goto out;
> > }
> > @@ -1680,13 +1680,13 @@ int remove_arg_zero(struct linux_binprm *bprm)
> > ret = -EFAULT;
> > goto out;
> > }
> > - kaddr = kmap_atomic(page);
> > + kaddr = kmap_local_page(page);
> >
> > for (; offset < PAGE_SIZE && kaddr[offset];
> > offset++, bprm->p++)
> > ;
> >
> > - kunmap_atomic(kaddr);
> > + kunmap_local(kaddr);
> > put_arg_page(page);
> > } while (offset == PAGE_SIZE);
>
On Thu, Jun 30, 2022 at 06:35:27PM +0200, Fabio M. De Francesco wrote:
> The use of kmap() and kmap_atomic() are being deprecated in favor of
> kmap_local_page().
>
> With kmap_local_page(), the mappings are per thread, CPU local and not
> globally visible. Furthermore, the mappings can be acquired from any
> context (including interrupts).
>
> Therefore, use kmap_local_page() in exec.c because these mappings are per
> thread, CPU local, and not globally visible.
>
> Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with
> HIGHMEM64GB enabled.
>
> Suggested-by: Ira Weiny <[email protected]>
This looks good but there is a kmap_atomic() in this file which I _think_ can
be converted as well. But that is good as a separate patch.
Reviewed-by: Ira Weiny <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>
> ---
> fs/exec.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 0989fb8472a1..4a2129c0d422 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -583,11 +583,11 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
>
> if (kmapped_page) {
> flush_dcache_page(kmapped_page);
> - kunmap(kmapped_page);
> + kunmap_local(kaddr);
> put_arg_page(kmapped_page);
> }
> kmapped_page = page;
> - kaddr = kmap(kmapped_page);
> + kaddr = kmap_local_page(kmapped_page);
> kpos = pos & PAGE_MASK;
> flush_arg_page(bprm, kpos, kmapped_page);
> }
> @@ -601,7 +601,7 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
> out:
> if (kmapped_page) {
> flush_dcache_page(kmapped_page);
> - kunmap(kmapped_page);
> + kunmap_local(kaddr);
> put_arg_page(kmapped_page);
> }
> return ret;
> @@ -883,11 +883,11 @@ int transfer_args_to_stack(struct linux_binprm *bprm,
>
> for (index = MAX_ARG_PAGES - 1; index >= stop; index--) {
> unsigned int offset = index == stop ? bprm->p & ~PAGE_MASK : 0;
> - char *src = kmap(bprm->page[index]) + offset;
> + char *src = kmap_local_page(bprm->page[index]) + offset;
> sp -= PAGE_SIZE - offset;
> if (copy_to_user((void *) sp, src, PAGE_SIZE - offset) != 0)
> ret = -EFAULT;
> - kunmap(bprm->page[index]);
> + kunmap_local(src);
> if (ret)
> goto out;
> }
> @@ -1680,13 +1680,13 @@ int remove_arg_zero(struct linux_binprm *bprm)
> ret = -EFAULT;
> goto out;
> }
> - kaddr = kmap_atomic(page);
> + kaddr = kmap_local_page(page);
>
> for (; offset < PAGE_SIZE && kaddr[offset];
> offset++, bprm->p++)
> ;
>
> - kunmap_atomic(kaddr);
> + kunmap_local(kaddr);
> put_arg_page(page);
> } while (offset == PAGE_SIZE);
>
> --
> 2.36.1
>
On venerdì 8 luglio 2022 22:18:35 CEST Ira Weiny wrote:
> On Thu, Jun 30, 2022 at 06:35:27PM +0200, Fabio M. De Francesco wrote:
> > The use of kmap() and kmap_atomic() are being deprecated in favor of
> > kmap_local_page().
> >
> > With kmap_local_page(), the mappings are per thread, CPU local and not
> > globally visible. Furthermore, the mappings can be acquired from any
> > context (including interrupts).
> >
> > Therefore, use kmap_local_page() in exec.c because these mappings are
per
> > thread, CPU local, and not globally visible.
> >
> > Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with
> > HIGHMEM64GB enabled.
> >
> > Suggested-by: Ira Weiny <[email protected]>
>
> This looks good but there is a kmap_atomic() in this file which I _think_
can
> be converted as well. But that is good as a separate patch.
>
> Reviewed-by: Ira Weiny <[email protected]>
>
Thanks for your review!
I didn't notice that kmap_atomic(). I'll send a conversion with a separate
patch.
Fabio