svn commit: r324179 - head/sys/netinet

[Jonathan Looney]

Hi Julien,

I apologize for just getting to this, but your code just made it into my
local tree.

The non-INVARIANTS case looks incorrect. Because tw stays on the list, it
can end up stuck at the front of the queue and block everything behind it.

Personally, I would be more comfortable just panic'ing here. This indicates
a problem. And, depending on the way things got corrupted, you could end up
with other hard-to-debug problems if you continue with a corrupted state.

Jonathan

On Sun, Oct 1, 2017 at 5:20 PM, Julien Charbon <jch at freebsd.org> wrote:

> Author: jch
> Date: Sun Oct  1 21:20:28 2017
> New Revision: 324179
> URL: https://svnweb.freebsd.org/changeset/base/324179
>
> Log:
>   Fix an infinite loop in tcp_tw_2msl_scan() when an INP_TIMEWAIT inp has
>   been destroyed before its tcptw with INVARIANTS undefined.
>
>   This is a symmetric change of r307551:
>
>   A INP_TIMEWAIT inp should not be destroyed before its tcptw, and
> INVARIANTS
>   will catch this case.  If INVARIANTS is undefined it will emit a
> log(LOG_ERR)
>   and avoid a hard to debug infinite loop in tcp_tw_2msl_scan().
>
>   Reported by:          Ben Rubson, hselasky
>   Submitted by:         hselasky
>   Tested by:            Ben Rubson, jch
>   MFC after:            1 week
>   Sponsored by:         Verisign, inc
>   Differential Revision:        https://reviews.freebsd.org/D12267
>
> Modified:
>   head/sys/netinet/tcp_timewait.c
>
> Modified: head/sys/netinet/tcp_timewait.c
> ============================================================
> ==================
> --- head/sys/netinet/tcp_timewait.c     Sun Oct  1 20:12:30 2017
> (r324178)
> +++ head/sys/netinet/tcp_timewait.c     Sun Oct  1 21:20:28 2017
> (r324179)
> @@ -709,10 +709,29 @@ tcp_tw_2msl_scan(int reuse)
>                         INP_WLOCK(inp);
>                         tw = intotw(inp);
>                         if (in_pcbrele_wlocked(inp)) {
> -                               KASSERT(tw == NULL, ("%s: held last inp "
> -                                   "reference but tw not NULL",
> __func__));
> -                               INP_INFO_RUNLOCK(&V_tcbinfo);
> -                               continue;
> +                               if (__predict_true(tw == NULL)) {
> +                                       INP_INFO_RUNLOCK(&V_tcbinfo);
> +                                       continue;
> +                               } else {
> +                                       /* This should not happen as in
> TIMEWAIT
> +                                        * state the inp should not be
> destroyed
> +                                        * before its tcptw. If INVARIANTS
> is
> +                                        * defined panic.
> +                                        */
> +#ifdef INVARIANTS
> +                                       panic("%s: Panic before an
> infinite "
> +                                           "loop: INP_TIMEWAIT &&
> (INP_FREED "
> +                                           "|| inp last reference) && tw
> != "
> +                                           "NULL", __func__);
> +#else
> +                                       log(LOG_ERR, "%s: Avoid an
> infinite "
> +                                           "loop: INP_TIMEWAIT &&
> (INP_FREED "
> +                                           "|| inp last reference) && tw
> != "
> +                                           "NULL", __func__);
> +#endif
> +                                       INP_INFO_RUNLOCK(&V_tcbinfo);
> +                                       break;
> +                               }
>                         }
>
>                         if (tw == NULL) {
>
>