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] [day] [month] [year] [list]

Date: Fri, 6 Oct 2023 21:59:08 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: Jakub Kicinski <kuba@...nel.org>, 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 9:39 PM, Jamal Hadi Salim wrote:
> On Fri, Oct 6, 2023 at 11:45 AM Daniel Borkmann <daniel@...earbox.net> wrote:
>> 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.
> 
> I think this should work - either Victor or myself will work on a followup.

Sounds great, thanks!

Cheers,
Daniel