brk should not be implemented in purecap, return -ENOSYS when not in compat.
Co-authored-by: Tudor Cretu tudor.cretu@arm.com Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com --- Thanks Tudor for providing the code snippet, making it much more clear than my original ideas.
mm/mmap.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/mm/mmap.c b/mm/mmap.c index ce282f9d9f8e..1048c358b872 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -194,6 +194,11 @@ static int do_brk_flags(unsigned long addr, unsigned long request, unsigned long struct list_head *uf); SYSCALL_DEFINE1(brk, unsigned long, brk) { +#ifdef CONFIG_CHERI_PURECAP_UABI + if (!in_compat_syscall()) { + return -ENOSYS; + } +#endif unsigned long newbrk, oldbrk, origbrk; struct mm_struct *mm = current->mm; struct vm_area_struct *next;
Hi Teo,
On 17-10-2022 17:55, Teo Couprie Diaz wrote:
brk should not be implemented in purecap, return -ENOSYS when not in compat.
Wrap the commit description to 75 chars per line.
Co-authored-by: Tudor Cretu tudor.cretu@arm.com
For future references, the correct tag is: Co-Developed-by. I appreciate adding me! In this case, I think it's more appropriate if you remove my name as it's your idea and your testing whereas I merely suggested an alternative, so no need to credit me for it 😉.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
Thanks Tudor for providing the code snippet, making it much more clear than my original ideas.
My pleasure, good job for the patches!
mm/mmap.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/mm/mmap.c b/mm/mmap.c index ce282f9d9f8e..1048c358b872 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -194,6 +194,11 @@ static int do_brk_flags(unsigned long addr, unsigned long request, unsigned long struct list_head *uf); SYSCALL_DEFINE1(brk, unsigned long, brk) { +#ifdef CONFIG_CHERI_PURECAP_UABI
- if (!in_compat_syscall()) {
- return -ENOSYS;
- }
+#endif
The code looks good to me. There are a few formatting nits, use tabs instead of spaces. Make sure to set your tab width to 8 spaces to check if everything aligns nicely as you expected. Also, no need for braces for single-statement blocks. Make sure to use checkpatch if it complains about anything else.
Thanks, Tudor
unsigned long newbrk, oldbrk, origbrk; struct mm_struct *mm = current->mm; struct vm_area_struct *next;
On 17/10/2022 19:29, Tudor Cretu wrote:
Hi Teo,
On 17-10-2022 17:55, Teo Couprie Diaz wrote:
brk should not be implemented in purecap, return -ENOSYS when not in compat.
Wrap the commit description to 75 chars per line.
Co-authored-by: Tudor Cretu tudor.cretu@arm.com
For future references, the correct tag is: Co-Developed-by. I appreciate adding me! In this case, I think it's more appropriate if you remove my name as it's your idea and your testing whereas I merely suggested an alternative, so no need to credit me for it 😉.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
Thanks Tudor for providing the code snippet, making it much more clear than my original ideas.
My pleasure, good job for the patches!
mm/mmap.c | 5 +++++ Â 1 file changed, 5 insertions(+)
diff --git a/mm/mmap.c b/mm/mmap.c index ce282f9d9f8e..1048c358b872 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -194,6 +194,11 @@ static int do_brk_flags(unsigned long addr, unsigned long request, unsigned long          struct list_head *uf);  SYSCALL_DEFINE1(brk, unsigned long, brk)  { +#ifdef CONFIG_CHERI_PURECAP_UABI + if (!in_compat_syscall()) { +   return -ENOSYS; + } +#endif
The code looks good to me. There are a few formatting nits, use tabs instead of spaces. Make sure to set your tab width to 8 spaces to check if everything aligns nicely as you expected. Also, no need for braces for single-statement blocks. Make sure to use checkpatch if it complains about anything else.
In addition to these (thanks Tudor), note that the kernel still uses the C89 style for local variable declarations, that is they should always be at the beginning of the current block. You should therefore move this after the declarations.
You would normally get a warning for this, but it hasn't worked on Clang until recently. Fortunately the upstream fix got recently backported on Morello LLVM [1], so we should get that warning again in the next release of Morello LLVM.
Kevin
[1] https://git.morello-project.org/morello/llvm-project/-/merge_requests/198
Thanks, Tudor
unsigned long newbrk, oldbrk, origbrk; Â Â Â Â Â struct mm_struct *mm = current->mm; Â Â Â Â Â struct vm_area_struct *next;
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
On 18/10/2022 10:06, Kevin Brodsky wrote:
On 17/10/2022 19:29, Tudor Cretu wrote:
Hi Teo,
On 17-10-2022 17:55, Teo Couprie Diaz wrote:
brk should not be implemented in purecap, return -ENOSYS when not in compat.
Wrap the commit description to 75 chars per line.
Co-authored-by: Tudor Cretu tudor.cretu@arm.com
For future references, the correct tag is: Co-Developed-by. I appreciate adding me! In this case, I think it's more appropriate if you remove my name as it's your idea and your testing whereas I merely suggested an alternative, so no need to credit me for it 😉.
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
Thanks Tudor for providing the code snippet, making it much more clear than my original ideas.
My pleasure, good job for the patches!
mm/mmap.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/mm/mmap.c b/mm/mmap.c index ce282f9d9f8e..1048c358b872 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -194,6 +194,11 @@ static int do_brk_flags(unsigned long addr, unsigned long request, unsigned long struct list_head *uf); SYSCALL_DEFINE1(brk, unsigned long, brk) { +#ifdef CONFIG_CHERI_PURECAP_UABI
- if (!in_compat_syscall()) {
- return -ENOSYS;
- }
+#endif
The code looks good to me. There are a few formatting nits, use tabs instead of spaces. Make sure to set your tab width to 8 spaces to check if everything aligns nicely as you expected. Also, no need for braces for single-statement blocks. Make sure to use checkpatch if it complains about anything else.
In addition to these (thanks Tudor), note that the kernel still uses the C89 style for local variable declarations, that is they should always be at the beginning of the current block. You should therefore move this after the declarations.
Right, that makes sense. Will move it after the declarations.
You would normally get a warning for this, but it hasn't worked on Clang until recently. Fortunately the upstream fix got recently backported on Morello LLVM [1], so we should get that warning again in the next release of Morello LLVM.
Interesting, that would explain why I did have some when working on Xen but not here ! I'll try to pay attention to it in the meantime.
Kevin
[1] https://git.morello-project.org/morello/llvm-project/-/merge_requests/198
Thanks, Téo
Thanks, Tudor
unsigned long newbrk, oldbrk, origbrk; struct mm_struct *mm = current->mm; struct vm_area_struct *next;
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Hi Tudor,
On 17/10/2022 18:29, Tudor Cretu wrote:
Hi Teo,
On 17-10-2022 17:55, Teo Couprie Diaz wrote:
Co-authored-by: Tudor Cretu tudor.cretu@arm.com
For future references, the correct tag is: Co-Developed-by.
Interesting, I didn't come across it before and grepping the git log did confirm that "Co-authored" was used at a least a few times. Will keep that in mind !
I appreciate adding me! In this case, I think it's more appropriate if you remove my name as it's your idea and your testing whereas I merely suggested an alternative, so no need to credit me for it 😉.
I'll trust your judgment then !
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
Thanks Tudor for providing the code snippet, making it much more clear than my original ideas.
My pleasure, good job for the patches!
mm/mmap.c | 5 +++++ Â 1 file changed, 5 insertions(+)
diff --git a/mm/mmap.c b/mm/mmap.c index ce282f9d9f8e..1048c358b872 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -194,6 +194,11 @@ static int do_brk_flags(unsigned long addr, unsigned long request, unsigned long          struct list_head *uf);  SYSCALL_DEFINE1(brk, unsigned long, brk)  { +#ifdef CONFIG_CHERI_PURECAP_UABI + if (!in_compat_syscall()) { +   return -ENOSYS; + } +#endif
The code looks good to me. There are a few formatting nits, use tabs instead of spaces. Make sure to set your tab width to 8 spaces to check if everything aligns nicely as you expected. Also, no need for braces for single-statement blocks. Make sure to use checkpatch if it complains about anything else.
Checkpatch complains about using -ENOSYS but that's to be expected, as we want it to act as if brk was not implemented and that seems to be valid.
Thanks, Tudor
Thanks for the review Tudor, mainly it's me not paying attention. I'll probably add a git hook to run checkpatch on commit to try and limit that. Téo
On 18/10/2022 10:08, Teo Couprie Diaz wrote:
Hi Tudor,
On 17/10/2022 18:29, Tudor Cretu wrote:
Hi Teo,
On 17-10-2022 17:55, Teo Couprie Diaz wrote:
Co-authored-by: Tudor Cretu tudor.cretu@arm.com
For future references, the correct tag is: Co-Developed-by.
Interesting, I didn't come across it before and grepping the git log did confirm that "Co-authored" was used at a least a few times. Will keep that in mind !
I appreciate adding me! In this case, I think it's more appropriate if you remove my name as it's your idea and your testing whereas I merely suggested an alternative, so no need to credit me for it 😉.
I'll trust your judgment then !
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
Thanks Tudor for providing the code snippet, making it much more clear than my original ideas.
My pleasure, good job for the patches!
mm/mmap.c | 5 +++++ Â 1 file changed, 5 insertions(+)
diff --git a/mm/mmap.c b/mm/mmap.c index ce282f9d9f8e..1048c358b872 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -194,6 +194,11 @@ static int do_brk_flags(unsigned long addr, unsigned long request, unsigned long          struct list_head *uf);  SYSCALL_DEFINE1(brk, unsigned long, brk)  { +#ifdef CONFIG_CHERI_PURECAP_UABI + if (!in_compat_syscall()) { +   return -ENOSYS; + } +#endif
The code looks good to me. There are a few formatting nits, use tabs instead of spaces. Make sure to set your tab width to 8 spaces to check if everything aligns nicely as you expected. Also, no need for braces for single-statement blocks. Make sure to use checkpatch if it complains about anything else.
Checkpatch complains about using -ENOSYS but that's to be expected, as we want it to act as if brk was not implemented and that seems to be valid.
Thanks, Tudor
Thanks for the review Tudor, mainly it's me not paying attention. I'll probably add a git hook to run checkpatch on commit to try and limit that. Téo
If you haven't already, worth a look at these also:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html https://www.kernel.org/doc/html/latest/process/coding-style.html
Thanks, Zach
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
On 18/10/2022 10:25, Zachary Leaf wrote:
On 18/10/2022 10:08, Teo Couprie Diaz wrote:
Hi Tudor,
On 17/10/2022 18:29, Tudor Cretu wrote:
Hi Teo,
On 17-10-2022 17:55, Teo Couprie Diaz wrote:
Co-authored-by: Tudor Cretu tudor.cretu@arm.com
For future references, the correct tag is: Co-Developed-by.
Interesting, I didn't come across it before and grepping the git log did confirm that "Co-authored" was used at a least a few times. Will keep that in mind !
I appreciate adding me! In this case, I think it's more appropriate if you remove my name as it's your idea and your testing whereas I merely suggested an alternative, so no need to credit me for it 😉.
I'll trust your judgment then !
Signed-off-by: Teo Couprie Diaz teo.coupriediaz@arm.com
Thanks Tudor for providing the code snippet, making it much more clear than my original ideas.
My pleasure, good job for the patches!
mm/mmap.c | 5 +++++ Â 1 file changed, 5 insertions(+)
diff --git a/mm/mmap.c b/mm/mmap.c index ce282f9d9f8e..1048c358b872 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -194,6 +194,11 @@ static int do_brk_flags(unsigned long addr, unsigned long request, unsigned long          struct list_head *uf);  SYSCALL_DEFINE1(brk, unsigned long, brk)  { +#ifdef CONFIG_CHERI_PURECAP_UABI + if (!in_compat_syscall()) { +   return -ENOSYS; + } +#endif
The code looks good to me. There are a few formatting nits, use tabs instead of spaces. Make sure to set your tab width to 8 spaces to check if everything aligns nicely as you expected. Also, no need for braces for single-statement blocks. Make sure to use checkpatch if it complains about anything else.
Checkpatch complains about using -ENOSYS but that's to be expected, as we want it to act as if brk was not implemented and that seems to be valid.
Thanks, Tudor
Thanks for the review Tudor, mainly it's me not paying attention. I'll probably add a git hook to run checkpatch on commit to try and limit that. Téo
If you haven't already, worth a look at these also:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html https://www.kernel.org/doc/html/latest/process/coding-style.html
They are good to have indeed, thanks Zach !
Thanks, Zach
Téo
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Teo Couprie Diaz -
Tudor Cretu -
Zachary Leaf