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]

Date:   Thu, 1 Dec 2022 13:40:34 -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 02:16, Jakub Kicinski wrote:
> On Mon, 28 Nov 2022 12:44:54 -0300 Pedro Tammela wrote:
>> On kernels compiled with CONFIG_RETPOLINE and CONFIG_NET_TC_INDIRECT_WRAPPER,
>> optimize actions and filters that are compiled as built-ins into a direct call.
>> The calls are ordered alphabetically, but new ones should be ideally
>> added last.
>>
>> On subsequent patches we expose the classifiers and actions functions
>> and wire up the wrapper into tc.
> 
>> +#if IS_ENABLED(CONFIG_RETPOLINE) && IS_ENABLED(CONFIG_NET_TC_INDIRECT_WRAPPER)
> 
> The latter 'depends on' former, so just check the latter.
> 
>> +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.
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.

> 
>> +#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?