All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.