Reentrant problem with inet_ntoa in the kernel

[Brooks Davis]

On Sat, Nov 04, 2006 at 03:01:00AM +0000, MQ wrote:
> I use google mail web interface to post messages, I can't connect to the
> google mail POP server because someone disabled it on the firewall :(
> 
> I don't know if this post will be better?
> 
> 2006/11/3, Max Laier <max at love2party.net>:
> >
> >Hello "MQ",
> >
> >your email client is seriously mis-configured, could you please look into
> >this as it is a bit annoying.
> >
> >On Friday 03 November 2006 10:46, MQ wrote:
> >> 2006/11/2, Brooks Davis <brooks at one-eyed-alien.net>:
> >> > On Thu, Nov 02, 2006 at 08:26:27AM +0000, . wrote:
> >> > > Hi,
> >> > >
> >> > > I am confused by the use of inet_ntoa function in the kernel.
> >> > >
> >> > > The function inet_ntoa in the /sys/libkern/inet_ntoa.c uses a
> >> > > static
> >> >
> >> > array
> >> >
> >> > > static char buf[4 * sizeof "123"];
> >> > > to store the result. And it returns the address of the array to the
> >> >
> >> > caller.
> >> >
> >> > > I think this inet_ntoa is not reentrant, though there are several
> >> >
> >> > functions
> >> >
> >> > > calling it. If two functions call it simultaneously, the result
> >> > > will be corrupted. Though I haven't really encountered this
> >> > > situation, it may
> >> >
> >> > occur
> >> >
> >> > > someday, especially when using multi-processors.
> >> > >
> >> > > There is another reentrant version of inet_ntoa called inet_ntoa_r
> >> > > in
> >> >
> >> > the
> >> >
> >> > > same file. It has been there for several years, but just used by
> >> > > ipfw2
> >> >
> >> > for
> >> >
> >> > > about four times in 7-CURRENT. In my patch, I replaced all the
> >> > > calls to inet_ntoa with calls to inet_ntoa_r.
> >> > >
> >> > > By the way, some of the original calls is written in this style:
> >> > > strcpy(buf, inet_ntoa(ip))
> >> > > The modified code is written in this style
> >> > > inet_ntoa_r(ip, buf)
> >> > > This change avoids a call to strcpy, and can save a little time.
> >> > >
> >> > > Here is the patch.
> >> >
> >> > http://people.freebsd.org/~delphij/misc/patch-itoa-by-nodummy-at-yeah
> >> >-net
> >> >
> >> > > I've already sent to PR(kern/104738), but got no reply, maybe it
> >> > > should
> >> >
> >> > be
> >> >
> >> > > discussed here first?
> >> >
> >> > I've got to agree with other posters that the stack variable
> >> > allocations are ugly.  What about extending log and printf to
> >> > understand ip4v addresses?  That's 90% of the uses and the others
> >> > appears to have buffers already.
> >> >
> >> > -- Brooks
> >> >
> >>
> >> Ugly? Why? Don't you use local variables in your sources?
> >
> >You have to understand, that stack space is a limited resource in the
> >kernel.  Some of the functions are leaf nodes in quite long call paths,
> >which means adding those buffers can hit quite hard.
> >
> >I guess what Brooks and I are trying to say is, that this needs to be
> >considered carefully.  A simple search and replace won't do.
> >
> >BTW, I took the PR for now and will look into it.  I don't think it's
> >something we need to rush, as I haven't seen any reports or indication of
> >real problems yet - fwiw we don't spew a lot of IP addresses to console
> >or log in normal operation.
> >
> >> By the way, implementing a printf/log which understands ipv4 address is
> >> tedious, perhaps.
> >
> >Actually, it's a ~15 line patch, I will see if I can get to it later
> >today.  If we reuse the number buffer we can even get away without
> >increase in stack space usage.  Whether or not this is a good idea is on
> >another page, but %I and %lI (for IPv6) would be available and we already
> >have %D and %b as special cases in the kernel.
> >
> I know for sure that the kernel stack is much smaller than that in
> userland programms, but is the kernel stack too small to contain
> another 32 bytes at most? I've no idea, and is there any way to trace
> the usage of the kernel stack? KGDB?
>
> The reason why I don't like the idea that adding the facility into
> printf is we already had the converter inet_ntoa, adding the facility
> seems duplicated. But if implemented, and use a local buffer, this
> should be a really good way to solve the reentrant problem.

Given that except for a few cases that should have obviously used
inet_ntoa_r, inet_ntoa is only used to feed printf and log, the right
answer is clearly to implement it in printf and log and the nuke this
fundamental broken API.

-- Brooks
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-net/attachments/20061105/4f582dde/attachment.pgp