* [PATCH] evl: fix the gdb accessing control buffer issue.
@ 2024-12-09 8:37 Qichen Qiu
2024-12-09 9:25 ` Florian Bezdeka
2024-12-09 15:01 ` Philippe Gerum
0 siblings, 2 replies; 9+ messages in thread
From: Qichen Qiu @ 2024-12-09 8:37 UTC (permalink / raw)
To: xenomai; +Cc: rpm, Qichen Qiu
Currently, EVL maps the control buffer using remap_pfn_range, tagging
the memory with VM_IO. However, this prevents access by gdb.
This patch introduces the control_mmap_access function for the control
VMA, enabling gdb access when CONFIG_HAVE_IOREMAP_PROT is supported on
the target architecture.
Signed-off-by: Qichen Qiu <ruiqurm@gmail.com>
---
kernel/evl/control.c | 53 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/kernel/evl/control.c b/kernel/evl/control.c
index 2d1b44f3b0dc..33ec47745740 100644
--- a/kernel/evl/control.c
+++ b/kernel/evl/control.c
@@ -8,6 +8,7 @@
#include <linux/mm.h>
#include <linux/sched/isolation.h>
#include <linux/bitmap.h>
+#include <linux/highmem.h>
#include <evl/memory.h>
#include <evl/factory.h>
#include <evl/tick.h>
@@ -335,8 +336,52 @@ static long control_ioctl(struct file *filp, unsigned int cmd,
return ret;
}
+static int control_mmap_access(struct vm_area_struct *vma, unsigned long addr,
+ void *buf, int len, int write)
+{
+ resource_size_t phys_addr;
+ pte_t *ptep, pte;
+ void *virt_addr;
+ spinlock_t *ptl;
+ struct page *page;
+ int offset = offset_in_page(addr);
+ int ret = -EINVAL;
+
+ if (follow_pte(vma, addr, &ptep, &ptl))
+ return -EINVAL;
+
+ pte = ptep_get(ptep);
+ pte_unmap_unlock(ptep, ptl);
+
+ phys_addr = (resource_size_t)pte_pfn(pte) << PAGE_SHIFT;
+ page = pfn_to_page(PFN_DOWN(phys_addr));
+
+ virt_addr = kmap(page) + offset;
+ if (!virt_addr) {
+ ret = -ENOMEM;
+ goto unmap;
+ }
+
+ if (write) {
+ memcpy(virt_addr, buf, len);
+ } else {
+ memcpy(buf, virt_addr, len);
+ }
+ ret = len;
+
+unmap:
+ kunmap(page);
+
+ return ret;
+}
+
+static const struct vm_operations_struct control_mmap_ops = {
+ .access = control_mmap_access
+};
+
static int control_mmap(struct file *filp, struct vm_area_struct *vma)
{
+ int err;
void *p = evl_get_heap_base(&evl_shared_heap);
unsigned long pfn = __pa(p) >> PAGE_SHIFT;
size_t len = vma->vm_end - vma->vm_start;
@@ -344,7 +389,13 @@ static int control_mmap(struct file *filp, struct vm_area_struct *vma)
if (len != evl_shm_size)
return -EINVAL;
- return remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
+ err = remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
+ if (err < 0){
+ return err;
+ }
+
+ vma->vm_ops = &control_mmap_ops;
+ return 0;
}
static const struct file_operations control_fops = {
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] evl: fix the gdb accessing control buffer issue.
2024-12-09 8:37 [PATCH] evl: fix the gdb accessing control buffer issue Qichen Qiu
@ 2024-12-09 9:25 ` Florian Bezdeka
2024-12-09 13:47 ` [PATCH v2] " Qichen Qiu
2024-12-09 15:11 ` [PATCH] " Philippe Gerum
2024-12-09 15:01 ` Philippe Gerum
1 sibling, 2 replies; 9+ messages in thread
From: Florian Bezdeka @ 2024-12-09 9:25 UTC (permalink / raw)
To: Qichen Qiu, xenomai; +Cc: rpm
On Mon, 2024-12-09 at 08:37 +0000, Qichen Qiu wrote:
> Currently, EVL maps the control buffer using remap_pfn_range, tagging
> the memory with VM_IO. However, this prevents access by gdb.
>
> This patch introduces the control_mmap_access function for the control
> VMA, enabling gdb access when CONFIG_HAVE_IOREMAP_PROT is supported on
> the target architecture.
@Philippe: I haven't checked the details yet, but the memory mapping is
different for Xenomai 3? Just wondering why this is necessary at all.
Does that mean that gdb did not work? On which platforms/architectures?
Is there no test for gdb like the gdb test in smokey?
>
> Signed-off-by: Qichen Qiu <ruiqurm@gmail.com>
> ---
> kernel/evl/control.c | 53 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/evl/control.c b/kernel/evl/control.c
> index 2d1b44f3b0dc..33ec47745740 100644
> --- a/kernel/evl/control.c
> +++ b/kernel/evl/control.c
> @@ -8,6 +8,7 @@
> #include <linux/mm.h>
> #include <linux/sched/isolation.h>
> #include <linux/bitmap.h>
> +#include <linux/highmem.h>
> #include <evl/memory.h>
> #include <evl/factory.h>
> #include <evl/tick.h>
> @@ -335,8 +336,52 @@ static long control_ioctl(struct file *filp, unsigned int cmd,
> return ret;
> }
>
> +static int control_mmap_access(struct vm_area_struct *vma, unsigned long addr,
> + void *buf, int len, int write)
> +{
> + resource_size_t phys_addr;
> + pte_t *ptep, pte;
> + void *virt_addr;
> + spinlock_t *ptl;
> + struct page *page;
> + int offset = offset_in_page(addr);
> + int ret = -EINVAL;
^^ Code style: "Reverse Christmas tree" ;-) Applies to the second hunk
as well.
DOS line endings everywhere...
> +
> + if (follow_pte(vma, addr, &ptep, &ptl))
> + return -EINVAL;
> +
> + pte = ptep_get(ptep);
> + pte_unmap_unlock(ptep, ptl);
> +
> + phys_addr = (resource_size_t)pte_pfn(pte) << PAGE_SHIFT;
> + page = pfn_to_page(PFN_DOWN(phys_addr));
> +
> + virt_addr = kmap(page) + offset;
kmap() as well as kunmap() below are deprecated (according to
checkpatch). kmap_local_page() and kunmap_local() should be preferred.
> + if (!virt_addr) {
> + ret = -ENOMEM;
> + goto unmap;
> + }
> +
> + if (write) {
> + memcpy(virt_addr, buf, len);
> + } else {
> + memcpy(buf, virt_addr, len);
> + }
Mixed code style. See checkpatch output.
> + ret = len;
> +
> +unmap:
> + kunmap(page);
> +
> + return ret;
> +}
> +
> +static const struct vm_operations_struct control_mmap_ops = {
> + .access = control_mmap_access
> +};
> +
> static int control_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> + int err;
> void *p = evl_get_heap_base(&evl_shared_heap);
> unsigned long pfn = __pa(p) >> PAGE_SHIFT;
> size_t len = vma->vm_end - vma->vm_start;
> @@ -344,7 +389,13 @@ static int control_mmap(struct file *filp, struct vm_area_struct *vma)
> if (len != evl_shm_size)
> return -EINVAL;
>
> - return remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
> + err = remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
> + if (err < 0){
if (err)
> + return err;
> + }
> +
> + vma->vm_ops = &control_mmap_ops;
> + return 0;
> }
>
> static const struct file_operations control_fops = {
> --
> 2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] evl: fix the gdb accessing control buffer issue
2024-12-09 9:25 ` Florian Bezdeka
@ 2024-12-09 13:47 ` Qichen Qiu
2024-12-09 15:11 ` [PATCH] " Philippe Gerum
1 sibling, 0 replies; 9+ messages in thread
From: Qichen Qiu @ 2024-12-09 13:47 UTC (permalink / raw)
To: florian.bezdeka; +Cc: rpm, ruiqurm, xenomai
Background
I tried using GDB to debug an EVL program but encountered an issue: I
couldn't access __evl_current_window. This variable resides in a
memory-mapped area of the control factory, where the kernel uses
remap_pfn_range to allocate a region for communication between the
kernel and user space. The remap_pfn_range function sets the VM_IO
flag for the memory, which may disallow gdb read the memory directly.
When GDB attempts to access this memory via __access_remote_vm, it
fails in get_user_pages_remote function, goes into the IS_ERR(page)
branch. If the CONFIG_HAVE_IOREMAP_PROT configuration is enabled,
which depends on architecture support, the function instead invokes
vma->vm_ops->access.
To address this, I implemented a custom vm_ops->access function that
allows GDB to copy values from this memory region, enabling proper
debugging. See the reference for detail.
Reproduce:
create a simple evl program:
```c
#include <stdio.h>
#include <evl/thread.h>
#include <evl/proxy.h>
#include <evl/timer.h>
int main()
{
int efd,tfd;
struct evl_sched_attrs evl_sched_attr;
evl_sched_attr.sched_priority = 1;
evl_sched_attr.sched_policy = SCHED_FIFO;
efd = evl_attach_self("a");
evl_set_schedattr(efd, &evl_sched_attr);
evl_printf("is_inband=%d\n",evl_is_inband());
}
```
Compile it with debug symbol. Here is an example command:
gcc -g -O0 -o a a.c /usr/local/lib/libevl.a
Debug it:
sudo gdb ./a -ex "b 14" -ex "r" -ex "p/x *__evl_current_window"
If the patch is not applied, the __evl_current_window can not be accessed.
Otherwise, it may print:
$1 = {state = 0x4080, info = 0x2000, pp_pending = 0x0}
(the gdb will affect the thread state)
Reference:
[1]: https://stackoverflow.com/questions/3640095/gdb-cant-access-mmapd-kernel-allocated-memory
[2]: https://stackoverflow.com/questions/654393/examining-mmaped-addresses-using-gdb
Signed-off-by: Qichen Qiu <ruiqurm@gmail.com>
---
kernel/evl/control.c | 52 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/kernel/evl/control.c b/kernel/evl/control.c
index 2d1b44f3b0dc..dba951d2e9d4 100644
--- a/kernel/evl/control.c
+++ b/kernel/evl/control.c
@@ -8,6 +8,7 @@
#include <linux/mm.h>
#include <linux/sched/isolation.h>
#include <linux/bitmap.h>
+#include <linux/highmem.h>
#include <evl/memory.h>
#include <evl/factory.h>
#include <evl/tick.h>
@@ -335,8 +336,52 @@ static long control_ioctl(struct file *filp, unsigned int cmd,
return ret;
}
+static int control_mmap_access(struct vm_area_struct *vma, unsigned long addr,
+ void *buf, int len, int write)
+{
+ resource_size_t phys_addr;
+ pte_t *ptep, pte;
+ void *virt_addr;
+ spinlock_t *ptl;
+ struct page *page;
+ int offset = offset_in_page(addr);
+ int ret = -EINVAL;
+
+ if (follow_pte(vma, addr, &ptep, &ptl))
+ return -EINVAL;
+
+ pte = ptep_get(ptep);
+ pte_unmap_unlock(ptep, ptl);
+
+ phys_addr = (resource_size_t)pte_pfn(pte) << PAGE_SHIFT;
+ page = pfn_to_page(PFN_DOWN(phys_addr));
+
+ virt_addr = kmap_local_page(page) + offset;
+ if (!virt_addr) {
+ ret = -ENOMEM;
+ goto unmap;
+ }
+
+ if (write)
+ memcpy(virt_addr, buf, len);
+ else
+ memcpy(buf, virt_addr, len);
+
+ ret = len;
+
+unmap:
+ kunmap_local(virt_addr);
+
+ return ret;
+}
+
+static const struct vm_operations_struct control_mmap_ops = {
+ .access = control_mmap_access
+};
+
static int control_mmap(struct file *filp, struct vm_area_struct *vma)
{
+ int err;
void *p = evl_get_heap_base(&evl_shared_heap);
unsigned long pfn = __pa(p) >> PAGE_SHIFT;
size_t len = vma->vm_end - vma->vm_start;
@@ -344,7 +389,12 @@ static int control_mmap(struct file *filp, struct vm_area_struct *vma)
if (len != evl_shm_size)
return -EINVAL;
- return remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
+ err = remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
+ if (err < 0)
+ return err;
+
+ vma->vm_ops = &control_mmap_ops;
+ return 0;
}
static const struct file_operations control_fops = {
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] evl: fix the gdb accessing control buffer issue.
2024-12-09 8:37 [PATCH] evl: fix the gdb accessing control buffer issue Qichen Qiu
2024-12-09 9:25 ` Florian Bezdeka
@ 2024-12-09 15:01 ` Philippe Gerum
2024-12-09 16:31 ` Qichen Qiu
2024-12-11 15:27 ` [PATCH] evl: replace `remap_pfn_range` with `vm_insert_page` in control_mmap Qichen Qiu
1 sibling, 2 replies; 9+ messages in thread
From: Philippe Gerum @ 2024-12-09 15:01 UTC (permalink / raw)
To: Qichen Qiu; +Cc: xenomai
Qichen Qiu <ruiqurm@gmail.com> writes:
> Currently, EVL maps the control buffer using remap_pfn_range, tagging
> the memory with VM_IO. However, this prevents access by gdb.
>
> This patch introduces the control_mmap_access function for the control
> VMA, enabling gdb access when CONFIG_HAVE_IOREMAP_PROT is supported on
> the target architecture.
>
> Signed-off-by: Qichen Qiu <ruiqurm@gmail.com>
> ---
> kernel/evl/control.c | 53 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/evl/control.c b/kernel/evl/control.c
> index 2d1b44f3b0dc..33ec47745740 100644
> --- a/kernel/evl/control.c
> +++ b/kernel/evl/control.c
> @@ -8,6 +8,7 @@
> #include <linux/mm.h>
> #include <linux/sched/isolation.h>
> #include <linux/bitmap.h>
> +#include <linux/highmem.h>
> #include <evl/memory.h>
> #include <evl/factory.h>
> #include <evl/tick.h>
> @@ -335,8 +336,52 @@ static long control_ioctl(struct file *filp, unsigned int cmd,
> return ret;
> }
>
> +static int control_mmap_access(struct vm_area_struct *vma, unsigned long addr,
> + void *buf, int len, int write)
> +{
> + resource_size_t phys_addr;
> + pte_t *ptep, pte;
> + void *virt_addr;
> + spinlock_t *ptl;
> + struct page *page;
> + int offset = offset_in_page(addr);
> + int ret = -EINVAL;
> +
> + if (follow_pte(vma, addr, &ptep, &ptl))
follow_pte() was deprecated in v6.12. We need to use the new
follow_pfnmap*() API from that point on.
> + return -EINVAL;
> +
> + pte = ptep_get(ptep);
> + pte_unmap_unlock(ptep, ptl);
> +
> + phys_addr = (resource_size_t)pte_pfn(pte) << PAGE_SHIFT;
> + page = pfn_to_page(PFN_DOWN(phys_addr));
> +
> + virt_addr = kmap(page) + offset;
> + if (!virt_addr) {
Mm, not sure about the test above.
> + ret = -ENOMEM;
> + goto unmap;
> + }
> +
> + if (write) {
> + memcpy(virt_addr, buf, len);
> + } else {
> + memcpy(buf, virt_addr, len);
> + }
> + ret = len;
> +
> +unmap:
> + kunmap(page);
> +
> + return ret;
> +}
> +
> +static const struct vm_operations_struct control_mmap_ops = {
> + .access = control_mmap_access
> +};
> +
> static int control_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> + int err;
> void *p = evl_get_heap_base(&evl_shared_heap);
> unsigned long pfn = __pa(p) >> PAGE_SHIFT;
> size_t len = vma->vm_end - vma->vm_start;
> @@ -344,7 +389,13 @@ static int control_mmap(struct file *filp, struct vm_area_struct *vma)
> if (len != evl_shm_size)
> return -EINVAL;
>
> - return remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
> + err = remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
> + if (err < 0){
> + return err;
> + }
> +
> + vma->vm_ops = &control_mmap_ops;
> + return 0;
> }
>
> static const struct file_operations control_fops = {
There may be another (version-agnostic) way: simply obtain the heap
memory from the vmalloc space instead of kmem/logical, then use
vm_insert_page() to populate the mapping, which would drop the
requirement for an access trampoline to pfn ranges. Dovetail deals with
vmalloc memory pinning appropriately.
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] evl: fix the gdb accessing control buffer issue.
2024-12-09 9:25 ` Florian Bezdeka
2024-12-09 13:47 ` [PATCH v2] " Qichen Qiu
@ 2024-12-09 15:11 ` Philippe Gerum
1 sibling, 0 replies; 9+ messages in thread
From: Philippe Gerum @ 2024-12-09 15:11 UTC (permalink / raw)
To: Florian Bezdeka; +Cc: Qichen Qiu, xenomai
Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> On Mon, 2024-12-09 at 08:37 +0000, Qichen Qiu wrote:
>> Currently, EVL maps the control buffer using remap_pfn_range, tagging
>> the memory with VM_IO. However, this prevents access by gdb.
>>
>> This patch introduces the control_mmap_access function for the control
>> VMA, enabling gdb access when CONFIG_HAVE_IOREMAP_PROT is supported on
>> the target architecture.
>
> @Philippe: I haven't checked the details yet, but the memory mapping is
> different for Xenomai 3? Just wondering why this is necessary at all.
>
Cobalt gets the u-mmapped heaps from the vmalloc space
(cobalt_umm_init), so we can populate the vma using vm_insert_page()
there (mmap_kmem_helper), which should not set VM_IO. EVL currently
obtains the heap from kmalloc instead, hence remap_pfn_range() to map a
range from the logical memory space. IOW, "that should be fine already"
(famous last words).
> Does that mean that gdb did not work? On which platforms/architectures?
> Is there no test for gdb like the gdb test in smokey?
>
The existing gdb test only checks the synchronous stop feature I
believe, not accessing the memory behind those mappings.
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] evl: fix the gdb accessing control buffer issue.
2024-12-09 15:01 ` Philippe Gerum
@ 2024-12-09 16:31 ` Qichen Qiu
2024-12-11 15:27 ` [PATCH] evl: replace `remap_pfn_range` with `vm_insert_page` in control_mmap Qichen Qiu
1 sibling, 0 replies; 9+ messages in thread
From: Qichen Qiu @ 2024-12-09 16:31 UTC (permalink / raw)
To: rpm; +Cc: ruiqurm, xenomai
> There may be another (version-agnostic) way: simply obtain the heap
> memory from the vmalloc space instead of kmem/logical, then use
> vm_insert_page() to populate the mapping, which would drop the
> requirement for an access trampoline to pfn ranges. Dovetail deals with
> vmalloc memory pinning appropriately.
Thanks. I will give this approach a try and see how it works.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] evl: replace `remap_pfn_range` with `vm_insert_page` in control_mmap.
2024-12-09 15:01 ` Philippe Gerum
2024-12-09 16:31 ` Qichen Qiu
@ 2024-12-11 15:27 ` Qichen Qiu
2024-12-16 14:22 ` Philippe Gerum
1 sibling, 1 reply; 9+ messages in thread
From: Qichen Qiu @ 2024-12-11 15:27 UTC (permalink / raw)
To: rpm; +Cc: ruiqurm, xenomai
Hi, I used vm_insert_page to map the area to user space, and it
successfully resolved the issue.
Signed-off-by: Qichen Qiu <ruiqurm@gmail.com>
---
kernel/evl/control.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/kernel/evl/control.c b/kernel/evl/control.c
index 2d1b44f3b0dc..eee4bcaad390 100644
--- a/kernel/evl/control.c
+++ b/kernel/evl/control.c
@@ -338,13 +338,22 @@ static long control_ioctl(struct file *filp, unsigned int cmd,
static int control_mmap(struct file *filp, struct vm_area_struct *vma)
{
void *p = evl_get_heap_base(&evl_shared_heap);
- unsigned long pfn = __pa(p) >> PAGE_SHIFT;
size_t len = vma->vm_end - vma->vm_start;
+ unsigned long addr = vma->vm_start;
+ int err;
if (len != evl_shm_size)
return -EINVAL;
+
+ vm_flags_clear(vma, VM_WRITE);
+
+ while (addr < vma->vm_end) {
+ err = vm_insert_page(vma, addr, virt_to_page(p));
+ if (err < 0)
+ return err;
+ addr += PAGE_SIZE;
+ p += PAGE_SIZE;
+ }
- return remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
+ return 0;
}
static const struct file_operations control_fops = {
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] evl: replace `remap_pfn_range` with `vm_insert_page` in control_mmap.
2024-12-11 15:27 ` [PATCH] evl: replace `remap_pfn_range` with `vm_insert_page` in control_mmap Qichen Qiu
@ 2024-12-16 14:22 ` Philippe Gerum
2024-12-17 14:26 ` Qichen Qiu
0 siblings, 1 reply; 9+ messages in thread
From: Philippe Gerum @ 2024-12-16 14:22 UTC (permalink / raw)
To: Qichen Qiu; +Cc: xenomai
Qichen Qiu <ruiqurm@gmail.com> writes:
> Hi, I used vm_insert_page to map the area to user space, and it
> successfully resolved the issue.
>
Thanks for the patch. A few observations:
- for this to work, you definitely need to update the system heap
allocation method accordingly in the same patch, i.e. using vzalloc
instead of kzalloc. This patch is incomplete.
- Why eagerly clearing the VM_WRITE flag from the vma?
- the short log should refer to what is being fixed as perceived by
$user, not how we do that. The latter should be part of the long log
instead. In this case, that would be something like
evl/memory: allow system heap inspection using GDB
The long log could then give a few details about GDB, ptracing, the
VM_IO issue, and the reasoning behind the change of interface from
remap_pfn_range to vm_insert_page.
> Signed-off-by: Qichen Qiu <ruiqurm@gmail.com>
> ---
> kernel/evl/control.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/evl/control.c b/kernel/evl/control.c
> index 2d1b44f3b0dc..eee4bcaad390 100644
> --- a/kernel/evl/control.c
> +++ b/kernel/evl/control.c
> @@ -338,13 +338,22 @@ static long control_ioctl(struct file *filp, unsigned int cmd,
> static int control_mmap(struct file *filp, struct vm_area_struct *vma)
> {
> void *p = evl_get_heap_base(&evl_shared_heap);
> - unsigned long pfn = __pa(p) >> PAGE_SHIFT;
> size_t len = vma->vm_end - vma->vm_start;
> + unsigned long addr = vma->vm_start;
> + int err;
>
> if (len != evl_shm_size)
> return -EINVAL;
> +
> + vm_flags_clear(vma, VM_WRITE);
> +
> + while (addr < vma->vm_end) {
> + err = vm_insert_page(vma, addr, virt_to_page(p));
> + if (err < 0)
> + return err;
> + addr += PAGE_SIZE;
> + p += PAGE_SIZE;
> + }
>
> - return remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
> + return 0;
> }
>
> static const struct file_operations control_fops = {
--
Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] evl: replace `remap_pfn_range` with `vm_insert_page` in control_mmap.
2024-12-16 14:22 ` Philippe Gerum
@ 2024-12-17 14:26 ` Qichen Qiu
0 siblings, 0 replies; 9+ messages in thread
From: Qichen Qiu @ 2024-12-17 14:26 UTC (permalink / raw)
To: Philippe Gerum; +Cc: xenomai
Thank you for your suggestions! I will take your feedback and send an
updated patch to the mailing list later
> - Why eagerly clearing the VM_WRITE flag from the vma?
I initially thought this was to prevent malicious flag modifications.
However, I later realized that the monitor might also need to use the
mapped area for certain operations that require write permissions. As a
result, I have removed this line in the new patch.
Philippe Gerum <rpm@xenomai.org> 于2024年12月16日周一 22:22写道:
>
> Qichen Qiu <ruiqurm@gmail.com> writes:
>
> > Hi, I used vm_insert_page to map the area to user space, and it
> > successfully resolved the issue.
> >
>
> Thanks for the patch. A few observations:
>
> - for this to work, you definitely need to update the system heap
> allocation method accordingly in the same patch, i.e. using vzalloc
> instead of kzalloc. This patch is incomplete.
>
> - Why eagerly clearing the VM_WRITE flag from the vma?
>
> - the short log should refer to what is being fixed as perceived by
> $user, not how we do that. The latter should be part of the long log
> instead. In this case, that would be something like
> evl/memory: allow system heap inspection using GDB
> The long log could then give a few details about GDB, ptracing, the
> VM_IO issue, and the reasoning behind the change of interface from
> remap_pfn_range to vm_insert_page.
>
> > Signed-off-by: Qichen Qiu <ruiqurm@gmail.com>
> > ---
> > kernel/evl/control.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/evl/control.c b/kernel/evl/control.c
> > index 2d1b44f3b0dc..eee4bcaad390 100644
> > --- a/kernel/evl/control.c
> > +++ b/kernel/evl/control.c
> > @@ -338,13 +338,22 @@ static long control_ioctl(struct file *filp, unsigned int cmd,
> > static int control_mmap(struct file *filp, struct vm_area_struct *vma)
> > {
> > void *p = evl_get_heap_base(&evl_shared_heap);
> > - unsigned long pfn = __pa(p) >> PAGE_SHIFT;
> > size_t len = vma->vm_end - vma->vm_start;
> > + unsigned long addr = vma->vm_start;
> > + int err;
> >
> > if (len != evl_shm_size)
> > return -EINVAL;
> > +
> > + vm_flags_clear(vma, VM_WRITE);
> > +
> > + while (addr < vma->vm_end) {
> > + err = vm_insert_page(vma, addr, virt_to_page(p));
> > + if (err < 0)
> > + return err;
> > + addr += PAGE_SIZE;
> > + p += PAGE_SIZE;
> > + }
> >
> > - return remap_pfn_range(vma, vma->vm_start, pfn, len, PAGE_SHARED);
> > + return 0;
> > }
> >
> > static const struct file_operations control_fops = {
>
> --
> Philippe.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-12-17 14:26 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-09 8:37 [PATCH] evl: fix the gdb accessing control buffer issue Qichen Qiu
2024-12-09 9:25 ` Florian Bezdeka
2024-12-09 13:47 ` [PATCH v2] " Qichen Qiu
2024-12-09 15:11 ` [PATCH] " Philippe Gerum
2024-12-09 15:01 ` Philippe Gerum
2024-12-09 16:31 ` Qichen Qiu
2024-12-11 15:27 ` [PATCH] evl: replace `remap_pfn_range` with `vm_insert_page` in control_mmap Qichen Qiu
2024-12-16 14:22 ` Philippe Gerum
2024-12-17 14:26 ` Qichen Qiu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.