[PATCH net-next 0/3] net: gre: add skb drop reasons to gre packet receive

[PATCH net-next 0/3] net: gre: add skb drop reasons to gre packet receive

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

In the commit c504e5c2f964 ("net: skb: introduce kfree_skb_reason()"),
we added the support of reporting the reasons of skb drops to kfree_skb
tracepoint. And in this series patches, reasons for skb drops are added
to gre packet receiving path.

gre_rcv() in gre_demux.c is handled in the 1th patch.

In order to report skb drop reasons, we make erspan_rcv() return
PACKET_NEXT when no tunnel device found in the 2th patch. This may don't
correspond to 'PACKET_NEXT', but don't matter.

And gre_rcv() in ip_gre.c is handled in the 3th patch.

Following drop reasons are added(what they mean can be see in the
document for them):

SKB_DROP_REASON_GRE_VERSION
SKB_DROP_REASON_GRE_NOHANDLER
SKB_DROP_REASON_GRE_CSUM
SKB_DROP_REASON_GRE_NOTUNNEL

Maybe SKB_DROP_REASON_GRE_NOHANDLER can be replaced with
SKB_DROP_REASON_GRE_VERSION? As no gre_protocol found means that gre
version not supported.

PS: This set is parallel with the set "net: icmp: add skb drop reasons
to icmp", please don't mind :)

Menglong Dong (3):
  net: gre_demux: add skb drop reasons to gre_rcv()
  net: ipgre: make erspan_rcv() return PACKET_NEXT
  net: ipgre: add skb drop reasons to gre_rcv()

 include/linux/skbuff.h     |  6 ++++++
 include/trace/events/skb.h |  4 ++++
 net/ipv4/gre_demux.c       | 12 +++++++++---
 net/ipv4/ip_gre.c          | 30 +++++++++++++++++++-----------
 4 files changed, 38 insertions(+), 14 deletions(-)

-- 
2.35.1

[PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() used in gre_rcv() with kfree_skb_reason(). Following
new drop reasons are added:

SKB_DROP_REASON_GRE_VERSION
SKB_DROP_REASON_GRE_NOHANDLER

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     |  4 ++++
 include/trace/events/skb.h |  2 ++
 net/ipv4/gre_demux.c       | 12 +++++++++---
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 26538ceb4b01..5edb704af5bb 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -444,6 +444,10 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_TAP_TXFILTER,	/* dropped by tx filter implemented
 					 * at tun/tap, e.g., check_filter()
 					 */
+	SKB_DROP_REASON_GRE_VERSION,	/* GRE version not supported */
+	SKB_DROP_REASON_GRE_NOHANDLER,	/* no handler found (version not
+					 * supported?)
+					 */
 	SKB_DROP_REASON_MAX,
 };
 
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index e1670e1e4934..f2bcffdc4bae 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -61,6 +61,8 @@
 	EM(SKB_DROP_REASON_HDR_TRUNC, HDR_TRUNC)		\
 	EM(SKB_DROP_REASON_TAP_FILTER, TAP_FILTER)		\
 	EM(SKB_DROP_REASON_TAP_TXFILTER, TAP_TXFILTER)		\
+	EM(SKB_DROP_REASON_GRE_VERSION, GRE_VERSION)		\
+	EM(SKB_DROP_REASON_GRE_NOHANDLER, GRE_NOHANDLER)	\
 	EMe(SKB_DROP_REASON_MAX, MAX)
 
 #undef EM
diff --git a/net/ipv4/gre_demux.c b/net/ipv4/gre_demux.c
index cbb2b4bb0dfa..066cbaadc52a 100644
--- a/net/ipv4/gre_demux.c
+++ b/net/ipv4/gre_demux.c
@@ -146,20 +146,26 @@ EXPORT_SYMBOL(gre_parse_header);
 static int gre_rcv(struct sk_buff *skb)
 {
 	const struct gre_protocol *proto;
+	enum skb_drop_reason reason;
 	u8 ver;
 	int ret;
 
+	reason = SKB_DROP_REASON_NOT_SPECIFIED;
 	if (!pskb_may_pull(skb, 12))
 		goto drop;
 
 	ver = skb->data[1]&0x7f;
-	if (ver >= GREPROTO_MAX)
+	if (ver >= GREPROTO_MAX) {
+		reason = SKB_DROP_REASON_GRE_VERSION;
 		goto drop;
+	}
 
 	rcu_read_lock();
 	proto = rcu_dereference(gre_proto[ver]);
-	if (!proto || !proto->handler)
+	if (!proto || !proto->handler) {
+		reason = SKB_DROP_REASON_GRE_NOHANDLER;
 		goto drop_unlock;
+	}
 	ret = proto->handler(skb);
 	rcu_read_unlock();
 	return ret;
@@ -167,7 +173,7 @@ static int gre_rcv(struct sk_buff *skb)
 drop_unlock:
 	rcu_read_unlock();
 drop:
-	kfree_skb(skb);
+	kfree_skb_reason(skb, reason);
 	return NET_RX_DROP;
 }
 
