[PATCH net] __netif_receive_skb_core: pass skb by reference

[PATCH net] __netif_receive_skb_core: pass skb by reference

[Boris Sukholitko]

__netif_receive_skb_core may change the skb pointer passed into it (e.g.
in rx_handler). The original skb may be freed as a result of this
operation.

The callers of __netif_receive_skb_core may further process original skb
by using pt_prev pointer returned by __netif_receive_skb_core thus
leading to unpleasant effects.

The solution is to pass skb by reference into __netif_receive_skb_core.

Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com>
---
 net/core/dev.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 6d327b7aa813..0c2bbf479f19 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4988,7 +4988,7 @@ static inline int nf_ingress(struct sk_buff *skb, struct packet_type **pt_prev,
 	return 0;
 }
 
-static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
+static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
 				    struct packet_type **ppt_prev)
 {
 	struct packet_type *ptype, *pt_prev;
@@ -4997,6 +4997,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
 	bool deliver_exact = false;
 	int ret = NET_RX_DROP;
 	__be16 type;
+	struct sk_buff *skb = *pskb;
 
 	net_timestamp_check(!netdev_tstamp_prequeue, skb);
 
@@ -5023,8 +5024,10 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
 		ret2 = do_xdp_generic(rcu_dereference(skb->dev->xdp_prog), skb);
 		preempt_enable();
 
-		if (ret2 != XDP_PASS)
-			return NET_RX_DROP;
+		if (ret2 != XDP_PASS) {
+			ret = NET_RX_DROP;
+			goto out;
+		}
 		skb_reset_mac_len(skb);
 	}
 
@@ -5174,6 +5177,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
 	}
 
 out:
+	*pskb = skb;
 	return ret;
 }
 
@@ -5183,7 +5187,7 @@ static int __netif_receive_skb_one_core(struct sk_buff *skb, bool pfmemalloc)
 	struct packet_type *pt_prev = NULL;
 	int ret;
 
-	ret = __netif_receive_skb_core(skb, pfmemalloc, &pt_prev);
+	ret = __netif_receive_skb_core(&skb, pfmemalloc, &pt_prev);
 	if (pt_prev)
 		ret = INDIRECT_CALL_INET(pt_prev->func, ipv6_rcv, ip_rcv, skb,
 					 skb->dev, pt_prev, orig_dev);
@@ -5261,7 +5265,7 @@ static void __netif_receive_skb_list_core(struct list_head *head, bool pfmemallo
 		struct packet_type *pt_prev = NULL;
 
 		skb_list_del_init(skb);
-		__netif_receive_skb_core(skb, pfmemalloc, &pt_prev);
+		__netif_receive_skb_core(&skb, pfmemalloc, &pt_prev);
 		if (!pt_prev)
 			continue;
 		if (pt_curr != pt_prev || od_curr != orig_dev) {
-- 
2.19.2

Re: [PATCH net] __netif_receive_skb_core: pass skb by reference

[Eric Dumazet]

On 5/18/20 2:01 AM, Boris Sukholitko wrote:
> __netif_receive_skb_core may change the skb pointer passed into it (e.g.
> in rx_handler). The original skb may be freed as a result of this
> operation.
> 
> The callers of __netif_receive_skb_core may further process original skb
> by using pt_prev pointer returned by __netif_receive_skb_core thus
> leading to unpleasant effects.
> 
> The solution is to pass skb by reference into __netif_receive_skb_core.
> 
> Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com>

Please provide a Fixes: tag for such a scary patch.

Thanks.

Re: [PATCH net] __netif_receive_skb_core: pass skb by reference

[Edward Cree]

On 18/05/2020 10:01, Boris Sukholitko wrote:
> __netif_receive_skb_core may change the skb pointer passed into it (e.g.
> in rx_handler). The original skb may be freed as a result of this
> operation.
>
> The callers of __netif_receive_skb_core may further process original skb
> by using pt_prev pointer returned by __netif_receive_skb_core thus
> leading to unpleasant effects.
>
> The solution is to pass skb by reference into __netif_receive_skb_core.
>
> Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com>
I think this patch is correct, but there's a couple of things I'd like
 to see before adding my Ack.
Firstly, please add a Fixes: tag; I expect the relevant commit will be
 88eb1944e18c ("net: core: propagate SKB lists through packet_type lookup")
 but I'm not 100% sure so do check that yourself.
Secondly:
> @@ -5174,6 +5177,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
>  	}
>  
>  out:
> +	*pskb = skb;
>  	return ret;
>  }
Could we have some sort of WARN_ONs (maybe under #ifdef DEBUG) to check
 that we never have a NULL skb with a non-NULL pt_prev?  Or at least a
 comment at the top of the function stating this part of its contract
 with callers?  I've gone through and convinced myself that it never
 happens currently, but that depends on some fairly subtle details.

-ed

Re: [PATCH net] __netif_receive_skb_core: pass skb by reference

[Boris Sukholitko]

On Mon, May 18, 2020 at 05:20:13PM +0100, Edward Cree wrote:
> Firstly, please add a Fixes: tag; I expect the relevant commit will be
>  88eb1944e18c ("net: core: propagate SKB lists through packet_type lookup")
>  but I'm not 100% sure so do check that yourself.

You are right, this is the right commit. Fixes tag added.

> Secondly:
> > @@ -5174,6 +5177,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
> >  	}
> >  
> >  out:
> > +	*pskb = skb;
> >  	return ret;
> >  }
> Could we have some sort of WARN_ONs (maybe under #ifdef DEBUG) to check
>  that we never have a NULL skb with a non-NULL pt_prev?  Or at least a
>  comment at the top of the function stating this part of its contract
>  with callers?  I've gone through and convinced myself that it never
>  happens currently, but that depends on some fairly subtle details.

I've added the comment at this hunk. The *ppt_prev assignment happens
several lines above and skb is being used right next to it.

Please see v2 of the patch with this changes.

Thanks,
Boris.

Re: [PATCH net] __netif_receive_skb_core: pass skb by reference

[Boris Sukholitko]

On Mon, May 18, 2020 at 08:59:56AM -0700, Eric Dumazet wrote:
> 
> Please provide a Fixes: tag for such a scary patch.

Done in v2.

Thanks,
Boris.