[net,v3] sch_netem: fix skb leak in netem_enqueue()

Message ID 1520272374-25124-1-git-send-email-alexey.kodanev@oracle.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net,v3] sch_netem: fix skb leak in netem_enqueue()
Related show

Commit Message

Alexey Kodanev March 5, 2018, 5:52 p.m.
When we exceed current packets limit and we have more than one
segment in the list returned by skb_gso_segment(), netem drops
only the first one, skipping the rest, hence kmemleak reports:

unreferenced object 0xffff880b5d23b600 (size 1024):
  comm "softirq", pid 0, jiffies 4384527763 (age 2770.629s)
  hex dump (first 32 bytes):
    00 80 23 5d 0b 88 ff ff 00 00 00 00 00 00 00 00  ..#]............
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<00000000d8a19b9d>] __alloc_skb+0xc9/0x520
    [<000000001709b32f>] skb_segment+0x8c8/0x3710
    [<00000000c7b9bb88>] tcp_gso_segment+0x331/0x1830
    [<00000000c921cba1>] inet_gso_segment+0x476/0x1370
    [<000000008b762dd4>] skb_mac_gso_segment+0x1f9/0x510
    [<000000002182660a>] __skb_gso_segment+0x1dd/0x620
    [<00000000412651b9>] netem_enqueue+0x1536/0x2590 [sch_netem]
    [<0000000005d3b2a9>] __dev_queue_xmit+0x1167/0x2120
    [<00000000fc5f7327>] ip_finish_output2+0x998/0xf00
    [<00000000d309e9d3>] ip_output+0x1aa/0x2c0
    [<000000007ecbd3a4>] tcp_transmit_skb+0x18db/0x3670
    [<0000000042d2a45f>] tcp_write_xmit+0x4d4/0x58c0
    [<0000000056a44199>] tcp_tasklet_func+0x3d9/0x540
    [<0000000013d06d02>] tasklet_action+0x1ca/0x250
    [<00000000fcde0b8b>] __do_softirq+0x1b4/0x5a3
    [<00000000e7ed027c>] irq_exit+0x1e2/0x210

Fix it by adding the rest of the segments, if any, to skb 'to_free'
list. Add new __qdisc_drop_all() and qdisc_drop_all() functions
because they can be useful in the future if we need to drop segmented
GSO packets in other places.

Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---

v3: use skb->prev to find the tail of the list. skb->prev can be NULL
    for not segmented skbs, so check it too.

v2: add new __qdisc_drop_all() and qdisc_drop_all(), and use
    qdisc_drop_all() in sch_netem.


 include/net/sch_generic.h | 19 +++++++++++++++++++
 net/sched/sch_netem.c     |  2 +-
 2 files changed, 20 insertions(+), 1 deletion(-)

Comments

