lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <3660fc5b-5cb3-61ee-a10c-0f541282eba4@gmail.com> Date: Wed, 12 Oct 2022 13:41:38 -0700 From: Eric Dumazet <eric.dumazet@...il.com> To: Thorsten Glaser <t.glaser@...ent.de>, Dave Taht <dave.taht@...il.com> Cc: netdev@...r.kernel.org Subject: Re: RFH, where did I go wrong? On 10/12/22 13:17, Thorsten Glaser wrote: > Dixi quod… > >>> I'll take a harder look, but does it crash if you rip out debugfs? > […] >> And yes, it (commit dbb99579808dcf106264f28f3c8cf5ef2f2c05bf) still >> crashes even if this time I get yet another message… all of those I > I may have found the case by reducing further. Disabling the periodic > dropping of too-old packets wasn’t it, but apparently, the code now > guarded by JANZ_HEADDROP is it. Replacing it (dropping the oldest > packet returning NET_XMIT_CN) with trivial code that rejects the new > packet-to-be-enqueued with qdisc_drop() instead… seems to not crash. > > So, the code in question that seems to introduce the crash is: > > > u32 prev_backlog = sch->qstats.backlog; > //… normal code to add the passed skb (timestamp, etc.) > // q->memusage += cb->truesz; > if (unlikely(overlimit = (++sch->q.qlen > sch->limit))) { > struct sk_buff *skbtodrop; > /* skbtodrop = head of FIFO and remove it from the FIFO */ > skbtodrop = q->q[1].first; > if (!(q->q[1].first = skbtodrop->next)) > q->q[1].last = NULL; > --sch->q.qlen; > /* accounting */ > q->memusage -= get_janz_skb(skbtodrop)->truesz; > qdisc_qstats_backlog_dec(sch, skbtodrop); > /* free the skb */ > rtnl_kfree_skbs(skbtodrop, skbtodrop) This looks wrong (although I have not read your code) I guess RTNL is not held at this point. Use kfree_skb(skb) or __qdisc_drop(skb, to_free) > } > //… normal code to add: > // line 879-885 enqueue into the FIFO > // qdisc_qstats_backlog_inc(sch, skb); > //… now code protected by the flag again: > if (unlikely(overlimit)) { > qdisc_qstats_overlimit(sch); > qdisc_tree_reduce_backlog(sch, 0, > prev_backlog - sch->qstats.backlog); > return (NET_XMIT_CN); > } > // normal code remaining: return (NET_XMIT_SUCCESS); > > > This *seems* pretty straightforward to me, given similar code > in other qdiscs, and what I could learn from their header and > implementation. > > TIA, > //mirabilos
Powered by blists - more mailing lists