svn commit: r313821 - in head/sys: dev/cxgb/ulp/iw_cxgb fs/nfsserver kern netinet netinet/libalias netpfil/ipfw

[Bruce Evans]

On Thu, 16 Feb 2017, Gleb Smirnoff wrote:

>  heh, things are worse. Multiple places you changes are CTR()
> macros. Neither inet_ntoa() nor inet_ntoa_r() will work with
> them. :(
>
> Basicly non-constant strings can not be logged with KTR. All
> the lines you touched should log actual binary value of the
> IPv4 address.
>
> I am not asking you to do this work! :) But I couldn't leave
> that without a comment.

Nothing in KTR printing (exept %% and literals) works right.  %s is
only works for strings in kernel memory (fetched by kread() in
ktrdump(8)).  All args, even for the string pointers for %s, have type
u_long.  ktrdump doesn't check or translate formats, so all format
descriptors except %lu (possibly qualified) except %lu give undefined
behaviour that mostly works.  ktr_tracepoint() only takes (printing)
args of type u_long, and CTR6() casts to this.  These casts inhibit
type checking, and printf format is defeated by the use of non-const
strings when ktrdump sees the format and by not using a variadic
printf-like function for ktr_tracepoint().

> On Thu, Feb 16, 2017 at 08:47:41PM +0000, Eric van Gyzen wrote:
> E> Author: vangyzen
> E> Date: Thu Feb 16 20:47:41 2017
> E> New Revision: 313821
> E> URL: https://svnweb.freebsd.org/changeset/base/313821
> E>
> E> Log:
> E>   Use inet_ntoa_r() instead of inet_ntoa() throughout the kernel
> E>
> E>   inet_ntoa() cannot be used safely in a multithreaded environment
> E>   because it uses a static local buffer. Instead, use inet_ntoa_r()
> E>   with a buffer on the caller's stack.

This is a lot of churn.  _r functions are hard to use.  If inet_ntoa()
had not already existed, then someone should have invented it.

It used to be a reasonable hack for functions that return static buffers
to use an array of such buffers, with each caller getting a unique
buffer unless the calls arrive at a high rate or the buffers remain
in use for too long.  This is not good enough with threads and/or
preemption.  The kernel inet_ntoa() didn't even do that.

A good enough inet_ntoa() is easy to implement using alloca().  Something
like:

#define	inet_ntoa(in) __extension__ ({	\
 	char *__cp;			\
 					\
 	__cp = alloca(16);		\
 	inet_ntoa_r(in, __cp, 16);	\
})

I don't know how to do this using VLAs.  The allocation must live until
after leaving the the macro.  alloc() maks it live until the end of the
function.  Kernel use must be careful not to allocate too much using
alloca(), but this is not much harder than using explicit auto
declarations.  Compilers usually pessimize the auto declarations for
space, so their space is not overlapped or reclaimed until the end of
the function.  alloca() only creates garbage at a higher rate if it
is used in a loop.

Converting to use malloc() would be worse than using auto declarations,
since malloc() is slower and messier to clean up, though the actual
malloc() can be hidden in a macro like the above.  Implementations
of alloca() on to of malloc() try to do garbage collection on function
return (really on the next call to alloca(), if it detects that results
of previous call(s) are garbage by looking at magic related to the calls).
The latter is related to the simpler implementation with an array of static
buffers.

Even more simply and portably, I think it works to use a per-thread buffer:

 	char	td_msgbuf[32];		/* Buffer for small messages. */
...
#define	td_alloc_msg(size)	...
#define	inet_ntoa(in)	inet_ntoa_r(in, td_alloc_msg(16), 16)

where td_alloc_msg(size) has to allocate 'size' bytes in
curthread->td_msbug, circularly, starting at the previous position.
This is the implementation using a static array, except I made
td_msgbuf[64] generic so that it can be used for other functions with
small buffer sizes other than 16.  The sub-buffers are unique for each
thread provided too many of them aren't used in a single statement.
64 allows just 2 buffers of size 16 for use in code like:

 	printf("src %s, dst %s\n", inet_ntoa(src), inet_ntoa(dst));

You can also have 2 active assignments of inet_ntoa(any) at a time.  If
you want more, then expand the 32.  This is already easier to use than
the old inet_ntoa(9) -- using that required the discipline of only
having 1 active assignment at a time, and using a UP && !PREEMPTION
kernel to avoid races.

Even more simpler to implement, but harder to use:

 	char	td_inet_ntoa_buf[16];
...
#define	inet_ntoa(in)	inet_ntoa_r(in, curthread->td_inet_ntoa_buf, 16)

Callers now have to copy the result if they want to use more than 1 result.
Old code already did this.

Bruce