-- 
2.35.1

[PATCH net-next 2/3] net: ipgre: make erspan_rcv() return PACKET_NEXT

[menglong8.dong]

From: Menglong Dong <imagedong@tencent.com>

PACKET_NEXT is returned in ipgre_rcv() when no tunnel device found. To
report skb drop reasons, we make erspan_rcv() the same way. Therefore,
we can know that skb is dropped out of tunnel device's absence when
PACKET_NEXT is returned.

Reviewed-by: Hao Peng <flyingpeng@tencent.com>
Reviewed-by: Biao Jiang <benbjiang@tencent.com>
Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 net/ipv4/ip_gre.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 99db2e41ed10..b1579d8374fd 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -341,7 +341,7 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 		ip_tunnel_rcv(tunnel, skb, tpi, tun_dst, log_ecn_error);
 		return PACKET_RCVD;
 	}
-	return PACKET_REJECT;
+	return PACKET_NEXT;
 
 drop:
 	kfree_skb(skb);
-- 
2.35.1

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

[menglong8.dong]

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

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[Jakub Kicinski]

On Mon, 14 Mar 2022 21:33:10 +0800 menglong8.dong@gmail.com wrote:
> +	reason = SKB_DROP_REASON_NOT_SPECIFIED;
>  	if (!pskb_may_pull(skb, 12))
>  		goto drop;

REASON_HDR_TRUNC ?

>  	ver = skb->data[1]&0x7f;
> -	if (ver >= GREPROTO_MAX)
> +	if (ver >= GREPROTO_MAX) {
> +		reason = SKB_DROP_REASON_GRE_VERSION;

TBH I'm still not sure what level of granularity we should be shooting
for with the reasons. I'd throw all unexpected header values into one 
bucket, not go for a reason per field, per protocol. But as I'm said
I'm not sure myself, so we can keep what you have..

>  		goto drop;
> +	}
>  
>  	rcu_read_lock();
>  	proto = rcu_dereference(gre_proto[ver]);
> -	if (!proto || !proto->handler)
> +	if (!proto || !proto->handler) {
> +		reason = SKB_DROP_REASON_GRE_NOHANDLER;

I think the ->handler check is defensive programming, there's no
protocol upstream which would leave handler NULL.

This is akin to SKB_DROP_REASON_PTYPE_ABSENT, we can reuse that or add
a new reason, but I'd think the phrasing should be kept similar.

>  		goto drop_unlock;
> +	}
>  	ret = proto->handler(skb);

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

[Jakub Kicinski]

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

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[David Ahern]

On 3/15/22 9:08 PM, Jakub Kicinski wrote:
> On Mon, 14 Mar 2022 21:33:10 +0800 menglong8.dong@gmail.com wrote:
>> +	reason = SKB_DROP_REASON_NOT_SPECIFIED;
>>  	if (!pskb_may_pull(skb, 12))
>>  		goto drop;
> 
> REASON_HDR_TRUNC ?
> 
>>  	ver = skb->data[1]&0x7f;
>> -	if (ver >= GREPROTO_MAX)
>> +	if (ver >= GREPROTO_MAX) {
>> +		reason = SKB_DROP_REASON_GRE_VERSION;
> 
> TBH I'm still not sure what level of granularity we should be shooting
> for with the reasons. I'd throw all unexpected header values into one 
> bucket, not go for a reason per field, per protocol. But as I'm said
> I'm not sure myself, so we can keep what you have..

I have stated before I do not believe every single drop point in the
kernel needs a unique reason code. This is overkill. The reason augments
information we already have -- the IP from kfree_skb tracepoint.

> 
>>  		goto drop;
>> +	}
>>  
>>  	rcu_read_lock();
>>  	proto = rcu_dereference(gre_proto[ver]);
>> -	if (!proto || !proto->handler)
>> +	if (!proto || !proto->handler) {
>> +		reason = SKB_DROP_REASON_GRE_NOHANDLER;
> 
> I think the ->handler check is defensive programming, there's no
> protocol upstream which would leave handler NULL.
> 
> This is akin to SKB_DROP_REASON_PTYPE_ABSENT, we can reuse that or add
> a new reason, but I'd think the phrasing should be kept similar.
> 
>>  		goto drop_unlock;
>> +	}
>>  	ret = proto->handler(skb);

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[Menglong Dong]

