| Message ID | 20200519073229.GA20624@noodle |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] __netif_receive_skb_core: pass skb by reference | expand |
On 19/05/2020 08:32, 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. > > v2: Added Fixes tag and comment regarding ppt_prev and skb invariant. > > Fixes: 88eb1944e18c ("net: core: propagate SKB lists through packet_type lookup") > Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com> > --- > net/core/dev.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 6d327b7aa813..38adb56624f7 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; Reverse christmas tree here, or else David will complain ;-) Apart from that, Acked-by: Edward Cree <ecree@solarflare.com>
From: Boris Sukholitko <boris.sukholitko@broadcom.com> Date: Tue, 19 May 2020 10:32:37 +0300 > __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. > > v2: Added Fixes tag and comment regarding ppt_prev and skb invariant. > > Fixes: 88eb1944e18c ("net: core: propagate SKB lists through packet_type lookup") > Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com> Applied and queued up for -stable. > @@ -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; I fixed up the reverse christmas tree variable ordering here, please take care in this area next time. Thank you.
diff --git a/net/core/dev.c b/net/core/dev.c index 6d327b7aa813..38adb56624f7 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,13 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc, } out: + /* The invariant here is that if *ppt_prev is not NULL + * then skb should also be non-NULL. + * + * Apparently *ppt_prev assignment above holds this invariant due to + * skb dereferencing near it. + */ + *pskb = skb; return ret; } @@ -5183,7 +5193,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 +5271,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) {
__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. v2: Added Fixes tag and comment regarding ppt_prev and skb invariant. Fixes: 88eb1944e18c ("net: core: propagate SKB lists through packet_type lookup") Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com> --- net/core/dev.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)