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: Thu, 7 Dec 2023 20:56:50 +0000
From: Simon Horman <horms@...nel.org>
To: Pedro Tammela <pctammela@...atatu.com>
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 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?

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
>