Re: git: a730d82378d3 - main - tcp: fix RACK and BBR when using VIMAGE enabled kernel

On Tue, 20 Jul 2021 10:29:30 +0930
Daniel O'Connor via freebsd-hackers <freebsd-hackers@freebsd.org> wrote:

> > On 20 Jul 2021, at 08:03, Michael Tuexen <tuexen@freebsd.org> wrote:
> > 
> > The branch main has been updated by tuexen:
> > 
> > URL:
> > https://cgit.FreeBSD.org/src/commit/?id=a730d82378d3cdf5356775ec0c23ad2ca40c5edb
> > 
> > diff --git a/sys/netinet/tcp_stacks/rack_bbr_common.c
> > b/sys/netinet/tcp_stacks/rack_bbr_common.c index
> > baa267b43752..bf93359368f9 100644 ---
> > a/sys/netinet/tcp_stacks/rack_bbr_common.c +++
> > b/sys/netinet/tcp_stacks/rack_bbr_common.c @@ -508,16 +508,18 @@
> > skip_vnet: m_freem(m);
> > 				m = m_save;
> > 			}
> > -			if (no_vn == 0)
> > +			if (no_vn == 0) {
> > 				CURVNET_RESTORE();
> > +			}
> > 			INP_UNLOCK_ASSERT(inp);
> > 			return(retval);
> > 		}
> > skipped_pkt:
> > 		m = m_save;
> > 	}
> > -	if (no_vn == 0)
> > +	if (no_vn == 0) {
> > 		CURVNET_RESTORE();
> > +	}
> > 	return(retval);
> > }  
> 
> Not to pick on this particular commit, but does anyone know why
> CURVNET_RESTORE os not defined such that it doesn't require wrapping
> with {}?

True, the body of CURVNET_RESTORE() macro could be redefined so that it
gets enclosed in a do {...} while (0) block, most probably without any
ill side-effects.

One reason it was never defined in such manner is that CURVNET_ macros
were never intended to be invoked conditionally, which is also clearly
documented in VNET(9) . The other reason is my clumsiness...

> Looking through vnet.h I see that VNET_ASSERT,
> VNET_GLOBAL_EVENTHANDLER_REGISTER_TAG, and
> VNET_GLOBAL_EVENTHANDLER_REGISTER have a do { } while(0) wrapper but
> CURVNET_SET_QUIET, CURVNET_SET_VERBOSE, CURVNET_RESTORE,
> VNET_SYSINIT, and VNET_SYSUNINIT don't.

CURVNET_SET macros cannot be wrapped in a separate block because they
declare a local variable which must be visible / available to the
corresponding CURVNET_RESTORE(s).

In this particular case, i.e. inside ctf_process_inbound_raw(), the
rule that CURVNET_SET() must not be conditionally called seems to be
circumvented by a local hack, in an attempt to handle a case when
VNET cannot be inferred from m->m_pkthdr->rcvif->if_vnet, when m ==
NULL.

Given that a non-NULL struct socket *so seem to be available as an
argument to ctf_process_inbound_raw(), perhaps it could be possible to
remove the if (no_vn == 0) hack, and instead to unconditionally invoke
CURVNET_SET(so->so_vnet) on entry to ctf_process_inbound_raw()?

Marko