diff mbox series

[net-next,8/9] net: ipv6: add skb drop reasons to ip6_rcv_core()

Message ID 20220413081600.187339-9-imagedong@tencent.com (mailing list archive)
State Accepted
Commit 4daf841a2ef3b2e987894c8107d309ce2b67c202
Delegated to: Netdev Maintainers
Headers show
Series net: ip: add skb drop reasons to ip ingress | 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: 1 this patch: 1
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
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: 1 this patch: 1
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 75 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 April 13, 2022, 8:15 a.m. UTC 
From: Menglong Dong <imagedong@tencent.com>

Replace kfree_skb() used in ip6_rcv_core() with kfree_skb_reason().
No new drop reasons are added.

Seems now we use 'SKB_DROP_REASON_IP_INHDR' for too many case during
ipv6 header parse or check, just like what 'IPSTATS_MIB_INHDRERRORS'
do. Will it be too general and hard to know what happened?

Signed-off-by: Menglong Dong <imagedong@tencent.com>
Reviewed-by: Jiang Biao <benbjiang@tencent.com>
Reviewed-by: Hao Peng <flyingpeng@tencent.com>
---
 net/ipv6/ip6_input.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Eric Dumazet April 13, 2022, 8:40 p.m. UTC | #1 
On 4/13/22 01:15, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
>
> Replace kfree_skb() used in ip6_rcv_core() with kfree_skb_reason().
> No new drop reasons are added.
>
> Seems now we use 'SKB_DROP_REASON_IP_INHDR' for too many case during
> ipv6 header parse or check, just like what 'IPSTATS_MIB_INHDRERRORS'
> do. Will it be too general and hard to know what happened?
>
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> Reviewed-by: Hao Peng <flyingpeng@tencent.com>
> ---
>   net/ipv6/ip6_input.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> index b4880c7c84eb..1b925ecb26e9 100644
> --- a/net/ipv6/ip6_input.c
> +++ b/net/ipv6/ip6_input.c
> @@ -145,13 +145,14 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
>   static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>   				    struct net *net)
>   {
> +	enum skb_drop_reason reason;
>   	const struct ipv6hdr *hdr;
>   	u32 pkt_len;
>   	struct inet6_dev *idev;
>   
>   	if (skb->pkt_type == PACKET_OTHERHOST) {
>   		dev_core_stats_rx_otherhost_dropped_inc(skb->dev);
> -		kfree_skb(skb);
> +		kfree_skb_reason(skb, SKB_DROP_REASON_OTHERHOST);
>   		return NULL;
>   	}
>   
> @@ -161,9 +162,12 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>   
>   	__IP6_UPD_PO_STATS(net, idev, IPSTATS_MIB_IN, skb->len);
>   
> +	SKB_DR_SET(reason, NOT_SPECIFIED);
>   	if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL ||
>   	    !idev || unlikely(idev->cnf.disable_ipv6)) {
>   		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INDISCARDS);
> +		if (unlikely(idev->cnf.disable_ipv6))

idev can be NULL here (according to surrounding code), and we crash :/



