On Tue, May 16, 2023 at 4:37 PM Jeff Xu <jeffxu@xxxxxxxxxx> wrote:
>
> On Tue, May 16, 2023 at 4:19 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
> >
> > On 5/15/23 06:05, jeffxu@xxxxxxxxxxxx wrote:
> > > /*
> > > * pkey==-1 when doing a legacy mprotect()
> > > + * syscall==true if this is called by syscall from userspace.
> > > + * Note: this is always true for now, added as a reminder in case that
> > > + * do_mprotect_pkey is called directly by kernel in the future.
> > > + * Also it is consistent with __do_munmap().
> > > */
> > > static int do_mprotect_pkey(unsigned long start, size_t len,
> > > - unsigned long prot, int pkey)
> > > + unsigned long prot, int pkey, bool syscall)
> > > {
> >
> > The 'syscall' seems kinda silly (and a bit confusing). It's easy to
> > check if the caller is a kthread or has a current->mm==NULL. If you
> > *really* want a warning, I'd check for those rather than plumb a
> > apparently unused argument in here.
> >
> > BTW, this warning is one of those things that will probably cause some
> > amount of angst. I'd move it to the end of the series or just axe it
> > completely.
>
Okay, I will move the logging part to the end of the series.
> Agreed. syscall is not a good name here.
> The intention is to check this at the system call entry point
> For example, munmap can get called inside mremap(), but by that time
> mremap() should already check that all the memory is writeable.
>
> I will remove "syscall" from do_mprotect_pkey signature, it seems it caused
> more confusion than helpful. I will keep the comments/note in place to remind
> future developer.
