netdev - Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason more flexible for remaining qdiscs

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 14 Dec 2023 10:31:24 -0500
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Jamal Hadi Salim <hadi@...atatu.com>, Eric Dumazet <edumazet@...gle.com>, 
	Victor Nogueira <victor@...atatu.com>, xiyou.wangcong@...il.com, jiri@...nulli.us, 
	davem@...emloft.net, pabeni@...hat.com, daniel@...earbox.net, 
	dcaratti@...hat.com, netdev@...r.kernel.org, kernel@...atatu.com, 
	Taehee Yoo <ap420073@...il.com>
Subject: Re: [PATCH net-next v3 2/3] net: sched: Make tc-related drop reason
 more flexible for remaining qdiscs

On Wed, Dec 13, 2023 at 4:08 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 13 Dec 2023 13:36:31 -0500 Jamal Hadi Salim wrote:
> > Putting this to rest:
> > Other than fq codel, the others that deal with multiple skbs due to
> > gso segments. So the conclusion is:  if we have a bunch in the list
> > then they all suffer the same fate. So a single reason for the list is
> > sufficient.
>
> Alright.
>
> I'm still a bit confused about the cb, tho.
>
> struct qdisc_skb_cb is the state struct.

Per packet state within tc though, no? Once it leaves tc whatever sits
in that space cant be trusted to be valid.
To answer your earlier question tcf_result is not available at the
qdisc level (when we call free_skb_list() but cb is and thats why we
used it)

> But we put the drop reason in struct tc_skb_cb.
> How does that work. Qdiscs will assume they own all of
> qdisc_skb_cb::data ?
>

Short answer, yes. Anyone can scribble over that. And multiple
consumers have a food fight going on - but it is expected behavior:
ebpf's skb->cb, cake, fq_codel etc - all use qdisc_skb_cb::data.
Out of the 48B in skb->cb qdisc_skb_cb redefined the first 28B and
left in qdisc_skb_cb::data as free-for-all space. I think,
unfortunately, that is now cast in stone.
Which still leaves us 20 bytes which is now being squatered by
tc_skb_cb where the drop reason was placed. Shit, i just noticed this
is not exclusive - seems like
drivers/net/amt.c is using this space - not sure why it is even using
tc space. I am wondering if we can make it use the 20B scratch pad.
+Cc author Taehee Yoo - it doesnt seem to conflict but strange that it
is considering qdisc_skb_cb

> Maybe some documentation about the lifetimes of these things
> would clarify things?

What text do you think makes sense and where should it go?

cheers,
jamal