netdev - Re: [PATCH net-next v3 4/5] net/sched: act_api: conditional notification of events

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, 8 Dec 2023 11:21:43 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, jhs@...atatu.com,
 xiyou.wangcong@...il.com, jiri@...nulli.us, marcelo.leitner@...il.com,
 vladbu@...dia.com
Subject: Re: [PATCH net-next v3 4/5] net/sched: act_api: conditional
 notification of events

On 07/12/2023 17:56, Simon Horman wrote:
> On Wed, Dec 06, 2023 at 01:44:15PM -0300, Pedro Tammela wrote:
>> As of today tc-action events are unconditionally built and sent to
>> RTNLGRP_TC. As with the introduction of rtnl_notify_needed we can check
>> before-hand if they are really needed.
>>
>> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
> 
> Hi Pedro,
> 
> a nice optimisation :)
> 
> ...
> 
>> +static int tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
>> +{
>> +	const struct tc_action_ops *ops = action->ops;
>> +	struct sk_buff *skb = NULL;
>> +	int ret;
>> +
>> +	if (!rtnl_notify_needed(net, 0, RTNLGRP_TC))
>> +		goto skip_msg;
> 
> Is there a reason (performance?) to use a goto here
> rather than putting the tcf_reoffload_del_notify_msg() call inside
> an if condition?

Not really, seems like the goto mosquito bit me on this one.
I will move to your suggestion and do the change Jiri asked which I 
totally forgot in v3.

> 
> Completely untested!
> 
> 	if (!rtnl_notify_needed(net, 0, RTNLGRP_TC)) {
> 		skb = NULL;
> 	} else {
> 		skb = tcf_reoffload_del_notify_msg(net, action);
> 		if (IS_ERR(skb))
> 			return PTR_ERR(skb);
> 	}
> 
> Or perhaps a helper, as this pattern seems to also appear in tcf_add_notify() >
> 
>> +
>> +	skb = tcf_reoffload_del_notify_msg(net, action);
>> +	if (IS_ERR(skb))
>> +		return PTR_ERR(skb);
>> +
>> +skip_msg:
>>   	ret = tcf_idr_release_unsafe(action);
>>   	if (ret == ACT_P_DELETED) {
>>   		module_put(ops->owner);
>> -		ret = rtnetlink_send(skb, net, 0, RTNLGRP_TC, 0);
>> +		ret = rtnetlink_maybe_send(skb, net, 0, RTNLGRP_TC, 0);
>>   	} else {
>>   		kfree_skb(skb);
>>   	}
> 
> ...
> 
>> +static int tcf_add_notify(struct net *net, struct nlmsghdr *n,
>> +			  struct tc_action *actions[], u32 portid,
>> +			  size_t attr_size, struct netlink_ext_ack *extack)
>> +{
>> +	struct sk_buff *skb = NULL;
>> +
>> +	if (!rtnl_notify_needed(net, n->nlmsg_flags, RTNLGRP_TC))
>> +		goto skip_msg;
>> +
>> +	skb = tcf_add_notify_msg(net, n, actions, portid, attr_size, extack);
>> +	if (IS_ERR(skb))
>> +		return PTR_ERR(skb);
>> +
>> +skip_msg:
>> +	return rtnetlink_maybe_send(skb, net, portid, RTNLGRP_TC,
>> +				    n->nlmsg_flags & NLM_F_ECHO);
>>   }
>>   
>>   static int tcf_action_add(struct net *net, struct nlattr *nla,
>> -- 
>> 2.40.1
>>