Neil Horman March 5, 2018, 11:43 p.m. | #1
On Mon, Mar 05, 2018 at 08:52:54PM +0300, Alexey Kodanev wrote:
> When we exceed current packets limit and we have more than one
> segment in the list returned by skb_gso_segment(), netem drops
> only the first one, skipping the rest, hence kmemleak reports:
> 
> unreferenced object 0xffff880b5d23b600 (size 1024):
>   comm "softirq", pid 0, jiffies 4384527763 (age 2770.629s)
>   hex dump (first 32 bytes):
>     00 80 23 5d 0b 88 ff ff 00 00 00 00 00 00 00 00  ..#]............
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<00000000d8a19b9d>] __alloc_skb+0xc9/0x520
>     [<000000001709b32f>] skb_segment+0x8c8/0x3710
>     [<00000000c7b9bb88>] tcp_gso_segment+0x331/0x1830
>     [<00000000c921cba1>] inet_gso_segment+0x476/0x1370
>     [<000000008b762dd4>] skb_mac_gso_segment+0x1f9/0x510
>     [<000000002182660a>] __skb_gso_segment+0x1dd/0x620
>     [<00000000412651b9>] netem_enqueue+0x1536/0x2590 [sch_netem]
>     [<0000000005d3b2a9>] __dev_queue_xmit+0x1167/0x2120
>     [<00000000fc5f7327>] ip_finish_output2+0x998/0xf00
>     [<00000000d309e9d3>] ip_output+0x1aa/0x2c0
>     [<000000007ecbd3a4>] tcp_transmit_skb+0x18db/0x3670
>     [<0000000042d2a45f>] tcp_write_xmit+0x4d4/0x58c0
>     [<0000000056a44199>] tcp_tasklet_func+0x3d9/0x540
>     [<0000000013d06d02>] tasklet_action+0x1ca/0x250
>     [<00000000fcde0b8b>] __do_softirq+0x1b4/0x5a3
>     [<00000000e7ed027c>] irq_exit+0x1e2/0x210
> 
> Fix it by adding the rest of the segments, if any, to skb 'to_free'
> list. Add new __qdisc_drop_all() and qdisc_drop_all() functions
> because they can be useful in the future if we need to drop segmented
> GSO packets in other places.
> 
> Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> ---
> 
> v3: use skb->prev to find the tail of the list. skb->prev can be NULL
>     for not segmented skbs, so check it too.
> 
> v2: add new __qdisc_drop_all() and qdisc_drop_all(), and use
>     qdisc_drop_all() in sch_netem.
> 
> 
>  include/net/sch_generic.h | 19 +++++++++++++++++++
>  net/sched/sch_netem.c     |  2 +-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index e2ab136..2092d33 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -824,6 +824,16 @@ static inline void __qdisc_drop(struct sk_buff *skb, struct sk_buff **to_free)
>  	*to_free = skb;
>  }
>  
> +static inline void __qdisc_drop_all(struct sk_buff *skb,
> +				    struct sk_buff **to_free)
> +{
> +	if (skb->prev)
> +		skb->prev->next = *to_free;
> +	else
> +		skb->next = *to_free;
> +	*to_free = skb;
> +}
> +
>  static inline unsigned int __qdisc_queue_drop_head(struct Qdisc *sch,
>  						   struct qdisc_skb_head *qh,
>  						   struct sk_buff **to_free)
> @@ -956,6 +966,15 @@ static inline int qdisc_drop(struct sk_buff *skb, struct Qdisc *sch,
>  	return NET_XMIT_DROP;
>  }
>  
> +static inline int qdisc_drop_all(struct sk_buff *skb, struct Qdisc *sch,
> +				 struct sk_buff **to_free)
> +{
> +	__qdisc_drop_all(skb, to_free);
> +	qdisc_qstats_drop(sch);
> +
> +	return NET_XMIT_DROP;
> +}
> +
>  /* Length to Time (L2T) lookup in a qdisc_rate_table, to determine how
>     long it will take to send a packet given its size.
>   */
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index 7c179ad..7d6801f 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -509,7 +509,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	}
>  
>  	if (unlikely(sch->q.qlen >= sch->limit))
> -		return qdisc_drop(skb, sch, to_free);
> +		return qdisc_drop_all(skb, sch, to_free);
>  
>  	qdisc_qstats_backlog_inc(sch, skb);
>  
> -- 
> 1.8.3.1
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>
David Miller March 7, 2018, 4:18 p.m. | #2
From: Alexey Kodanev <alexey.kodanev@oracle.com>
Date: Mon,  5 Mar 2018 20:52:54 +0300

> When we exceed current packets limit and we have more than one
> segment in the list returned by skb_gso_segment(), netem drops
> only the first one, skipping the rest, hence kmemleak reports:
 ...
> Fix it by adding the rest of the segments, if any, to skb 'to_free'
> list. Add new __qdisc_drop_all() and qdisc_drop_all() functions
> because they can be useful in the future if we need to drop segmented
> GSO packets in other places.
> 
> Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> ---
> 
> v3: use skb->prev to find the tail of the list. skb->prev can be NULL
>     for not segmented skbs, so check it too.
> 
> v2: add new __qdisc_drop_all() and qdisc_drop_all(), and use
>     qdisc_drop_all() in sch_netem.

Applied and queued up for -stable, thanks!

Patch

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index e2ab136..2092d33 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -824,6 +824,16 @@  static inline void __qdisc_drop(struct sk_buff *skb, struct sk_buff **to_free)
 	*to_free = skb;
 }
 
+static inline void __qdisc_drop_all(struct sk_buff *skb,
+				    struct sk_buff **to_free)
+{
+	if (skb->prev)
+		skb->prev->next = *to_free;
+	else
+		skb->next = *to_free;
+	*to_free = skb;
+}
+
 static inline unsigned int __qdisc_queue_drop_head(struct Qdisc *sch,
 						   struct qdisc_skb_head *qh,
 						   struct sk_buff **to_free)
@@ -956,6 +966,15 @@  static inline int qdisc_drop(struct sk_buff *skb, struct Qdisc *sch,
 	return NET_XMIT_DROP;
 }
 
+static inline int qdisc_drop_all(struct sk_buff *skb, struct Qdisc *sch,
+				 struct sk_buff **to_free)
+{
+	__qdisc_drop_all(skb, to_free);
+	qdisc_qstats_drop(sch);
+
+	return NET_XMIT_DROP;
+}
+
 /* Length to Time (L2T) lookup in a qdisc_rate_table, to determine how
    long it will take to send a packet given its size.
  */
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 7c179ad..7d6801f 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -509,7 +509,7 @@  static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
 	}
 
 	if (unlikely(sch->q.qlen >= sch->limit))
-		return qdisc_drop(skb, sch, to_free);
+		return qdisc_drop_all(skb, sch, to_free);
 
 	qdisc_qstats_backlog_inc(sch, skb);