netdev - Re: [PATCH net-next 1/1] net/sched: Disambiguate verdict from return code

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 for Android: free password hash cracker in your pocket

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

Message-ID: <686dd999-bee4-ecf8-8dc4-c85a098c4a92@iogearbox.net>
Date: Fri, 6 Oct 2023 15:49:18 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, Victor Nogueira
 <victor@...atatu.com>, xiyou.wangcong@...il.com, jiri@...nulli.us,
 davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 paulb@...dia.com, netdev@...r.kernel.org, kernel@...atatu.com,
 martin.lau@...ux.dev, bpf@...r.kernel.org
Subject: Re: [PATCH net-next 1/1] net/sched: Disambiguate verdict from return
 code

On 10/6/23 3:32 PM, Jakub Kicinski wrote:
> On Fri, 6 Oct 2023 13:18:40 +0200 Daniel Borkmann wrote:
>> That should be possible with some work this way, agree. I've been toying a bit
>> more on this issue, and actually there is an even better way which would cleanly
>> solve all use cases and we likely would utilize it for bpf as well in future.
>> I wasn't aware of it before, but the drop reason actually has per-subsystem infra
>> already which so far only mac80211 and ovs makes use of.
> 
> FWIW I'm not sure if leaning into the subsys specific error codes for
> something as core as TC is a good direction. I'd think that what
> matters to the user is was it an intentional policy drop or some form
> of an error / exception. More detailed info can come from stats.
> 
> Maybe I'm overly conservative because I don't care about debugging
> mac80211 or ovs but do very much care about TC :) And I think Alastair
> (bpftrace) is working on auto-prettifying enums when bpftrace outputs
> maps. So we can do something like:
> 
> $ bpftrace -e 'tracepoint:skb:kfree_skb { @[args->reason] = count(); }'
> Attaching 1 probe...
> ^C
> 
> @[SKB_DROP_REASON_TC_INGRESS]: 2
> @[SKB_CONSUMED]: 34
> 
>    ^^^^^^^^^^^^ names!!
> 
> Auto-magically.

Very cool!

> Which will no longer work with the "pack multiple values into
> the reason" scheme of subsys-specific values :(

Too bad, do you happen to know why it won't work? Given they went into the
length of extending this for subsystems, they presumably would also like to
benefit from above. :/

> What I'm saying is that there is a trade-off here between providing
> as much info as possible vs basic user getting intelligible data..

Makes sense. I think we can drop that aspect for the subsys specific error
codes. Fwiw, TCP has 22 drop codes in the core section alone, so this should
be fine if you think it's better. The rest of the patch shown should still
apply the same way. I can tweak it to use the core section for codes, and
then it can be successively extended if that looks good to you - unless you
are saying from above, that just one error code is better and then going via
detailed stats for specific errors is preferred.

Thanks,
Daniel