Re: [PATCH v2 07/17] net: tcp: allow tcp_timewait_sock to validate skbs before handing to device

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Daniel Zahka wrote:
> Provide a callback to validate skb's originating from tcp timewait
> socks before passing to the device layer. Full socks have a
> sk_validate_xmit_skb member for checking that a device is capable of
> performing offloads required for transmitting an skb. With psp, tcp
> timewait socks will inherit the crypto state from their corresponding
> full socks. Any ACKs or RSTs that originate from a tcp timewait sock
> carrying psp state should be psp encapsulated.
> 
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
> ---
> 
> Notes:
>     v2:
>     - patch introduced in v2
> 
>  include/net/inet_timewait_sock.h |  5 +++++
>  net/core/dev.c                   | 14 ++++++++++++--
>  net/ipv4/inet_timewait_sock.c    |  3 +++
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/inet_timewait_sock.h b/include/net/inet_timewait_sock.h
> index c1295246216c..3a31c74c9e15 100644
> --- a/include/net/inet_timewait_sock.h
> +++ b/include/net/inet_timewait_sock.h
> @@ -84,6 +84,11 @@ struct inet_timewait_sock {
>  #if IS_ENABLED(CONFIG_INET_PSP)
>  	struct psp_assoc __rcu	  *psp_assoc;
>  #endif
> +#ifdef CONFIG_SOCK_VALIDATE_XMIT
> +	struct sk_buff*		(*tw_validate_xmit_skb)(struct sock *sk,
> +							struct net_device *dev,
> +							struct sk_buff *skb);
> +#endif
>  };
>  #define tw_tclass tw_tos
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b825b3f5b7db..bf013436a57b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3904,10 +3904,20 @@ static struct sk_buff *sk_validate_xmit_skb(struct sk_buff *skb,
>  					    struct net_device *dev)
>  {
>  #ifdef CONFIG_SOCK_VALIDATE_XMIT
> +	struct sk_buff *(*sk_validate)(struct sock *sk, struct net_device *dev,
> +				       struct sk_buff *skb);
>  	struct sock *sk = skb->sk;
>  
> -	if (sk && sk_fullsock(sk) && sk->sk_validate_xmit_skb) {
> -		skb = sk->sk_validate_xmit_skb(sk, dev, skb);
> +	sk_validate = NULL;
> +	if (sk) {
> +		if (sk_fullsock(sk))
> +			sk_validate = sk->sk_validate_xmit_skb;
> +		else if (sk->sk_state == TCP_TIME_WAIT)

Can this shadow a different constant for other protocols?

To be on the safe side: sk_is_tcp

> +			sk_validate = inet_twsk(sk)->tw_validate_xmit_skb;
> +	}
> +
> +	if (sk_validate) {
> +		skb = sk_validate(sk, dev, skb);

If this callback is in the hot path, a candidate for an INDIRECT_CALL wrapper.
There are only two users, TLS and PSP.

>  	} else if (unlikely(skb_is_decrypted(skb))) {
>  		pr_warn_ratelimited("unencrypted skb with no associated socket - dropping\n");
>  		kfree_skb(skb);
> diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c
> index dfde7895d8f2..859c03e07466 100644
> --- a/net/ipv4/inet_timewait_sock.c
> +++ b/net/ipv4/inet_timewait_sock.c
> @@ -210,6 +210,9 @@ struct inet_timewait_sock *inet_twsk_alloc(const struct sock *sk,
>  		atomic64_set(&tw->tw_cookie, atomic64_read(&sk->sk_cookie));
>  		twsk_net_set(tw, sock_net(sk));
>  		timer_setup(&tw->tw_timer, tw_timer_handler, 0);
> +#ifdef CONFIG_SOCK_VALIDATE_XMIT
> +		tw->tw_validate_xmit_skb = NULL;
> +#endif
>  		/*
>  		 * Because we use RCU lookups, we should not set tw_refcnt
>  		 * to a non null value before everything is setup for this
> -- 
> 2.47.1
>