* [PATCH net] sch_netem: fix skb leak in netem_enqueue()
@ 2018-03-02 18:16 Alexey Kodanev
2018-03-02 18:44 ` Stephen Hemminger
2018-03-03 12:20 ` Neil Horman
0 siblings, 2 replies; 8+ messages in thread
From: Alexey Kodanev @ 2018-03-02 18:16 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, David Miller, Alexey Kodanev
When we exceed current packets limit and 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 in that case.
Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
---
net/sched/sch_netem.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 7c179ad..a5023a2 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -508,8 +508,14 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
1<<(prandom_u32() % 8);
}
- if (unlikely(sch->q.qlen >= sch->limit))
+ if (unlikely(sch->q.qlen >= sch->limit)) {
+ while (segs) {
+ skb2 = segs->next;
+ __qdisc_drop(segs, to_free);
+ segs = skb2;
+ }
return qdisc_drop(skb, sch, to_free);
+ }
qdisc_qstats_backlog_inc(sch, skb);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
2018-03-02 18:16 [PATCH net] sch_netem: fix skb leak in netem_enqueue() Alexey Kodanev
@ 2018-03-02 18:44 ` Stephen Hemminger
2018-03-03 3:30 ` Eric Dumazet
2018-03-03 12:20 ` Neil Horman
1 sibling, 1 reply; 8+ messages in thread
From: Stephen Hemminger @ 2018-03-02 18:44 UTC (permalink / raw)
To: Alexey Kodanev; +Cc: netdev, Neil Horman, David Miller
On Fri, 2 Mar 2018 21:16:48 +0300
Alexey Kodanev <alexey.kodanev@oracle.com> wrote:
> When we exceed current packets limit and 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 in that case.
>
> Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> ---
> net/sched/sch_netem.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index 7c179ad..a5023a2 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -508,8 +508,14 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> 1<<(prandom_u32() % 8);
> }
>
> - if (unlikely(sch->q.qlen >= sch->limit))
> + if (unlikely(sch->q.qlen >= sch->limit)) {
> + while (segs) {
> + skb2 = segs->next;
> + __qdisc_drop(segs, to_free);
> + segs = skb2;
> + }
> return qdisc_drop(skb, sch, to_free);
> + }
>
> qdisc_qstats_backlog_inc(sch, skb);
>
Since this is a generic problem why is not fixed in qdisc_drop instead?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
2018-03-02 18:44 ` Stephen Hemminger
@ 2018-03-03 3:30 ` Eric Dumazet
0 siblings, 0 replies; 8+ messages in thread
From: Eric Dumazet @ 2018-03-03 3:30 UTC (permalink / raw)
To: Stephen Hemminger, Alexey Kodanev; +Cc: netdev, Neil Horman, David Miller
On Fri, 2018-03-02 at 10:44 -0800, Stephen Hemminger wrote:
> On Fri, 2 Mar 2018 21:16:48 +0300
>
> Since this is a generic problem why is not fixed in qdisc_drop instead?
AFAIK only netem and tbf might segment GSO packets so far.
I am not sure we want to add code in qdisc_drop() that is used under
stress on normal qdiscs and are inline code.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
2018-03-02 18:16 [PATCH net] sch_netem: fix skb leak in netem_enqueue() Alexey Kodanev
2018-03-02 18:44 ` Stephen Hemminger
@ 2018-03-03 12:20 ` Neil Horman
2018-03-05 12:57 ` Alexey Kodanev
1 sibling, 1 reply; 8+ messages in thread
From: Neil Horman @ 2018-03-03 12:20 UTC (permalink / raw)
To: Alexey Kodanev; +Cc: netdev, David Miller
On Fri, Mar 02, 2018 at 09:16:48PM +0300, Alexey Kodanev wrote:
> When we exceed current packets limit and 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 in that case.
>
> Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> ---
> net/sched/sch_netem.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index 7c179ad..a5023a2 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
> @@ -508,8 +508,14 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> 1<<(prandom_u32() % 8);
> }
>
> - if (unlikely(sch->q.qlen >= sch->limit))
> + if (unlikely(sch->q.qlen >= sch->limit)) {
> + while (segs) {
> + skb2 = segs->next;
> + __qdisc_drop(segs, to_free);
> + segs = skb2;
> + }
> return qdisc_drop(skb, sch, to_free);
> + }
>
It seems like it might be nice to wrap up this drop loop into a
qdisc_drop_all inline function. Then we can easily drop segments in other
locations if we should need to
Regards
Neil
> qdisc_qstats_backlog_inc(sch, skb);
>
> --
> 1.8.3.1
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
2018-03-03 12:20 ` Neil Horman
@ 2018-03-05 12:57 ` Alexey Kodanev
2018-03-05 15:13 ` Eric Dumazet
2018-03-05 23:11 ` Neil Horman
0 siblings, 2 replies; 8+ messages in thread
From: Alexey Kodanev @ 2018-03-05 12:57 UTC (permalink / raw)
To: Neil Horman; +Cc: netdev, David Miller
On 03/03/2018 03:20 PM, Neil Horman wrote:
> On Fri, Mar 02, 2018 at 09:16:48PM +0300, Alexey Kodanev wrote:
>> When we exceed current packets limit and 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 in that case.
>>
>> Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
>> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
>> ---
>> net/sched/sch_netem.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
>> index 7c179ad..a5023a2 100644
>> --- a/net/sched/sch_netem.c
>> +++ b/net/sched/sch_netem.c
>> @@ -508,8 +508,14 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>> 1<<(prandom_u32() % 8);
>> }
>>
>> - if (unlikely(sch->q.qlen >= sch->limit))
>> + if (unlikely(sch->q.qlen >= sch->limit)) {
>> + while (segs) {
>> + skb2 = segs->next;
>> + __qdisc_drop(segs, to_free);
>> + segs = skb2;
>> + }
>> return qdisc_drop(skb, sch, to_free);
>> + }
>>
> It seems like it might be nice to wrap up this drop loop into a
> qdisc_drop_all inline function. Then we can easily drop segments in other
> locations if we should need to
Agree, will prepare the patch. I guess we could just add 'segs' to 'to_free'
list, then add qdisc_drop_all() with stats counter and returning status,
something like this:
@@ -824,6 +824,18 @@ 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)
+{
+ struct sk_buff *first = skb;
+
+ while (skb->next)
+ skb = skb->next;
+
+ skb->next = *to_free;
+ *to_free = first;
+}
Thanks,
Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
2018-03-05 12:57 ` Alexey Kodanev
@ 2018-03-05 15:13 ` Eric Dumazet
2018-03-05 15:40 ` Alexey Kodanev
2018-03-05 23:11 ` Neil Horman
1 sibling, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2018-03-05 15:13 UTC (permalink / raw)
To: Alexey Kodanev, Neil Horman; +Cc: netdev, David Miller
On Mon, 2018-03-05 at 15:57 +0300, Alexey Kodanev wrote:
>
> +static inline void __qdisc_drop_all(struct sk_buff *skb,
> + struct sk_buff **to_free)
> +{
> + struct sk_buff *first = skb;
> +
> + while (skb->next)
> + skb = skb->next;
> +
> + skb->next = *to_free;
> + *to_free = first;
> +}
You probably can leverage what I did for commit bec3cfdca36bf43cfa
("net: skb_segment() provides list head and tail")
to avoid the iteration.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
2018-03-05 15:13 ` Eric Dumazet
@ 2018-03-05 15:40 ` Alexey Kodanev
0 siblings, 0 replies; 8+ messages in thread
From: Alexey Kodanev @ 2018-03-05 15:40 UTC (permalink / raw)
To: Eric Dumazet, Neil Horman; +Cc: netdev, David Miller
On 03/05/2018 06:13 PM, Eric Dumazet wrote:
> On Mon, 2018-03-05 at 15:57 +0300, Alexey Kodanev wrote:
>>
>> +static inline void __qdisc_drop_all(struct sk_buff *skb,
>> + struct sk_buff **to_free)
>> +{
>> + struct sk_buff *first = skb;
>> +
>> + while (skb->next)
>> + skb = skb->next;
>> +
>> + skb->next = *to_free;
>> + *to_free = first;
>> +}
>
> You probably can leverage what I did for commit bec3cfdca36bf43cfa
> ("net: skb_segment() provides list head and tail")
> to avoid the iteration.
OK, thanks! Will use it and send a new version of the patch.
Thanks,
Alexey
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
2018-03-05 12:57 ` Alexey Kodanev
2018-03-05 15:13 ` Eric Dumazet
@ 2018-03-05 23:11 ` Neil Horman
1 sibling, 0 replies; 8+ messages in thread
From: Neil Horman @ 2018-03-05 23:11 UTC (permalink / raw)
To: Alexey Kodanev; +Cc: netdev, David Miller
On Mon, Mar 05, 2018 at 03:57:52PM +0300, Alexey Kodanev wrote:
> On 03/03/2018 03:20 PM, Neil Horman wrote:
> > On Fri, Mar 02, 2018 at 09:16:48PM +0300, Alexey Kodanev wrote:
> >> When we exceed current packets limit and 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 in that case.
> >>
> >> Fixes: 6071bd1aa13e ("netem: Segment GSO packets on enqueue")
> >> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
> >> ---
> >> net/sched/sch_netem.c | 8 +++++++-
> >> 1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> >> index 7c179ad..a5023a2 100644
> >> --- a/net/sched/sch_netem.c
> >> +++ b/net/sched/sch_netem.c
> >> @@ -508,8 +508,14 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >> 1<<(prandom_u32() % 8);
> >> }
> >>
> >> - if (unlikely(sch->q.qlen >= sch->limit))
> >> + if (unlikely(sch->q.qlen >= sch->limit)) {
> >> + while (segs) {
> >> + skb2 = segs->next;
> >> + __qdisc_drop(segs, to_free);
> >> + segs = skb2;
> >> + }
> >> return qdisc_drop(skb, sch, to_free);
> >> + }
> >>
> > It seems like it might be nice to wrap up this drop loop into a
> > qdisc_drop_all inline function. Then we can easily drop segments in other
> > locations if we should need to
>
>
> Agree, will prepare the patch. I guess we could just add 'segs' to 'to_free'
> list, then add qdisc_drop_all() with stats counter and returning status,
> something like this:
>
> @@ -824,6 +824,18 @@ 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)
> +{
> + struct sk_buff *first = skb;
> +
> + while (skb->next)
> + skb = skb->next;
> +
> + skb->next = *to_free;
> + *to_free = first;
> +}
>
I agree
Thanks!
Neil
> Thanks,
> Alexey
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-03-05 23:11 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-02 18:16 [PATCH net] sch_netem: fix skb leak in netem_enqueue() Alexey Kodanev
2018-03-02 18:44 ` Stephen Hemminger
2018-03-03 3:30 ` Eric Dumazet
2018-03-03 12:20 ` Neil Horman
2018-03-05 12:57 ` Alexey Kodanev
2018-03-05 15:13 ` Eric Dumazet
2018-03-05 15:40 ` Alexey Kodanev
2018-03-05 23:11 ` Neil Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).