| Message ID | 1520272374-25124-1-git-send-email-alexey.kodanev@oracle.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v3] sch_netem: fix skb leak in netem_enqueue() | expand |
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>
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!
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);
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(-)