Re: git: 220ee18f1964 - main - netinet/tcp_var.h: always define IS_FASTOPEN() for kernel compilation env

On 3/13/24 5:24 PM, Gleb Smirnoff wrote:
> On Wed, Mar 13, 2024 at 11:21:23PM +0000, Konstantin Belousov wrote:
> K> diff --git a/sys/netinet/tcp_var.h b/sys/netinet/tcp_var.h
> K> index 6f7f7115c2f4..7b5c57d39213 100644
> K> --- a/sys/netinet/tcp_var.h
> K> +++ b/sys/netinet/tcp_var.h
> K> @@ -812,11 +812,13 @@ tcp_packets_this_ack(struct tcpcb *tp, tcp_seq ack)
> K>  #define	ENTER_RECOVERY(t_flags) t_flags |= (TF_CONGRECOVERY | TF_FASTRECOVERY)
> K>  #define	EXIT_RECOVERY(t_flags) t_flags &= ~(TF_CONGRECOVERY | TF_FASTRECOVERY)
> K>
> K> -#if defined(_KERNEL) && !defined(TCP_RFC7413)
> K> +#if defined(_KERNEL)
> K> +#if !defined(TCP_RFC7413)
> K>  #define	IS_FASTOPEN(t_flags)		(false)
> K>  #else
> K>  #define	IS_FASTOPEN(t_flags)		(t_flags & TF_FASTOPEN)
> K>  #endif
> K> +#endif
> K>
> K>  #define	BYTES_THIS_ACK(tp, th)	(th->th_ack - tp->snd_una)
> 
> I know Konstantin in doing that to clear path for IPSEC changes, and the patch
> does improve code.  So the message isn't addressed to him, rather it is for
> other src-committers.
> 
> Using ifdefs that come from the kernel config inside include files that are not
> ephemeral opt_foo.h is laying out a minefield for the future. Best case - for
> yourself, worst case - for somebody else.

+100

> In the best case this ends in cryptic kernel failure builds, where your custom
> kernel doesn't build, but GENERIC builds and it is not clear why. In the worst
> case this creates runtime failures even more cryptic in their nature.
> 
> In this particular case TCP_RFC7413 comes via opt_inet.h.  Thus, if you got a
> userland tool, e.g. netstat(1) using IS_FASTOPEN() it will always be false.
> Again, netstat would be a best case.  A worst case would be netinet6 kernel
> compilation unit that does not include opt_inet.h, but uses IS_FASTOPEN().
> 
> Other than that, we got 32 flags for t_flags and only one is obfuscated via a
> macro.  I'd really like to remove the macro and test the flag directly.  Any
> objections?

+1

-- 
John Baldwin