On Wed, Mar 16, 2022 at 11:08 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 14 Mar 2022 21:33:10 +0800 menglong8.dong@gmail.com wrote:
> > +     reason = SKB_DROP_REASON_NOT_SPECIFIED;
> >       if (!pskb_may_pull(skb, 12))
> >               goto drop;
>
> REASON_HDR_TRUNC ?

I'm still not sure about such a 'pskb_pull' failure, whose reasons may be
complex, such as no memory or packet length too small. I see somewhere
return a '-NOMEM' when skb pull fails.

So maybe such cases can be ignored? In my opinion, not all skb drops
need a reason.


>
> >       ver = skb->data[1]&0x7f;
> > -     if (ver >= GREPROTO_MAX)
> > +     if (ver >= GREPROTO_MAX) {
> > +             reason = SKB_DROP_REASON_GRE_VERSION;
>
> TBH I'm still not sure what level of granularity we should be shooting
> for with the reasons. I'd throw all unexpected header values into one
> bucket, not go for a reason per field, per protocol. But as I'm said
> I'm not sure myself, so we can keep what you have..
>
> >               goto drop;
> > +     }
> >
> >       rcu_read_lock();
> >       proto = rcu_dereference(gre_proto[ver]);
> > -     if (!proto || !proto->handler)
> > +     if (!proto || !proto->handler) {
> > +             reason = SKB_DROP_REASON_GRE_NOHANDLER;
>
> I think the ->handler check is defensive programming, there's no
> protocol upstream which would leave handler NULL.
>
> This is akin to SKB_DROP_REASON_PTYPE_ABSENT, we can reuse that or add
> a new reason, but I'd think the phrasing should be kept similar.

With the handler not NULL, does it mean the gre version is not supported here,
and this 'SKB_DROP_REASON_GRE_NOHANDLER' can be replaced with
SKB_DROP_REASON_GRE_VERSION above?

>
> >               goto drop_unlock;
> > +     }
> >       ret = proto->handler(skb);

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[Jakub Kicinski]

On Wed, 16 Mar 2022 12:41:45 +0800 Menglong Dong wrote:
> > On Mon, 14 Mar 2022 21:33:10 +0800 menglong8.dong@gmail.com wrote:  
> > > +     reason = SKB_DROP_REASON_NOT_SPECIFIED;
> > >       if (!pskb_may_pull(skb, 12))
> > >               goto drop;  
> >
> > REASON_HDR_TRUNC ?  
> 
> I'm still not sure about such a 'pskb_pull' failure, whose reasons may be
> complex, such as no memory or packet length too small. I see somewhere
> return a '-NOMEM' when skb pull fails.
> 
> So maybe such cases can be ignored? In my opinion, not all skb drops
> need a reason.

Ah, okay, I saw Dave's email as well. I wasn't sure if this omission
was intentional. But if it is, that's fine.

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[Menglong Dong]

On Wed, Mar 16, 2022 at 11:49 AM David Ahern <dsahern@kernel.org> wrote:
>
> On 3/15/22 9:08 PM, Jakub Kicinski wrote:
> > On Mon, 14 Mar 2022 21:33:10 +0800 menglong8.dong@gmail.com wrote:
> >> +    reason = SKB_DROP_REASON_NOT_SPECIFIED;
> >>      if (!pskb_may_pull(skb, 12))
> >>              goto drop;
> >
> > REASON_HDR_TRUNC ?
> >
> >>      ver = skb->data[1]&0x7f;
> >> -    if (ver >= GREPROTO_MAX)
> >> +    if (ver >= GREPROTO_MAX) {
> >> +            reason = SKB_DROP_REASON_GRE_VERSION;
> >
> > TBH I'm still not sure what level of granularity we should be shooting
> > for with the reasons. I'd throw all unexpected header values into one
> > bucket, not go for a reason per field, per protocol. But as I'm said
> > I'm not sure myself, so we can keep what you have..
>
> I have stated before I do not believe every single drop point in the
> kernel needs a unique reason code. This is overkill. The reason augments
> information we already have -- the IP from kfree_skb tracepoint.

Is this reason unnecessary? I'm not sure if the GRE version problem should
be reported. With versions not supported by the kernel, it seems we
can't get the
drop reason from the packet data, as they are fine. And previous seems not
suitable here, as it is a L4 problem.

I'll remove the reason here if there is no positive reply.

Thanks!
Menglong Dong
>
> >
> >>              goto drop;
> >> +    }
> >>
> >>      rcu_read_lock();
> >>      proto = rcu_dereference(gre_proto[ver]);
> >> -    if (!proto || !proto->handler)
> >> +    if (!proto || !proto->handler) {
> >> +            reason = SKB_DROP_REASON_GRE_NOHANDLER;
> >
> > I think the ->handler check is defensive programming, there's no
> > protocol upstream which would leave handler NULL.
> >
> > This is akin to SKB_DROP_REASON_PTYPE_ABSENT, we can reuse that or add
> > a new reason, but I'd think the phrasing should be kept similar.
> >
> >>              goto drop_unlock;
> >> +    }
> >>      ret = proto->handler(skb);
>

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[Jakub Kicinski]

