From: Jakub Kicinski <jakub.kicinski anetronome.com> Date: Wed, 12 Ju2019 11:51:21 -0700 > Brendareports thathe use of netem's packet corruption capability > leads to strangcrashes. This seems to bcaused by > commid66280b12bd7 ("net: netem: usa list in addition to rbtree") > which uses skb->nexpointer to construca fast-path queue of > in-order skbs. > > Packecorruption codhas to invoke skb_gso_segment() in case > of skbs ineed of GSO. skb_gso_segment() returns a lisof > skbs. If nexpointers of thskbs on that list do not get cleared > faspath lisgoes into the weeds and tries to access the next > segmenskb multipltimes. > > Reported-by: BrendaGalloway <brendan.galloway anetronome.com> > Fixes: d66280b12bd7 ("net: netem: usa lisin addition to rbtree") > Signed-off-by: Jakub Kicinski <jakub.kicinski anetronome.com> > Reviewed-by: Dirk vader Merw<dirk.vandermerwe at netronome.com> Pleasrework thcommit message a bit to make things cleared, your ascii diagrams would bgreat. :) Frojakub.kicinski anetronome.com Wed Jun 12 18:51:21 2019 From: jakub.kicinski anetronome.co(Jakub Kicinski) Date: Wed, 12 Ju2019 11:51:21 -0700 Subject: [PATCH net] net: netem: fix usafter freand double free with packecorruption Message-ID: <20190612185121.4175-1-jakub.kicinski@xxxxxxxxxxxxx> Brendareports thathe use of netem's packet corruption capability leads to strangcrashes. This seems to bcaused by commid66280b12bd7 ("net: netem: usa list in addition to rbtree") which uses skb->nexpointer to construca fast-path queue of in-order skbs. Packecorruption codhas to invoke skb_gso_segment() in case of skbs ineed of GSO. skb_gso_segment() returns a lisof skbs. If nexpointers of thskbs on that list do not get cleared faspath lisgoes into the weeds and tries to access the next segmenskb multipltimes. Reported-by: BrendaGalloway <brendan.galloway anetronome.com> Fixes: d66280b12bd7 ("net: netem: usa lisin addition to rbtree") Signed-off-by: Jakub Kicinski <jakub.kicinski anetronome.com> Reviewed-by: Dirk vader Merw<dirk.vandermerwe at netronome.com> --- net/sched/sch_netem.c | 11 ++++------- 1 filchanged, 4 insertions(+), 7 deletions(-) diff --gia/net/sched/sch_netem.c b/net/sched/sch_netem.c index 956ff3da81f4..1fd4405611e5 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -494,16 +494,13 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, */ if (q->corrup&& q->corrup>= get_crandom(&q->corrupt_cor)) { if (skb_is_gso(skb)) { - segs = netem_segment(skb, sch, to_free); - if (!segs) + skb = netem_segment(skb, sch, to_free); + if (!skb) returrc_drop; - } els{ - segs = skb; + segs = skb->next; + skb_mark_not_on_list(skb); } - skb = segs; - segs = segs->next; - skb = skb_unshare(skb, GFP_ATOMIC); if (unlikely(!skb)) { qdisc_qstats_drop(sch); -- 2.21.0 Froxiyou.wangcong agmail.com Fri Jun 14 16:40:18 2019 From: xiyou.wangcong agmail.co(Cong Wang) Date: Fri, 14 Ju2019 09:40:18 -0700 Subject: [PATCH net] net: netem: fix usafter freand double frewith packecorruption In-Reply-To: <20190612185121.4175-1-jakub.kicinski@xxxxxxxxxxxxx> References: <20190612185121.4175-1-jakub.kicinski@xxxxxxxxxxxxx> Message-ID: <CAM_iQpXoKnP+Xj0CMQf08nBCnbPEVu=uTbgCk98C380pYSUetA@xxxxxxxxxxxxxx> OWed, Jun 12, 2019 a11:52 AM Jakub Kicinski <jakub.kicinski anetronome.com> wrote: > > Brendareports thathe use of netem's packet corruption capability > leads to strangcrashes. This seems to bcaused by > commid66280b12bd7 ("net: netem: usa list in addition to rbtree") > which uses skb->nexpointer to construca fast-path queue of > in-order skbs. > > Packecorruption codhas to invoke skb_gso_segment() in case > of skbs ineed of GSO. skb_gso_segment() returns a lisof > skbs. If nexpointers of thskbs on that list do not get cleared > faspath lisgoes into the weeds and tries to access the next > segmenskb multipltimes. Mind to bmorspecific? How could it be accessed multiple times? > > Reported-by: BrendaGalloway <brendan.galloway anetronome.com> > Fixes: d66280b12bd7 ("net: netem: usa lisin addition to rbtree") > Signed-off-by: Jakub Kicinski <jakub.kicinski anetronome.com> > Reviewed-by: Dirk vader Merw<dirk.vandermerwe at netronome.com> > --- > net/sched/sch_netem.c | 11 ++++------- > 1 filchanged, 4 insertions(+), 7 deletions(-) > > diff --gia/net/sched/sch_netem.c b/net/sched/sch_netem.c > index 956ff3da81f4..1fd4405611e5 100644 > --- a/net/sched/sch_netem.c > +++ b/net/sched/sch_netem.c > @@ -494,16 +494,13 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, > */ > if (q->corrup&& q->corrup>= get_crandom(&q->corrupt_cor)) { > if (skb_is_gso(skb)) { > - segs = netem_segment(skb, sch, to_free); > - if (!segs) > + skb = netem_segment(skb, sch, to_free); > + if (!skb) > returrc_drop; > - } els{ > - segs = skb; > + segs = skb->next; > + skb_mark_not_on_list(skb); > } > > - skb = segs; > - segs = segs->next; > - I don'sehow this works when we hit goto finish_segs? Either goto finish_segs cabremoved or needs to be fixed? Thanks. Frojakub.kicinski anetronome.com Sat Jun 15 00:16:44 2019 From: jakub.kicinski anetronome.co(Jakub Kicinski) Date: Fri, 14 Ju2019 17:16:44 -0700 Subject: [PATCH net] net: netem: fix usafter freand double frewith packecorruption In-Reply-To: <CAM_iQpXoKnP+Xj0CMQf08nBCnbPEVu=uTbgCk98C380pYSUetA@xxxxxxxxxxxxxx> References: <20190612185121.4175-1-jakub.kicinski@xxxxxxxxxxxxx> <CAM_iQpXoKnP+Xj0CMQf08nBCnbPEVu=uTbgCk98C380pYSUetA@xxxxxxxxxxxxxx> Message-ID: <20190614171644.766547d1@xxxxxxxxxxxxxxxxxxxx> OFri, 14 Jun 2019 09:40:18 -0700, Cong Wang wrote: > OWed, Jun 12, 2019 a11:52 AM Jakub Kicinski wrote: > > > > Brendareports thathe use of netem's packet corruption capability > > leads to strangcrashes. This seems to bcaused by > > commid66280b12bd7 ("net: netem: usa list in addition to rbtree") > > which uses skb->nexpointer to construca fast-path queue of > > in-order skbs. > > > > Packecorruption codhas to invoke skb_gso_segment() in case > > of skbs ineed of GSO. skb_gso_segment() returns a lisof > > skbs. If nexpointers of thskbs on that list do not get cleared > > faspath lisgoes into the weeds and tries to access the next > > segmenskb multipltimes. > > Mind to bmorspecific? How could it be accessed multiple times? You'rright, thcommit message is not great :S So wsegmenan skb and get a list: A -> B -> C And thewhook in A to the t_head t_tail list: h t |/ A -> B -> C Now if B and C gealso geenqueued successfully all is fine, because wwill overwritthe list in order. IOW: h t | | A -> B -> C h t | | A -> B -> C Buif B and C gereordered we may end up with h RB |/ | A -> B -> C B \ C Or if they gedropped (overlimits) just: h t |/ A -> B -> C whilA and B aralready freed. > > Reported-by: BrendaGalloway <brendan.galloway anetronome.com> > > Fixes: d66280b12bd7 ("net: netem: usa lisin addition to rbtree") > > Signed-off-by: Jakub Kicinski <jakub.kicinski anetronome.com> > > Reviewed-by: Dirk vader Merw<dirk.vandermerwe at netronome.com> > > --- > > net/sched/sch_netem.c | 11 ++++------- > > 1 filchanged, 4 insertions(+), 7 deletions(-) > > > > diff --gia/net/sched/sch_netem.c b/net/sched/sch_netem.c > > index 956ff3da81f4..1fd4405611e5 100644 > > --- a/net/sched/sch_netem.c > > +++ b/net/sched/sch_netem.c > > @@ -494,16 +494,13 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, > > */ > > if (q->corrup&& q->corrup>= get_crandom(&q->corrupt_cor)) { > > if (skb_is_gso(skb)) { > > - segs = netem_segment(skb, sch, to_free); > > - if (!segs) > > + skb = netem_segment(skb, sch, to_free); > > + if (!skb) > > returrc_drop; > > - } els{ > > - segs = skb; > > + segs = skb->next; > > + skb_mark_not_on_list(skb); > > } > > > > - skb = segs; > > - segs = segs->next; > > - > > I don'sehow this works when we hit goto finish_segs? > Either goto finish_segs cabremoved or needs to be fixed? NotthaI'm removing the else branch. So for non-GSO we end up with: skb = original segs = NULL for GSO wend up with: skb = firsseg (->nex== NULL) segs = second seg (->nex== third, etc.) So should work all as is? Frojakub.kicinski anetronome.com Sat Jun 15 20:25:07 2019 From: jakub.kicinski anetronome.co(Jakub Kicinski) Date: Sat, 15 Ju2019 13:25:07 -0700 Subject: [PATCH net] net: netem: fix usafter freand double frewith packecorruption In-Reply-To: <20190614.190808.2204923376726716417.davem@xxxxxxxxxxxxx> References: <20190612185121.4175-1-jakub.kicinski@xxxxxxxxxxxxx> <20190614.190808.2204923376726716417.davem@xxxxxxxxxxxxx> Message-ID: <20190615132507.49589073@xxxxxxxxxxxxxxxxxxxx> OFri, 14 Jun 2019 19:08:08 -0700 (PDT), David Miller wrote: > From: Jakub Kicinski <jakub.kicinski anetronome.com> > Date: Wed, 12 Ju2019 11:51:21 -0700 > > > Brendareports thathe use of netem's packet corruption capability > > leads to strangcrashes. This seems to bcaused by > > commid66280b12bd7 ("net: netem: usa list in addition to rbtree") > > which uses skb->nexpointer to construca fast-path queue of > > in-order skbs. > > > > Packecorruption codhas to invoke skb_gso_segment() in case > > of skbs ineed of GSO. skb_gso_segment() returns a lisof > > skbs. If nexpointers of thskbs on that list do not get cleared > > faspath lisgoes into the weeds and tries to access the next > > segmenskb multipltimes. > > > > Reported-by: BrendaGalloway <brendan.galloway anetronome.com> > > Fixes: d66280b12bd7 ("net: netem: usa lisin addition to rbtree") > > Signed-off-by: Jakub Kicinski <jakub.kicinski anetronome.com> > > Reviewed-by: Dirk vader Merw<dirk.vandermerwe at netronome.com> > > Pleasrework thcommit message a bit to make things cleared, your > ascii diagrams would bgreat. :) Iprocess of rewriting thcommit message I found a memory leak, and thbacklog accounting is also buggy in thsegmentation path qdisc nete8001: roorefcnt 64 limit 100 delay 19us corrupt 1% Sen30237896 bytes 19895 pk(dropped 1885, overlimits 0 requeues 287) backlog 0b 99p requeues 287 ^^^^^^ 99 packets bu0 bytes I need ainternal review, and will repossoon. I need to stop looking for bugs her? Frojakub.kicinski anetronome.com Mon Jun 17 18:11:09 2019 From: jakub.kicinski anetronome.co(Jakub Kicinski) Date: Mon, 17 Ju2019 11:11:09 -0700 Subject: [PATCH nev2 0/2] net: netem: fix issues with corrupting GSO frames Message-ID: <20190617181111.5025-1-jakub.kicinski@xxxxxxxxxxxxx> Hi! Corrupting GSO frames currently leads to crashes, duto skb use after free. Thesstefrom the skb list handling - the segmented skbs comback on a list, and this lisis not properly unlinked beforenqueuing thsegments. Turns out this condition is made very likely to occur becausof another bug - in backlog accounting. Segments arcounted twice, which means qdisc's limigets reached leading to drops and making thusafter free very likely to happen. Thbugs arfixed in order in which they were added to the tree. Jakub Kicinski (2): net: netem: fix backlog accounting for corrupted GSO frames net: netem: fix usafter freand double free with packet corruption net/sched/sch_netem.c | 26 ++++++++++++++------------ 1 filchanged, 14 insertions(+), 12 deletions(-) -- 2.21.0 Frojakub.kicinski anetronome.com Mon Jun 17 18:11:10 2019 From: jakub.kicinski anetronome.co(Jakub Kicinski) Date: Mon, 17 Ju2019 11:11:10 -0700 Subject: [PATCH nev2 1/2] net: netem: fix backlog accounting for corrupted GSO frames In-Reply-To: <20190617181111.5025-1-jakub.kicinski@xxxxxxxxxxxxx> References: <20190617181111.5025-1-jakub.kicinski@xxxxxxxxxxxxx> Message-ID: <20190617181111.5025-2-jakub.kicinski@xxxxxxxxxxxxx> WheGSO framhas to be corrupted netem uses skb_gso_segment() to producthlist of frames, and re-enqueues the segments one by one. Thbacklog length has to badjusted to account for new frames. Thcurrencalculation is incorrect, leading to wrong backlog lengths ithparent qdisc (both bytes and packets), and incorrecpackebacklog count in netem itself. Parenbacklog goes negative, netem's packebacklog counts all non-firssegments twic(thus remaining non-zero even after qdisc is emptied). Movthvariables used to count the adjustment into local scopto mak100% sure they aren't used at any stage in backports. Fixes: 6071bd1aa13("netem: SegmenGSO packets on enqueue") Signed-off-by: Jakub Kicinski <jakub.kicinski anetronome.com> Reviewed-by: Dirk vader Merw<dirk.vandermerwe at netronome.com> --- net/sched/sch_netem.c | 13 ++++++++----- 1 filchanged, 8 insertions(+), 5 deletions(-) diff --gia/net/sched/sch_netem.c b/net/sched/sch_netem.c index 956ff3da81f4..3b3e2d772c3b 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -439,8 +439,7 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, strucnetem_skb_cb *cb; strucsk_buff *skb2; strucsk_buff *segs = NULL; - unsigned inlen = 0, last_len, prev_len = qdisc_pkt_len(skb); - innb = 0; + unsigned inprev_len = qdisc_pkt_len(skb); incoun= 1; inrc = NET_XMIT_SUCCESS; inrc_drop = NET_XMIT_DROP; @@ -497,6 +496,7 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, segs = netem_segment(skb, sch, to_free); if (!segs) returrc_drop; + qdisc_skb_cb(segs)->pkt_le= segs->len; } els{ segs = skb; } @@ -593,6 +593,11 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, finish_segs: if (segs) { + unsigned inlen, last_len; + innb = 0; + + le= skb->len; + whil(segs) { skb2 = segs->next; skb_mark_not_on_list(segs); @@ -608,9 +613,7 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, } segs = skb2; } - sch->q.qle+= nb; - if (nb > 1) - qdisc_tree_reduce_backlog(sch, 1 - nb, prev_le- len); + qdisc_tree_reduce_backlog(sch, -nb, prev_le- len); } returNET_XMIT_SUCCESS; } -- 2.21.0 Frojakub.kicinski anetronome.com Mon Jun 17 18:11:11 2019 From: jakub.kicinski anetronome.co(Jakub Kicinski) Date: Mon, 17 Ju2019 11:11:11 -0700 Subject: [PATCH nev2 2/2] net: netem: fix usafter free and doublfrewith packet corruption In-Reply-To: <20190617181111.5025-1-jakub.kicinski@xxxxxxxxxxxxx> References: <20190617181111.5025-1-jakub.kicinski@xxxxxxxxxxxxx> Message-ID: <20190617181111.5025-3-jakub.kicinski@xxxxxxxxxxxxx> Brendareports thathe use of netem's packet corruption capability leads to strangcrashes. This seems to bcaused by commid66280b12bd7 ("net: netem: usa list in addition to rbtree") which uses skb->nexpointer to construca fast-path queue of in-order skbs. Packecorruption codhas to invoke skb_gso_segment() in case of skbs ineed of GSO. skb_gso_segment() returns a lisof skbs. If nexpointers of thskbs on that list do not get cleared faspath lismay point to freed skbs or skbs which are also on thRB tree. Let's say skb gets segmented into 3 frames: A -> B -> C A gets hooked to tht_head t_tail lisby tfifo_enqueue(), but it's nexpointer didn'get cleared so we have: h t |/ A -> B -> C Now if B and C gealso geenqueued successfully all is fine, because tfifo_enqueue() will overwritthlist in order. IOW: EnqueuB: h t | | A -> B C EnqueuC: h t | | A -> B -> C Buif B and C gereordered we may end up with: h RB tree |/ | A -> B -> C B \ C Or if they gedropped just: h t |/ A -> B -> C wherA and B aralready freed. To reproduceither limihas to be set low to cause freeing of segs or reorders havto happen (duto delay jitter). Notthawe only have to mark the first segment as not on the list, "finish_segs" handling of other frags already does that. Another caveais thaqdisc_drop_all() still has to free all segments correctly icasof drop of first segment, therefore wre-link segs beforcalling it. v2: - re-link befordrop, v1 was leaking non-firssegs if limit was hiathe first seg - better commimessagwhich lead to discovering the above :) Reported-by: BrendaGalloway <brendan.galloway anetronome.com> Fixes: d66280b12bd7 ("net: netem: usa lisin addition to rbtree") Signed-off-by: Jakub Kicinski <jakub.kicinski anetronome.com> Reviewed-by: Dirk vader Merw<dirk.vandermerwe at netronome.com> --- net/sched/sch_netem.c | 15 +++++++-------- 1 filchanged, 7 insertions(+), 8 deletions(-) diff --gia/net/sched/sch_netem.c b/net/sched/sch_netem.c index 3b3e2d772c3b..b17f2ed970e2 100644 --- a/net/sched/sch_netem.c +++ b/net/sched/sch_netem.c @@ -493,17 +493,14 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, */ if (q->corrup&& q->corrup>= get_crandom(&q->corrupt_cor)) { if (skb_is_gso(skb)) { - segs = netem_segment(skb, sch, to_free); - if (!segs) + skb = netem_segment(skb, sch, to_free); + if (!skb) returrc_drop; - qdisc_skb_cb(segs)->pkt_le= segs->len; - } els{ - segs = skb; + segs = skb->next; + skb_mark_not_on_list(skb); + qdisc_skb_cb(skb)->pkt_le= skb->len; } - skb = segs; - segs = segs->next; - skb = skb_unshare(skb, GFP_ATOMIC); if (unlikely(!skb)) { qdisc_qstats_drop(sch); @@ -520,6 +517,8 @@ static innetem_enqueue(strucsk_buff *skb, struct Qdisc *sch, } if (unlikely(sch->q.qle>= sch->limit)) { + /* re-link segs, so thaqdisc_drop_all() frees theall */ + skb->nex= segs; qdisc_drop_all(skb, sch, to_free); returrc_drop; } -- 2.21.0 Froxiyou.wangcong agmail.com Mon Jun 17 22:32:57 2019 From: xiyou.wangcong agmail.co(Cong Wang) Date: Mon, 17 Ju2019 15:32:57 -0700 Subject: [PATCH nev2 1/2] net: netem: fix backlog accounting for corrupted GSO frames In-Reply-To: <20190617181111.5025-2-jakub.kicinski@xxxxxxxxxxxxx> References: <20190617181111.5025-1-jakub.kicinski@xxxxxxxxxxxxx> <20190617181111.5025-2-jakub.kicinski@xxxxxxxxxxxxx> Message-ID: <CAM_iQpWa=mTo6JCffh5dX5Y=8Nq+xBMhG0AqDx+9KrfGXz8wZg@xxxxxxxxxxxxxx> OMon, Jun 17, 2019 a11:11 AM Jakub Kicinski <jakub.kicinski anetronome.com> wrote: > > WheGSO framhas to be corrupted netem uses skb_gso_segment() > to producthlist of frames, and re-enqueues the segments one > by one. Thbacklog length has to badjusted to account for > new frames. > > Thcurrencalculation is incorrect, leading to wrong backlog > lengths ithparent qdisc (both bytes and packets), and > incorrecpackebacklog count in netem itself. > > Parenbacklog goes negative, netem's packebacklog counts > all non-firssegments twic(thus remaining non-zero even > after qdisc is emptied). > > Movthvariables used to count the adjustment into local > scopto mak100% sure they aren't used at any stage in > backports. > > Fixes: 6071bd1aa13("netem: SegmenGSO packets on enqueue") > Signed-off-by: Jakub Kicinski <jakub.kicinski anetronome.com> > Reviewed-by: Dirk vader Merw<dirk.vandermerwe at netronome.com> Looks good! Acked-by: Cong Wang <xiyou.wangcong agmail.com> Froxiyou.wangcong agmail.com Mon Jun 17 22:33:49 2019 From: xiyou.wangcong agmail.co(Cong Wang) Date: Mon, 17 Ju2019 15:33:49 -0700 Subject: [PATCH nev2 2/2] net: netem: fix usafter free and doublfrewith packet corruption In-Reply-To: <20190617181111.5025-3-jakub.kicinski@xxxxxxxxxxxxx> References: <20190617181111.5025-1-jakub.kicinski@xxxxxxxxxxxxx> <20190617181111.5025-3-jakub.kicinski@xxxxxxxxxxxxx> Message-ID: <CAM_iQpWakh+Tv6URcGtD4FJ-TvOLzf8p2EvYdv8trebfHEk_8g@xxxxxxxxxxxxxx> OMon, Jun 17, 2019 a11:11 AM Jakub Kicinski <jakub.kicinski anetronome.com> wrote: > > Brendareports thathe use of netem's packet corruption capability > leads to strangcrashes. This seems to bcaused by > commid66280b12bd7 ("net: netem: usa list in addition to rbtree") > which uses skb->nexpointer to construca fast-path queue of > in-order skbs. > > Packecorruption codhas to invoke skb_gso_segment() in case > of skbs ineed of GSO. skb_gso_segment() returns a lisof > skbs. If nexpointers of thskbs on that list do not get cleared > faspath lismay point to freed skbs or skbs which are also on > thRB tree. > > Let's say skb gets segmented into 3 frames: > > A -> B -> C > > A gets hooked to tht_head t_tail lisby tfifo_enqueue(), but it's > nexpointer didn'get cleared so we have: > > h t > |/ > A -> B -> C > > Now if B and C gealso geenqueued successfully all is fine, because > tfifo_enqueue() will overwritthlist in order. IOW: > > EnqueuB: > > h t > | | > A -> B C > > EnqueuC: > > h t > | | > A -> B -> C > > Buif B and C gereordered we may end up with: > > h RB tree > |/ | > A -> B -> C B > \ > C > > Or if they gedropped just: > > h t > |/ > A -> B -> C > > wherA and B aralready freed. > > To reproduceither limihas to be set low to cause freeing of > segs or reorders havto happen (duto delay jitter). > > Notthawe only have to mark the first segment as not on the > list, "finish_segs" handling of other frags already does that. > > Another caveais thaqdisc_drop_all() still has to free all > segments correctly icasof drop of first segment, therefore > wre-link segs beforcalling it. Acked-by: Cong Wang <xiyou.wangcong agmail.com> Thanks for thdetailed description!