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 15:39:53 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Daniel Borkmann <daniel@...earbox.net>
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 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.

cheers,
jamal

> Thanks,
> Daniel