diff mbox series

[net-next,3/3] net: ipgre: add skb drop reasons to gre_rcv()

Message ID 20220314133312.336653-4-imagedong@tencent.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: gre: add skb drop reasons to gre packet receive | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5920 this patch: 5920
netdev/cc_maintainers success CCed 11 of 11 maintainers
netdev/build_clang success Errors and warnings before: 880 this patch: 880
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 6071 this patch: 6071
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 62 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Menglong Dong March 14, 2022, 1:33 p.m. UTC 
From: Menglong Dong <imagedong@tencent.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@tencent.com>
Reviewed-by: Biao Jiang <benbjiang@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.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(-)

Comments

Jakub Kicinski March 16, 2022, 3:17 a.m. UTC | #1 
On Mon, 14 Mar 2022 21:33:12 +0800 menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.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@tencent.com>
> Reviewed-by: Biao Jiang <benbjiang@tencent.com>
> Signed-off-by: Menglong Dong <imagedong@tencent.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;
>  }
>
Menglong Dong March 16, 2022, 6:21 a.m. UTC | #2 
On Wed, Mar 16, 2022 at 11:17 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 14 Mar 2022 21:33:12 +0800 menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.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@tencent.com>
> > Reviewed-by: Biao Jiang <benbjiang@tencent.com>
> > Signed-off-by: Menglong Dong <imagedong@tencent.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.
>

Enn...isn't gre_parse_header() returning the header length? And I
didn't find much useful reason in this function.

>
> >               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.

Yeah, it seems not friendly. I think it's ok to ignore such 'NOMEM' reasons?
Therefore, we only need to consider the PACKET_NEXT return value, and
keep ipgre_rcv() still.

>
> >
> > -     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;
> >  }
> >
>
Jakub Kicinski March 16, 2022, 6:50 p.m. UTC | #3 
On Wed, 16 Mar 2022 14:21:24 +0800 Menglong Dong wrote:
> > I feel like gre_parse_header() is a good candidate for converting
> > to return a reason instead of errno.
> 
> Enn...isn't gre_parse_header() returning the header length? And I
> didn't find much useful reason in this function.

Ah, you're right, it returns negative error or hdr_len.
We'd need to make the reason negative, I guess that's not pretty.

What made me wonder is that it already takes a boolean for csum error
and callers don't care _which_ error gets returned. 

We can replace the csum_err output param with reason code, then?

> > >               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.  
> 
> Yeah, it seems not friendly. I think it's ok to ignore such 'NOMEM' reasons?
> Therefore, we only need to consider the PACKET_NEXT return value, and
> keep ipgre_rcv() still.
Jakub Kicinski March 16, 2022, 6:51 p.m. UTC | #4 
On Wed, 16 Mar 2022 14:21:24 +0800 Menglong Dong wrote:
> > ipgre_rcv() OTOH may be better off taking the reason as an output
> > argument. Assuming PACKET_REJECT means NOMEM is a little fragile.  
> 
> Yeah, it seems not friendly. I think it's ok to ignore such 'NOMEM' reasons?
> Therefore, we only need to consider the PACKET_NEXT return value, and
> keep ipgre_rcv() still.

SGTM.
diff mbox series

Patch

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)
 		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);
 
-	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;
 }