Re: [RFC PATCH v2 0/1] seal system mappings

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Jeff Xu <jeffxu@chromium.org> [241016 20:59]:
> On Wed, Oct 16, 2024 at 4:18 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * jeffxu@chromium.org <jeffxu@chromium.org> [241014 17:50]:
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > Seal vdso, vvar, sigpage, uprobes and vsyscall.
> > >
> > > Those mappings are readonly or executable only, sealing can protect
> > > them from ever changing during the life time of the process. For
> > > complete descriptions of memory sealing, please see mseal.rst [1].
> > >
> > > System mappings such as vdso, vvar, and sigpage (for arm) are
> > > generated by the kernel during program initialization. These mappings
> > > are designated as non-writable, and sealing them will prevent them
> > > from ever becoming writeable.
> >                               ^ or ever removed.
> >
> > This is a pretty big deal.  Platforms are trying to make it so that vdso
> > is the fast path, but if they are removed then things stop using them
> > and it's not a problem.  This description is robbing them of the
> > information they need to know that, and it's not in your change log
> > either.
> >
> > I had said before that you need to be clear about the inability to
> > remove the mappings that are sealed, as the description above heavily
> > implies that it is only stopping them from becoming writeable.
> >
> The mseal.rst has the full description about memory sealing, I don't
> see the point to repeat all the blocked operations in the commit
> message here.

That's not what I said or asked for.

It's three words to add.

> 
> I don't know why you would think this heavily implies that only
> stopping them from becoming writable, 

Because you left it out of the description that states what this does,
and it's three words that adds a whole lot of context.

I told you to add that detail because it is _way_ more important to
almost everyone that you are making mappings exist for the lifetime of
the process than the fact that not writeable mappings will still be not
writeable.

One points out the fundamental change to the mappings you list while the
other tells people how things won't change.  I get that not changing is
the point of the patch, but it's not the only thing it does.

> There is already reminder: **
> For complete descriptions ** of memory sealing, please see mseal.rst

Getting good code reviews is difficult.  There are very few people who
do a good job of the very few people who do it at all.  If you want
people to read your code, then you need to spoon feed it to them.

You have just put up a huge barrier to someone doing a code review at
all, let alone a good code review.  So if you want good code, you need
to provide good information with a low bar of entrance.  We can all
agree that good review creates good code (well, better code, at least).

What you have done here is limited the number of people who are willing
to even entertain the idea of looking at the following patch.  And of
the people who do read the patch, probably close to zero will read that
document.

It's (usually) nothing personal, it's just human nature and a function
of time.

Thanks,
Liam