linux-kernel - Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

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: Tue, 2 Jul 2024 11:53:01 +0200
From: Ilya Maximets <i.maximets@....org>
To: Simon Horman <horms@...nel.org>, Adrián Moreno
 <amorenoz@...hat.com>
Cc: i.maximets@....org, Aaron Conole <aconole@...hat.com>,
 netdev@...r.kernel.org, echaudro@...hat.com, dev@...nvswitch.org,
 Donald Hunter <donald.hunter@...il.com>, Jakub Kicinski <kuba@...nel.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Pravin B Shelar <pshelar@....org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v7 05/10] net: openvswitch: add psample action

On 7/2/24 11:37, Simon Horman wrote:
> On Tue, Jul 02, 2024 at 03:05:02AM -0400, Adrián Moreno wrote:
>> On Mon, Jul 01, 2024 at 02:23:12PM GMT, Aaron Conole wrote:
>>> Adrian Moreno <amorenoz@...hat.com> writes:
> 
> ...
> 
>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> 
> ...
> 
>>>> @@ -1299,6 +1304,39 @@ static int execute_dec_ttl(struct sk_buff *skb, struct sw_flow_key *key)
>>>>  	return 0;
>>>>  }
>>>>
>>>> +#if IS_ENABLED(CONFIG_PSAMPLE)
>>>> +static void execute_psample(struct datapath *dp, struct sk_buff *skb,
>>>> +			    const struct nlattr *attr)
>>>> +{
>>>> +	struct psample_group psample_group = {};
>>>> +	struct psample_metadata md = {};
>>>> +	const struct nlattr *a;
>>>> +	int rem;
>>>> +
>>>> +	nla_for_each_attr(a, nla_data(attr), nla_len(attr), rem) {
>>>> +		switch (nla_type(a)) {
>>>> +		case OVS_PSAMPLE_ATTR_GROUP:
>>>> +			psample_group.group_num = nla_get_u32(a);
>>>> +			break;
>>>> +
>>>> +		case OVS_PSAMPLE_ATTR_COOKIE:
>>>> +			md.user_cookie = nla_data(a);
>>>> +			md.user_cookie_len = nla_len(a);
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	psample_group.net = ovs_dp_get_net(dp);
>>>> +	md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
>>>> +	md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
>>>> +
>>>> +	psample_sample_packet(&psample_group, skb, 0, &md);
>>>> +}
>>>> +#else
>>>> +static inline void execute_psample(struct datapath *dp, struct sk_buff *skb,
>>>> +				   const struct nlattr *attr) {}
>>>
>>> I noticed that this got flagged in patchwork since it is 'static inline'
>>> while being part of a complete translation unit - but I also see some
>>> other places where that has been done.  I guess it should be just
>>> 'static' though.  I don't feel very strongly about it.
>>>
>>
>> We had a bit of discussion about this with Ilya. It seems "static
>> inline" is a common pattern around the kernel. The coding style
>> documentation says:
>> "Generally, inline functions are preferable to macros resembling functions."
>>
>> So I think this "inline" is correct but I might be missing something.
> 
> Hi Adrián,
> 
> TL;DR: Please remove this inline keyword
> 
> For Kernel networking code at least it is strongly preferred not
> to use inline in .c files unless there is a demonstrable - usually
> performance - reason to do so. Rather, it is preferred to let the
> compiler decide when to inline such functions. OTOH, the inline
> keyword in .h files is fine.

FWIW, the main reason for 'inline' here is not performance, but silencing
compiler's potential 'maybe unused' warnings:

 Function-like macros with unused parameters should be replaced by static
 inline functions to avoid the issue of unused variables

I think, the rule for static inline functions in .c files is at odds with
the 'Conditional Compilation' section of coding style.  The section does
recommend to avoid conditional function declaration in .c files, but I'm not
sure it is reasonable to export internal static functions for that reason.

In this particular case we can either define a macro, which is discouraged
by the coding style:

 Generally, inline functions are preferable to macros resembling functions.

Or create a static inline function, that is against rule of no static
inline functions in .c files.

Or create a simple static function and mark all the arguments as unused,
which kind of compliant to the coding style, but the least pretty.

Best regards, Ilya Maximets.