Re: [PATCH 0/9] mm: Hardened usercopy

[Ingo Molnar]

* PaX Team <pageexec@xxxxxxxxxxx> wrote:

> On 9 Jul 2016 at 14:27, Andy Lutomirski wrote:
> 
> > I like the series, but I have one minor nit to pick.  The effect of this 
> > series is to harden usercopy, but most of the code is really about 
> > infrastructure to validate that a pointed-to object is valid.
> 
> actually USERCOPY has never been about validating pointers. its sole purpose is 
> to validate the *size* argument of copy*user calls, a very specific form of 
> runtime bounds checking.

What this code has been about originally is largely immaterial, unless you can 
formulate it into a technical argument.

There are a number of cheap tests we can do and there are a number of ways how a 
'pointer' can be validated runtime, without any 'size' information:

 - for example if a pointer points into a red zone straight away then we know it's
   bogus.

 - or if a kernel pointer is points outside the valid kernel virtual memory range
   we know it's bogus as well.

So while only doing a bounds check might have been the original purpose of the 
patch set, Andy's point is that it might make sense to treat this facility as a 
more generic 'object validation' code of (pointer,size) object and not limit it to 
'runtime bounds checking'. That kind of extended purpose behind a facility should 
be reflected in the naming.

Confusing names are often the source of misunderstandings and bugs.

The 9-patch series as submitted here is neither just 'bounds checking' nor just 
pure 'pointer checking', it's about validating that a (pointer,size) range of 
memory passed to a (user) memory copy function is fully within a valid object the 
kernel might know about (in an fast to check fashion).

This necessary means:

 - the start of the range points to a valid object to begin with (if known)

 - the range itself does not point beyond the end of the object (if known)

 - even if the kernel does not know anything about the pointed to object it can 
   do a pointer check (for example is it pointing inside kernel virtual memory) 
   and do a bounds check on the size.

Do you disagree with that?

> > Might it make sense to call the infrastructure part something else?
> 
> yes, more bikeshedding will surely help, [...]

Insulting and ridiculing a reviewer who explicitly qualified his comments with 
"one minor nit to pick" sure does not help upstream integration either. (Unless 
the goal is to prevent upstream integration.)

> [...] like the renaming of .data..read_only to .data..ro_after_init which also 
> had nothing to do with init but everything to do with objects being conceptually 
> read-only...

.data..ro_after_init objects get written to during bootup so it's conceptually 
quite confusing to name it "read-only" without any clear qualifiers.

That it's named consistently with its role of "read-write before init and read 
only after init" on the other hand is not confusing at all. Not sure what your 
problem is with the new name.

Names within submitted patches get renamed on a routine basis during review. It's 
often only minor improvements in naming (which you can consider bike shedding), 
but in this particular case the rename was clearly useful in not just improving 
the name but in avoiding an actively confusing name. So I disagree not just with 
the hostile tone of your reply but with your underlying technical point as well.

Thanks,

	Ingo