Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
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> >> --- > >> 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 >
Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
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
Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
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.
Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
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>> --- >> 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
Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
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 0x880b5d23b600 (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: > [] __alloc_skb+0xc9/0x520 > [<1709b32f>] skb_segment+0x8c8/0x3710 > [ ] tcp_gso_segment+0x331/0x1830 > [ ] inet_gso_segment+0x476/0x1370 > [<8b762dd4>] skb_mac_gso_segment+0x1f9/0x510 > [<2182660a>] __skb_gso_segment+0x1dd/0x620 > [<412651b9>] netem_enqueue+0x1536/0x2590 [sch_netem] > [<05d3b2a9>] __dev_queue_xmit+0x1167/0x2120 > [ ] ip_finish_output2+0x998/0xf00 > [ ] ip_output+0x1aa/0x2c0 > [<7ecbd3a4>] tcp_transmit_skb+0x18db/0x3670 > [<42d2a45f>] tcp_write_xmit+0x4d4/0x58c0 > [<56a44199>] tcp_tasklet_func+0x3d9/0x540 > [<13d06d02>] tasklet_action+0x1ca/0x250 > [ ] __do_softirq+0x1b4/0x5a3 > [ ] 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 > --- > 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 > >
Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
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.
Re: [PATCH net] sch_netem: fix skb leak in netem_enqueue()
On Fri, 2 Mar 2018 21:16:48 +0300 Alexey Kodanevwrote: > 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 0x880b5d23b600 (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: > [ ] __alloc_skb+0xc9/0x520 > [<1709b32f>] skb_segment+0x8c8/0x3710 > [ ] tcp_gso_segment+0x331/0x1830 > [ ] inet_gso_segment+0x476/0x1370 > [<8b762dd4>] skb_mac_gso_segment+0x1f9/0x510 > [<2182660a>] __skb_gso_segment+0x1dd/0x620 > [<412651b9>] netem_enqueue+0x1536/0x2590 [sch_netem] > [<05d3b2a9>] __dev_queue_xmit+0x1167/0x2120 > [ ] ip_finish_output2+0x998/0xf00 > [ ] ip_output+0x1aa/0x2c0 > [<7ecbd3a4>] tcp_transmit_skb+0x18db/0x3670 > [<42d2a45f>] tcp_write_xmit+0x4d4/0x58c0 > [<56a44199>] tcp_tasklet_func+0x3d9/0x540 > [<13d06d02>] tasklet_action+0x1ca/0x250 > [ ] __do_softirq+0x1b4/0x5a3 > [ ] 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 > --- > 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?
[PATCH net] sch_netem: fix skb leak in netem_enqueue()
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 0x880b5d23b600 (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: [] __alloc_skb+0xc9/0x520 [<1709b32f>] skb_segment+0x8c8/0x3710 [ ] tcp_gso_segment+0x331/0x1830 [ ] inet_gso_segment+0x476/0x1370 [<8b762dd4>] skb_mac_gso_segment+0x1f9/0x510 [<2182660a>] __skb_gso_segment+0x1dd/0x620 [<412651b9>] netem_enqueue+0x1536/0x2590 [sch_netem] [<05d3b2a9>] __dev_queue_xmit+0x1167/0x2120 [ ] ip_finish_output2+0x998/0xf00 [ ] ip_output+0x1aa/0x2c0 [<7ecbd3a4>] tcp_transmit_skb+0x18db/0x3670 [<42d2a45f>] tcp_write_xmit+0x4d4/0x58c0 [<56a44199>] tcp_tasklet_func+0x3d9/0x540 [<13d06d02>] tasklet_action+0x1ca/0x250 [ ] __do_softirq+0x1b4/0x5a3 [ ] 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 --- 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