svn commit: r330285 - head/sys/sys

[Konstantin Belousov]

On Fri, Mar 02, 2018 at 12:43:34PM -0500, Pedro Giffuni wrote:
> (cc in Eitan as he may be interested in the extra restrict cases)
> 
> 
> On 02/03/2018 11:47, Konstantin Belousov wrote:
> > Author: kib
> > Date: Fri Mar  2 16:47:02 2018
> > New Revision: 330285
> > URL: https://svnweb.freebsd.org/changeset/base/330285
> >
> > Log:
> >    Remove _Nonnull attributes from user addresses arguments for
> >    copyout(9) family.
> >    
> >    The addresses are user-controllable, and if the process ABI allows
> >    mapping at zero, then the zero address is meaningful, contradicting
> >    the definition of _Nonnull.  In any case, it does not require any
> >    special code to handle NULL udaddr.
> >    
> 
> FWIW, the _Nonnull attributes didn't do much at all beyond producing a 
> warning.
> They replaced the GNU __nonnull() attributes which were much more dangerous.
> I am OK with seeing both gone here though.
> 
> >    It is not clear if __restrict makes sense as well, since kaddr and
> >    udaddr point to different address spaces, so equal numeric values of
> >    the pointers do not imply aliasing and a legitimate.  But leave it for
> >    later.
> >    
> >    copyinstr(9) does not have its user address argument annotated.
> 
> I think use of _Nonnull attributes in the threading functions may also 
> be a waste (I introduced them mostly to be compatible with Android).
> FWIW, Dragonfly sprinkled some restrict there recently:
> 
> http://gitweb.dragonflybsd.org/dragonfly.git/commit/d33005aaee6af52c80428b59b52aee522c002492
> 
> Just in case someone is considering more cleanups.

This is not a cleanup for me, but a needed change. Right now x86
copyouts are implemented in asm, so whatever damage is done to the
prototypes, only effect is at the caller side. In my work, i386 copyouts
are done in C, so it starts matter.

Also I looked at the dragonfly commit because I become curious what do you
mean by threading functions.  The first example was
 int    pthread_attr_getguardsize(const pthread_attr_t * __restrict,
-                       size_t *);
+           size_t * __restrict);
POSIX agrees with the dragonfly change, but I do not understand it.
Aliasing rules already disallow the first and second arguments to point
to the same memory, because they have different types.