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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Fri, 6 Oct 2023 17:45:40 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Jamal Hadi Salim <jhs@...atatu.com>, Jakub Kicinski <kuba@...nel.org>
Cc: 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 5:25 PM, Jamal Hadi Salim wrote:
> On Fri, Oct 6, 2023 at 10:12 AM Jakub Kicinski <kuba@...nel.org> wrote:
>> On Fri, 6 Oct 2023 15:49:18 +0200 Daniel Borkmann wrote:
>>>> 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?
>>
>> I'm just guessing but the reason is enum skb_drop_reason
>> and the values of subsystem specific reasons won't be part
>> of that enum.
> 
> IIUC, this would gives us the readability and never require any
> changes to bpftrace, whereas the major:minor encoding would require
> further logic in bpftrace.

Makes sense, agree.

>>> 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.
>>
>> No, no, multiple reasons are perfectly fine. The non-technical
>> advantage of mac80211 error codes being separate is that there
>> are no git conflicts when we add new ones. TC codes can just
>> be added to the main enum like TCP 🤷️
> 
> We still need to differentiate policy vs error - I suppose we could go
> with Daniel's idea of introducing TC_ACT_ABORT/ERROR and ensure all
> the callees set the drop_reason.

I've simplified the set (attached). The disambiguation could eventually be on
SKB_DROP_REASON_TC_{INGRESS,EGRESS} == intentional drop vs SKB_DROP_REASON_TC_ERROR_*
which indicates an internal error code once these are covered on all locations.
There could probably also be just a SKB_DROP_REASON_TC_ERROR which could act as
a catch-all for the time being to initially mark all error locations with something
generic. I think this should be flexible where you wouldn't need extra TC_ACT_ABORT.

Thanks,
Daniel

View attachment "0001-net-tc-Make-tc-related-drop-reason-more-flexible.patch" of type "text/x-patch" (6571 bytes)

View attachment "0002-net-tc-Add-tc_set_drop_reason-for-reclassify-limit.patch" of type "text/x-patch" (1909 bytes)