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: <6a70805b-953d-494e-a9b9-a2cb0e0aff18@mojatatu.com>
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;
>> +}
>> +
> 
> [...]


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