On Tue, 15 Mar 2022 21:49:01 -0600 David Ahern wrote:
> >>  	ver = skb->data[1]&0x7f;
> >> -	if (ver >= GREPROTO_MAX)
> >> +	if (ver >= GREPROTO_MAX) {
> >> +		reason = SKB_DROP_REASON_GRE_VERSION;  
> > 
> > TBH I'm still not sure what level of granularity we should be shooting
> > for with the reasons. I'd throw all unexpected header values into one 
> > bucket, not go for a reason per field, per protocol. But as I'm said
> > I'm not sure myself, so we can keep what you have..  
> 
> I have stated before I do not believe every single drop point in the
> kernel needs a unique reason code. This is overkill. The reason augments
> information we already have -- the IP from kfree_skb tracepoint.

That's certainly true. I wonder if there is a systematic way of
approaching these additions that'd help us picking the points were 
we add reasons less of a judgment call.

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

[Menglong Dong]

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

RE: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[David Laight]

From: Jakub Kicinski
> Sent: 16 March 2022 04:56
> 
> On Tue, 15 Mar 2022 21:49:01 -0600 David Ahern wrote:
> > >>  	ver = skb->data[1]&0x7f;
> > >> -	if (ver >= GREPROTO_MAX)
> > >> +	if (ver >= GREPROTO_MAX) {
> > >> +		reason = SKB_DROP_REASON_GRE_VERSION;
> > >
> > > TBH I'm still not sure what level of granularity we should be shooting
> > > for with the reasons. I'd throw all unexpected header values into one
> > > bucket, not go for a reason per field, per protocol. But as I'm said
> > > I'm not sure myself, so we can keep what you have..
> >
> > I have stated before I do not believe every single drop point in the
> > kernel needs a unique reason code. This is overkill. The reason augments
> > information we already have -- the IP from kfree_skb tracepoint.
> 
> That's certainly true. I wonder if there is a systematic way of
> approaching these additions that'd help us picking the points were
> we add reasons less of a judgment call.

Is it worth considering splitting the 'reason' into two parts?
Eg x << 16 | y
One part being the overall reason - and probably a define.
The other qualifying the actual failing test and probably just
being a number.

Then you get an overall view of the fails (which might even
be counted) while still being able to locate the actual
failing test.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[David Ahern]

On 3/15/22 10:53 PM, Menglong Dong wrote:
> On Wed, Mar 16, 2022 at 11:49 AM David Ahern <dsahern@kernel.org> wrote:
>>
>> On 3/15/22 9:08 PM, Jakub Kicinski wrote:
>>> On Mon, 14 Mar 2022 21:33:10 +0800 menglong8.dong@gmail.com wrote:
>>>> +    reason = SKB_DROP_REASON_NOT_SPECIFIED;
>>>>      if (!pskb_may_pull(skb, 12))
>>>>              goto drop;
>>>
>>> REASON_HDR_TRUNC ?
>>>
>>>>      ver = skb->data[1]&0x7f;
>>>> -    if (ver >= GREPROTO_MAX)
>>>> +    if (ver >= GREPROTO_MAX) {
>>>> +            reason = SKB_DROP_REASON_GRE_VERSION;
>>>
>>> TBH I'm still not sure what level of granularity we should be shooting
>>> for with the reasons. I'd throw all unexpected header values into one
>>> bucket, not go for a reason per field, per protocol. But as I'm said
>>> I'm not sure myself, so we can keep what you have..
>>
>> I have stated before I do not believe every single drop point in the
>> kernel needs a unique reason code. This is overkill. The reason augments
>> information we already have -- the IP from kfree_skb tracepoint.
> 
> Is this reason unnecessary? I'm not sure if the GRE version problem should
> be reported. With versions not supported by the kernel, it seems we
> can't get the
> drop reason from the packet data, as they are fine. And previous seems not
> suitable here, as it is a L4 problem.
> 

Generically, it is "no support for a protocol version". That kind of
reason code + the IP tells you GRE is processing a packet with an
unsupported protocol version.

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[David Ahern]

On 3/15/22 10:55 PM, Jakub Kicinski wrote:
> On Tue, 15 Mar 2022 21:49:01 -0600 David Ahern wrote:
>>>>  	ver = skb->data[1]&0x7f;
>>>> -	if (ver >= GREPROTO_MAX)
>>>> +	if (ver >= GREPROTO_MAX) {
>>>> +		reason = SKB_DROP_REASON_GRE_VERSION;  
>>>
>>> TBH I'm still not sure what level of granularity we should be shooting
>>> for with the reasons. I'd throw all unexpected header values into one 
>>> bucket, not go for a reason per field, per protocol. But as I'm said
>>> I'm not sure myself, so we can keep what you have..  
>>
>> I have stated before I do not believe every single drop point in the
>> kernel needs a unique reason code. This is overkill. The reason augments
>> information we already have -- the IP from kfree_skb tracepoint.
> 
> That's certainly true. I wonder if there is a systematic way of
> approaching these additions that'd help us picking the points were 
> we add reasons less of a judgment call.

In my head it's split between OS housekeeping and user visible data.
Housekeeping side of it is more the technical failure points like skb
manipulations - maybe interesting to a user collecting stats about how a
node is performing, but more than likely not. IMHO, those are ignored
for now (NOT_SPECIFIED).

The immediate big win is for packets from a network where an analysis
can show code location (instruction pointer), user focused reason (csum
failure, 'otherhost', no socket open, no socket buffer space, ...) and
traceable to a specific host (headers in skb data).

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

[Jakub Kicinski]

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.

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

[Jakub Kicinski]

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.

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[Jakub Kicinski]

On Wed, 16 Mar 2022 10:12:00 +0000 David Laight wrote:
> Is it worth considering splitting the 'reason' into two parts?
> Eg x << 16 | y
> One part being the overall reason - and probably a define.
> The other qualifying the actual failing test and probably just
> being a number.
> 
> Then you get an overall view of the fails (which might even
> be counted) while still being able to locate the actual
> failing test.

That popped to my mind, but other than the fact that it "seems fine" 
I can't really convince myself that (a) 2 levels are enough, why not 3;
(b) I personally don't often look at the drops, so IDK what'd fit the
needs of the consumer of this API. TCP bad csum drops are the only ones
I have experience with, and we have those already covered.

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[Jakub Kicinski]

On Wed, 16 Mar 2022 08:56:14 -0600 David Ahern wrote:
> > That's certainly true. I wonder if there is a systematic way of
> > approaching these additions that'd help us picking the points were 
> > we add reasons less of a judgment call.  
> 
> In my head it's split between OS housekeeping and user visible data.
> Housekeeping side of it is more the technical failure points like skb
> manipulations - maybe interesting to a user collecting stats about how a
> node is performing, but more than likely not. IMHO, those are ignored
> for now (NOT_SPECIFIED).
> 
> The immediate big win is for packets from a network where an analysis
> can show code location (instruction pointer), user focused reason (csum
> failure, 'otherhost', no socket open, no socket buffer space, ...) and
> traceable to a specific host (headers in skb data).

Maybe I'm oversimplifying but would that mean first order of business
is to have drop codes for where we already bump MIB exception stats?

Re: [PATCH net-next 1/3] net: gre_demux: add skb drop reasons to gre_rcv()

[David Ahern]

On 3/16/22 12:57 PM, Jakub Kicinski wrote:
> On Wed, 16 Mar 2022 08:56:14 -0600 David Ahern wrote:
>>> That's certainly true. I wonder if there is a systematic way of
>>> approaching these additions that'd help us picking the points were 
>>> we add reasons less of a judgment call.  
>>
>> In my head it's split between OS housekeeping and user visible data.
>> Housekeeping side of it is more the technical failure points like skb
>> manipulations - maybe interesting to a user collecting stats about how a
>> node is performing, but more than likely not. IMHO, those are ignored
>> for now (NOT_SPECIFIED).
>>
>> The immediate big win is for packets from a network where an analysis
>> can show code location (instruction pointer), user focused reason (csum
>> failure, 'otherhost', no socket open, no socket buffer space, ...) and
>> traceable to a specific host (headers in skb data).
> 
> Maybe I'm oversimplifying but would that mean first order of business
> is to have drop codes for where we already bump MIB exception stats?

That was the original motivation and it has since spun out of control -
walking the code base and assigning unique drop reasons for every single
call to kfree_skb.