netdev - Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc

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]

Message-ID: <d2455b9f-78a3-81fe-46d5-c7aa03879294@mojatatu.com>
Date:   Fri, 2 Dec 2022 09:48:49 -0300
From:   Pedro Tammela <pctammela@...atatu.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com, jhs@...atatu.com, xiyou.wangcong@...il.com,
        jiri@...nulli.us, kuniyu@...zon.com
Subject: Re: [PATCH net-next v2 1/3] net/sched: add retpoline wrapper for tc

On 01/12/2022 19:38, Jakub Kicinski wrote:
> On Thu, 1 Dec 2022 13:40:34 -0300 Pedro Tammela wrote:
>>>> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
>>>> +			   struct tcf_result *res)
>>>> +{
>>>> +	if (0) { /* noop */ }
>>>> +#if IS_BUILTIN(CONFIG_NET_ACT_BPF)
>>>> +	else if (a->ops->act == tcf_bpf_act)
>>>> +		return tcf_bpf_act(skb, a, res);
>>>> +#endif
>>>
>>> How does the 'else if' ladder compare to a switch statement?
>>
>> It's the semantically the same, we would just need to do some casts to
>> unsigned long.
> 
> Sorry, should've been clearer, I mean in terms of generated code.
> Is the machine code identical / better / worse?
> 
>> WDYT about the following?
>>
>>     #define __TC_ACT_BUILTIN(builtin, fname) \
>>        if (builtin && a->ops->act == fname) return fname(skb, a, res)
>>
>>     #define _TC_ACT_BUILTIN(builtin, fname) __TC_ACT_BUILTIN(builtin, fname)
>>     #define TC_ACT_BUILTIN(cfg, fname)  _TC_ACT_BUILTIN(IS_BUILTIN(cfg),
>> fname)
>>
>>     static inline int __tc_act(struct sk_buff *skb, const struct
>> tc_action *a,
>>                                struct tcf_result *res)
>>     {
>>             TC_ACT_BUILTIN(CONFIG_NET_ACT_BPF, tcf_bpf_act);
>>     ...
>>
>> It might be more pleasant to the reader.
> 
> Most definitely not to this reader :) The less macro magic the better.
> I'm primarily curious about whether the compiler treats this sort of
> construct the same as a switch.

Apparently we can't do a switch case in pointer addresses because the 
case condition must be constant.

> 
>>>> +#ifdef CONFIG_NET_CLS_ACT
>>>> +static inline int __tc_act(struct sk_buff *skb, const struct tc_action *a,
>>>> +			   struct tcf_result *res)
>>>> +{
>>>> +	return a->ops->act(skb, a, res);
>>>> +}
>>>> +#endif
>>>> +
>>>> +#ifdef CONFIG_NET_CLS
>>>> +static inline int __tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>>>> +				struct tcf_result *res)
>>>> +{
>>>> +	return tp->classify(skb, tp, res);
>>>> +}
>>>> +#endif
>>>
>>> please don't wrap the static inline helpers in #ifdefs unless it's
>>> actually necessary for build to pass.
>>
>> The only one really needed is CONFIG_NET_CLS_ACT because the struct
>> tc_action definition is protected by it. Perhaps we should move it out
>> of the #ifdef?
> 
> Yes, I think that's a nice cleanup.