netdev - Re: [ovs-dev] [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]

Message-ID: <CAG=2xmNXJyyTtbEMDT149pZSv75birHYAH67oC+xc6jpxtQjDw@mail.gmail.com>
Date: Tue, 2 Jul 2024 18:37:52 +0000
From: Adrián Moreno <amorenoz@...hat.com>
To: Simon Horman <horms@...nel.org>
Cc: Ilya Maximets <i.maximets@....org>, dev@...nvswitch.org, 
	Donald Hunter <donald.hunter@...il.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [ovs-dev] [PATCH net-next v7 05/10] net: openvswitch: add psample action

On Tue, Jul 02, 2024 at 01:33:01PM GMT, Simon Horman wrote:
> On Tue, Jul 02, 2024 at 11:53:01AM +0200, Ilya Maximets wrote:
> > 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.
>
> Hi Ilya,
>
> I guess I would lean towards the last option.
> But in any case, thanks for pointing out that this is complex:
> I had not realised that when I wrote my previous email.
>

In that case this version (v7) should be good to go? Are there any other
comments? Or is v8 preferred (the only change is between them is
dropping the "inline")?

Thanks.
Adrián