linux-kernel - Re: [PATCH net-next 3/3] net: ipgre: add skb drop reasons to gre

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220315201706.464d5ecd@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 15 Mar 2022 20:17:06 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     menglong8.dong@...il.com
Cc:     dsahern@...nel.org, rostedt@...dmis.org, mingo@...hat.com,
        xeb@...l.ru, davem@...emloft.net, yoshfuji@...ux-ipv6.org,
        imagedong@...cent.com, edumazet@...gle.com, kafai@...com,
        talalahmad@...gle.com, keescook@...omium.org, alobakin@...me,
        flyingpeng@...cent.com, mengensun@...cent.com,
        dongli.zhang@...cle.com, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Biao Jiang <benbjiang@...cent.com>
Subject: Re: [PATCH net-next 3/3] net: ipgre: add skb drop reasons to
 gre_rcv()

On Mon, 14 Mar 2022 21:33:12 +0800 menglong8.dong@...il.com wrote:
> From: Menglong Dong <imagedong@...cent.com>
> 
> Replace kfree_skb() used in gre_rcv() with kfree_skb_reason(). With
> previous patch, we can tell that no tunnel device is found when
> PACKET_NEXT is returned by erspan_rcv() or ipgre_rcv().
> 
> In this commit, following new drop reasons are added:
> 
> SKB_DROP_REASON_GRE_CSUM
> SKB_DROP_REASON_GRE_NOTUNNEL
> 
> Reviewed-by: Hao Peng <flyingpeng@...cent.com>
> Reviewed-by: Biao Jiang <benbjiang@...cent.com>
> Signed-off-by: Menglong Dong <imagedong@...cent.com>
> ---
>  include/linux/skbuff.h     |  2 ++
>  include/trace/events/skb.h |  2 ++
>  net/ipv4/ip_gre.c          | 28 ++++++++++++++++++----------
>  3 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 5edb704af5bb..4f5e58e717ee 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -448,6 +448,8 @@ enum skb_drop_reason {
>  	SKB_DROP_REASON_GRE_NOHANDLER,	/* no handler found (version not
>  					 * supported?)
>  					 */
> +	SKB_DROP_REASON_GRE_CSUM,	/* GRE csum error */
> +	SKB_DROP_REASON_GRE_NOTUNNEL,	/* no tunnel device found */
>  	SKB_DROP_REASON_MAX,
>  };
>  
> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
> index f2bcffdc4bae..e8f95c96cf9d 100644
> --- a/include/trace/events/skb.h
> +++ b/include/trace/events/skb.h
> @@ -63,6 +63,8 @@
>  	EM(SKB_DROP_REASON_TAP_TXFILTER, TAP_TXFILTER)		\
>  	EM(SKB_DROP_REASON_GRE_VERSION, GRE_VERSION)		\
>  	EM(SKB_DROP_REASON_GRE_NOHANDLER, GRE_NOHANDLER)	\
> +	EM(SKB_DROP_REASON_GRE_CSUM, GRE_CSUM)			\
> +	EM(SKB_DROP_REASON_GRE_NOTUNNEL, GRE_NOTUNNEL)		\
>  	EMe(SKB_DROP_REASON_MAX, MAX)
>  
>  #undef EM
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index b1579d8374fd..b989239e4abc 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -421,9 +421,10 @@ static int ipgre_rcv(struct sk_buff *skb, const struct tnl_ptk_info *tpi,
>  
>  static int gre_rcv(struct sk_buff *skb)
>  {
> +	enum skb_drop_reason reason = SKB_DROP_REASON_NOT_SPECIFIED;
>  	struct tnl_ptk_info tpi;
>  	bool csum_err = false;
> -	int hdr_len;
> +	int hdr_len, ret;
>  
>  #ifdef CONFIG_NET_IPGRE_BROADCAST
>  	if (ipv4_is_multicast(ip_hdr(skb)->daddr)) {
> @@ -438,19 +439,26 @@ static int gre_rcv(struct sk_buff *skb)

I feel like gre_parse_header() is a good candidate for converting
to return a reason instead of errno.


>  		goto drop;
>  
>  	if (unlikely(tpi.proto == htons(ETH_P_ERSPAN) ||
> -		     tpi.proto == htons(ETH_P_ERSPAN2))) {
> -		if (erspan_rcv(skb, &tpi, hdr_len) == PACKET_RCVD)
> -			return 0;
> -		goto out;
> -	}
> +		     tpi.proto == htons(ETH_P_ERSPAN2)))
> +		ret = erspan_rcv(skb, &tpi, hdr_len);
> +	else
> +		ret = ipgre_rcv(skb, &tpi, hdr_len);

ipgre_rcv() OTOH may be better off taking the reason as an output
argument. Assuming PACKET_REJECT means NOMEM is a little fragile.

>  
> -	if (ipgre_rcv(skb, &tpi, hdr_len) == PACKET_RCVD)
> +	switch (ret) {
> +	case PACKET_NEXT:
> +		reason = SKB_DROP_REASON_GRE_NOTUNNEL;
> +		break;
> +	case PACKET_RCVD:
>  		return 0;
> -
> -out:
> +	case PACKET_REJECT:
> +		reason = SKB_DROP_REASON_NOMEM;
> +		break;
> +	}
>  	icmp_send(skb, ICMP_DEST_UNREACH, ICMP_PORT_UNREACH, 0);
>  drop:
> -	kfree_skb(skb);
> +	if (csum_err)
> +		reason = SKB_DROP_REASON_GRE_CSUM;
> +	kfree_skb_reason(skb, reason);
>  	return 0;
>  }
>