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

On 05/12/2023 08:32, Jiri Pirko wrote:
> Mon, Dec 04, 2023 at 09:39:06PM CET, pctammela@...atatu.com wrote:
>> As of today tc-action events are unconditionally built and sent to
>> RTNLGRP_TC. As with the introduction of tc_should_notify we can check
>> before-hand if they are really needed.
>>
>> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
>> ---
>> net/sched/act_api.c | 105 ++++++++++++++++++++++++++++++++------------
>> 1 file changed, 76 insertions(+), 29 deletions(-)
>>
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index c39252d61ebb..55c62a8e8803 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -1780,31 +1780,45 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
>> 	return 0;
>> }
>>
>> -static int
>> -tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
>> +static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net,
> 
> I wonder, why this new function is needed? If I'm reading things
> correctly, tcf_reoffload_del_notify() with added check would be just ok,
> woundn't it?
> 
> Same for others.

In V1 we had it like what you are suggesting[1].
Jakub suggested to refactor the functions a bit more. The main argument 
was the code duplication introduced for the delete routines.
Note that for the case that no notification is needed, we still need to 
do the action delete etc...
I agree that code duplication is bad in the long term, so I did the 
changes, but I don't have a strong opinion here (either way is fine for me).
Let's see what he has to say, perhaps I overdid what he was suggesting :)

[1] 
https://lore.kernel.org/all/20231201204314.220543-4-pctammela@mojatatu.com/

> 
>> +						    struct tc_action *action)
>> {
>> 	size_t attr_size = tcf_action_fill_size(action);
>> 	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
>> 		[0] = action,
>> 	};
>> -	const struct tc_action_ops *ops = action->ops;
>> 	struct sk_buff *skb;
>> -	int ret;
>>
>> -	skb = alloc_skb(attr_size <= NLMSG_GOODSIZE ? NLMSG_GOODSIZE : attr_size,
>> -			GFP_KERNEL);
>> +	skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL);
> 
> I don't see how this is related to this patch. Can't you do it in separate
> patch?
> 
> Same for others.

Sure, will split it out.

> 
>> 	if (!skb)
>> -		return -ENOBUFS;
>> +		return ERR_PTR(-ENOBUFS);
>>
>> 	if (tca_get_fill(skb, actions, 0, 0, 0, RTM_DELACTION, 0, 1, NULL) <= 0) {
>> 		kfree_skb(skb);
>> -		return -EINVAL;
>> +		return ERR_PTR(-EINVAL);
>> 	}
>>
>> +	return skb;
>> +}
>> +
> 
> [...]