Fix sporadic odd MSS values when TCP HostCache is not in use

[rscheff]

Summary

When the use of the TCP Hostcache facility is disabled (not the default),
a code path exists where an uninitialized struct hc_metrics_lite is
presenting some small MSS value, which can end up being used; Note that
in the majority of cases, the expectation would be for the uninitialized
values to hold larger MSS values, which are clamped down to the MTU of
the interface.

The probability to hit this is therefore only 1 / (uint32/(1500-576)) or 2.1E-7

The offending branch is line 440/441 in tcp_hostcache:

void
tcp_hc_get(struct in_conninfo *inc, struct hc_metrics_lite *hc_metrics_lite)
{
    struct hc_metrics *hc_entry;

    if (!V_tcp_use_hostcache)
	return;

while other branches all return or zero the struct.

However, initializing the struct in tcp_mss_update also a addresses a
compiler warning.

Test Plan

Disable tcp hostcache
Validate MSS Option during TCP session setup, for about 10 million
handshakes.

Without this fix, one or two of those may show some random, small MSS
value - provided the compiler doesn't automatically produce
initialization code when entering a function (some do, some
don't; C99 does not define this behavior)

[rscheff]

• fix the uninit hc_metrics_lite issue with belt and suspenders

[cc]

Even better. Tested on my side. Looks good.

[tuexen]

I think the proposed fix tcp_hc_get() is the correct thing to do. I don't think we need the initialisers in cc_conn_init() and tcp_mss_update. Which compiler warning are you referring to?

[rscheff]

We are using a commercial compiler suite internally, using a complex build toolchain and automation, but traditionally ignore warnings in the fbsd code around possibly uninitialized variables. Setting them explicitly to zero does silence these two instances. But the normal fbsd build doesn't throw errors around such instances (and gcc / clang may even emit initialization code under the hood).

Also, while initializing a struct on the stack like this is not normally done in the /sys/netinet/tcp* area, many similar examples can be found in the hw drivers (/sys/dev).

As mentioned, this is a belt-and-suspender fix, the bzero in tcp_hc_get alone should be sufficient to address future uses of that function; initialization of stack variables is not necessarily done in C99 and the behavior of different compilers apparently came into play here.

[tuexen]

OK, I double checked the code. I think the way to go is taking your change to tcp_hc_get() and not do the initialisations in cc_conn_init() and tcp_mss_update().
It is clear that tcp_hc_get() always returns the memory located at hc_metrics_lite in a correct state. So there is no need to initialise the memory before calling tcp_hc_get().
The code should be clear on this.

I also think that the compiler warning is a false positive. If you see a way that part of the variable metrics is used uninitialised please let me know. If not,
please update the review by removing the changes to tcp_input.c. Then I'll approve and commit.

[rgrimes]

In D23687#520879, @tuexen wrote:

OK, I double checked the code. I think the way to go is taking your change to tcp_hc_get() and not do the initialisations in cc_conn_init() and tcp_mss_update().

...
I concur with Michael's conclusion, the struct initializers are unneeded and wasteful of cycles, and accept the differential based on them being removed before commiot.

[rscheff]

• always return sane struct in tcp_hc_get