On Mon, Nov 11, 2024 at 2:35 PM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> > To make modulization better, I can do below adjustment:
> > if (seal_system_mapping_enabled()) <-- implemented by fs/exec.c
> > add_vm_sealed() <- keep in include/linux/mm.h
> >
> > However, if you have a strong opinion on this, I could move the
> > parsing logic to mm/mseal.
>
> Yes, please move the parsing logic together with the other mseal code.
>
Sure
>
> I am not sure what one you are renaming or what these functions would
> do. I think you need to look at your overall design and try to fit it
> into what exists instead of putting a call in a wrapper function
> (_install_special_mapping) to alter an argument.
>
Please see V3.
> >
> > > > > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > > > > index 57fd5ab2abe7..d4717e34a60d 100644
> > > > > > --- a/mm/mmap.c
> > > > > > +++ b/mm/mmap.c
> > > > > > @@ -2133,6 +2133,7 @@ struct vm_area_struct *_install_special_mapping(
> > > > > > unsigned long addr, unsigned long len,
> > > > > > unsigned long vm_flags, const struct vm_special_mapping *spec)
> > > > > > {
> > > > > > + update_seal_exec_system_mappings(&vm_flags);
> > > > > > return __install_special_mapping(mm, addr, len, vm_flags, (void *)spec,
> > > > > > &special_mapping_vmops);
> > > > >
> > > > > If you were to return a flag, you could change the vm_flags argument to
> > > > > vm_flags | mseal_flag()
> > > > >
> > > > passing pointer seems to be the most efficient way.
> > >
> > > I disagree. Here is the godbolt.org output for gcc x86-64 14.2 of your
> > > code (with some added #defines to make it compile)
> > >
> > > seal_system_mappings:
> > > .long 1
> > > seal_system_mappings_enabled:
> > > push rbp
> > > mov rbp, rsp
> > > mov eax, DWORD PTR seal_system_mappings[rip]
> > > cmp eax, 1
> > > jne .L2
> > > mov eax, 1
> > > jmp .L3
> > > .L2:
> > > mov eax, 0
> > > .L3:
> > > pop rbp
> > > ret
> > > update_seal_exec_system_mappings:
> > > push rbp
> > > mov rbp, rsp
> > > sub rsp, 8
> > > mov QWORD PTR [rbp-8], rdi
> > > mov rax, QWORD PTR [rbp-8]
> > > mov rax, QWORD PTR [rax]
> > > and eax, 2
> > > test rax, rax
> > > jne .L6
> > > call seal_system_mappings_enabled
> > > test al, al
> > > je .L6
> > > mov rax, QWORD PTR [rbp-8]
> > > mov rax, QWORD PTR [rax]
> > > or rax, 2
> > > mov rdx, rax
> > > mov rax, QWORD PTR [rbp-8]
> > > mov QWORD PTR [rax], rdx
> > > .L6:
> > > nop
> > > leave
> > > ret
> > > main:
> > > push rbp
> > > mov rbp, rsp
> > > sub rsp, 16
> > > mov QWORD PTR [rbp-8], 0
> > > lea rax, [rbp-8]
> > > mov rdi, rax
> > > call update_seal_exec_system_mappings
> > > mov rax, QWORD PTR [rbp-8]
> > > leave
> > > ret
> > >
> > > ----- 48 lines -----
> > > Here is what I am suggesting to do with replacing the passing of a
> > > pointer with a concise "vm_flags | mseal_exec_flags()" (with the same
> > > added #defines to make it compile)
> > >
> > > seal_system_mappings:
> > > .long 1
> > > mseal_exec_flags:
> > > push rbp
> > > mov rbp, rsp
> > > mov eax, DWORD PTR seal_system_mappings[rip]
> > > cmp eax, 1
> > > jne .L2
> > > mov eax, 2
> > > jmp .L3
> > > .L2:
> > > mov eax, 0
> > > .L3:
> > > pop rbp
> > > ret
> > > main:
> > > push rbp
> > > mov rbp, rsp
> > > sub rsp, 16
> > > mov QWORD PTR [rbp-8], 0
> > > call mseal_exec_flags
> > > mov edx, eax
> > > mov rax, QWORD PTR [rbp-8]
> > > or eax, edx
> > > leave
> > > ret
> > >
> > > ----- 26 lines -----
> > >
> > > So as you can see, there are less instructions in my version; there are
> > > 47.92% less lines of assembly.
> > >
> > vm_flags already run out of space in 32 bit, sooner or later we will
> > need to change that to *** a struct ***, passing address will be
> > becoming necessary with struct. Since this is not a performance
> > sensitive code path, 3 or 4 times during execve(), I think it would be
> > good to start here.
>
> No.
>
> I have pointed out why this is not 'the most efficient way' and you are
> now switching your argument to 'this will be needed in the future'.
>
> Please fix this.
>
Sure, updated in V3