> +			SKB_DR_SET(reason, IPV6DISABLED);
>   		goto drop;
>   	}
>   
> @@ -187,8 +191,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>   
>   	hdr = ipv6_hdr(skb);
>   
> -	if (hdr->version != 6)
> +	if (hdr->version != 6) {
> +		SKB_DR_SET(reason, UNHANDLED_PROTO);
>   		goto err;
> +	}
>   
>   	__IP6_ADD_STATS(net, idev,
>   			IPSTATS_MIB_NOECTPKTS +
> @@ -226,8 +232,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>   	if (!ipv6_addr_is_multicast(&hdr->daddr) &&
>   	    (skb->pkt_type == PACKET_BROADCAST ||
>   	     skb->pkt_type == PACKET_MULTICAST) &&
> -	    idev->cnf.drop_unicast_in_l2_multicast)
> +	    idev->cnf.drop_unicast_in_l2_multicast) {
> +		SKB_DR_SET(reason, UNICAST_IN_L2_MULTICAST);
>   		goto err;
> +	}
>   
>   	/* RFC4291 2.7
>   	 * Nodes must not originate a packet to a multicast address whose scope
> @@ -256,12 +264,11 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>   		if (pkt_len + sizeof(struct ipv6hdr) > skb->len) {
>   			__IP6_INC_STATS(net,
>   					idev, IPSTATS_MIB_INTRUNCATEDPKTS);
> +			SKB_DR_SET(reason, PKT_TOO_SMALL);
>   			goto drop;
>   		}
> -		if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr))) {
> -			__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> -			goto drop;
> -		}
> +		if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr)))
> +			goto err;
>   		hdr = ipv6_hdr(skb);
>   	}
>   
> @@ -282,9 +289,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
>   	return skb;
>   err:
>   	__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> +	SKB_DR_OR(reason, IP_INHDR);
>   drop:
>   	rcu_read_unlock();
> -	kfree_skb(skb);
> +	kfree_skb_reason(skb, reason);
>   	return NULL;
>   }
>
Menglong Dong April 14, 2022, 1:20 a.m. UTC | #2 
On Thu, Apr 14, 2022 at 4:41 AM Eric Dumazet <edumazet@gmail.com> wrote:
>
>
> On 4/13/22 01:15, menglong8.dong@gmail.com wrote:
> > From: Menglong Dong <imagedong@tencent.com>
> >
> > Replace kfree_skb() used in ip6_rcv_core() with kfree_skb_reason().
> > No new drop reasons are added.
> >
> > Seems now we use 'SKB_DROP_REASON_IP_INHDR' for too many case during
> > ipv6 header parse or check, just like what 'IPSTATS_MIB_INHDRERRORS'
> > do. Will it be too general and hard to know what happened?
> >
> > Signed-off-by: Menglong Dong <imagedong@tencent.com>
> > Reviewed-by: Jiang Biao <benbjiang@tencent.com>
> > Reviewed-by: Hao Peng <flyingpeng@tencent.com>
> > ---
> >   net/ipv6/ip6_input.c | 24 ++++++++++++++++--------
> >   1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
> > index b4880c7c84eb..1b925ecb26e9 100644
> > --- a/net/ipv6/ip6_input.c
> > +++ b/net/ipv6/ip6_input.c
> > @@ -145,13 +145,14 @@ static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
> >   static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >                                   struct net *net)
> >   {
> > +     enum skb_drop_reason reason;
> >       const struct ipv6hdr *hdr;
> >       u32 pkt_len;
> >       struct inet6_dev *idev;
> >
> >       if (skb->pkt_type == PACKET_OTHERHOST) {
> >               dev_core_stats_rx_otherhost_dropped_inc(skb->dev);
> > -             kfree_skb(skb);
> > +             kfree_skb_reason(skb, SKB_DROP_REASON_OTHERHOST);
> >               return NULL;
> >       }
> >
> > @@ -161,9 +162,12 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >
> >       __IP6_UPD_PO_STATS(net, idev, IPSTATS_MIB_IN, skb->len);
> >
> > +     SKB_DR_SET(reason, NOT_SPECIFIED);
> >       if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL ||
> >           !idev || unlikely(idev->cnf.disable_ipv6)) {
> >               __IP6_INC_STATS(net, idev, IPSTATS_MIB_INDISCARDS);
> > +             if (unlikely(idev->cnf.disable_ipv6))
>
> idev can be NULL here (according to surrounding code), and we crash :/

Yeah, you are right :)

Thanks for your fix!

>
>
>
> > +                     SKB_DR_SET(reason, IPV6DISABLED);
> >               goto drop;
> >       }
> >
> > @@ -187,8 +191,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >
> >       hdr = ipv6_hdr(skb);
> >
> > -     if (hdr->version != 6)
> > +     if (hdr->version != 6) {
> > +             SKB_DR_SET(reason, UNHANDLED_PROTO);
> >               goto err;
> > +     }
> >
> >       __IP6_ADD_STATS(net, idev,
> >                       IPSTATS_MIB_NOECTPKTS +
> > @@ -226,8 +232,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >       if (!ipv6_addr_is_multicast(&hdr->daddr) &&
> >           (skb->pkt_type == PACKET_BROADCAST ||
> >            skb->pkt_type == PACKET_MULTICAST) &&
> > -         idev->cnf.drop_unicast_in_l2_multicast)
> > +         idev->cnf.drop_unicast_in_l2_multicast) {
> > +             SKB_DR_SET(reason, UNICAST_IN_L2_MULTICAST);
> >               goto err;
> > +     }
> >
> >       /* RFC4291 2.7
> >        * Nodes must not originate a packet to a multicast address whose scope
> > @@ -256,12 +264,11 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >               if (pkt_len + sizeof(struct ipv6hdr) > skb->len) {
> >                       __IP6_INC_STATS(net,
> >                                       idev, IPSTATS_MIB_INTRUNCATEDPKTS);
> > +                     SKB_DR_SET(reason, PKT_TOO_SMALL);
> >                       goto drop;
> >               }
> > -             if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr))) {
> > -                     __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> > -                     goto drop;
> > -             }
> > +             if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr)))
> > +                     goto err;
> >               hdr = ipv6_hdr(skb);
> >       }
> >
> > @@ -282,9 +289,10 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
> >       return skb;
> >   err:
> >       __IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
> > +     SKB_DR_OR(reason, IP_INHDR);
> >   drop:
> >       rcu_read_unlock();
> > -     kfree_skb(skb);
> > +     kfree_skb_reason(skb, reason);
> >       return NULL;
> >   }
> >
diff mbox series

Patch

diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
index b4880c7c84eb..1b925ecb26e9 100644
--- a/net/ipv6/ip6_input.c
+++ b/net/ipv6/ip6_input.c
@@ -145,13 +145,14 @@  static void ip6_list_rcv_finish(struct net *net, struct sock *sk,
 static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 				    struct net *net)
 {
+	enum skb_drop_reason reason;
 	const struct ipv6hdr *hdr;
 	u32 pkt_len;
 	struct inet6_dev *idev;
 
 	if (skb->pkt_type == PACKET_OTHERHOST) {
 		dev_core_stats_rx_otherhost_dropped_inc(skb->dev);
-		kfree_skb(skb);
+		kfree_skb_reason(skb, SKB_DROP_REASON_OTHERHOST);
 		return NULL;
 	}
 
@@ -161,9 +162,12 @@  static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 
 	__IP6_UPD_PO_STATS(net, idev, IPSTATS_MIB_IN, skb->len);
 
+	SKB_DR_SET(reason, NOT_SPECIFIED);
 	if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL ||
 	    !idev || unlikely(idev->cnf.disable_ipv6)) {
 		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INDISCARDS);
+		if (unlikely(idev->cnf.disable_ipv6))
+			SKB_DR_SET(reason, IPV6DISABLED);
 		goto drop;
 	}
 
@@ -187,8 +191,10 @@  static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 
 	hdr = ipv6_hdr(skb);
 
-	if (hdr->version != 6)
+	if (hdr->version != 6) {
+		SKB_DR_SET(reason, UNHANDLED_PROTO);
 		goto err;
+	}
 
 	__IP6_ADD_STATS(net, idev,
 			IPSTATS_MIB_NOECTPKTS +
@@ -226,8 +232,10 @@  static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 	if (!ipv6_addr_is_multicast(&hdr->daddr) &&
 	    (skb->pkt_type == PACKET_BROADCAST ||
 	     skb->pkt_type == PACKET_MULTICAST) &&
-	    idev->cnf.drop_unicast_in_l2_multicast)
+	    idev->cnf.drop_unicast_in_l2_multicast) {
+		SKB_DR_SET(reason, UNICAST_IN_L2_MULTICAST);
 		goto err;
+	}
 
 	/* RFC4291 2.7
 	 * Nodes must not originate a packet to a multicast address whose scope
@@ -256,12 +264,11 @@  static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 		if (pkt_len + sizeof(struct ipv6hdr) > skb->len) {
 			__IP6_INC_STATS(net,
 					idev, IPSTATS_MIB_INTRUNCATEDPKTS);
+			SKB_DR_SET(reason, PKT_TOO_SMALL);
 			goto drop;
 		}
-		if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr))) {
-			__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
-			goto drop;
-		}
+		if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr)))
+			goto err;
 		hdr = ipv6_hdr(skb);
 	}
 
@@ -282,9 +289,10 @@  static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 	return skb;
 err:
 	__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
+	SKB_DR_OR(reason, IP_INHDR);
 drop:
 	rcu_read_unlock();
-	kfree_skb(skb);
+	kfree_skb_reason(skb, reason);
 	return NULL;
 }