netdev - Re: [PATCH v5 net-next 04/12] net-shapers: implement NL group operation

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d0244464-0596-4309-89ff-d8dcd9aa3d35@redhat.com>
Date: Fri, 30 Aug 2024 18:48:41 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
 Madhu Chittim <madhu.chittim@...el.com>,
 Sridhar Samudrala <sridhar.samudrala@...el.com>,
 Simon Horman <horms@...nel.org>, John Fastabend <john.fastabend@...il.com>,
 Sunil Kovvuri Goutham <sgoutham@...vell.com>,
 Jamal Hadi Salim <jhs@...atatu.com>, Donald Hunter
 <donald.hunter@...il.com>, anthony.l.nguyen@...el.com,
 przemyslaw.kitszel@...el.com, intel-wired-lan@...ts.osuosl.org,
 edumazet@...gle.com
Subject: Re: [PATCH v5 net-next 04/12] net-shapers: implement NL group
 operation

On 8/30/24 04:04, Jakub Kicinski wrote:
>> +static int __net_shaper_group(struct net_shaper_binding *binding,
>> +			      int leaves_count,
>> +			      const struct net_shaper_handle *leaves_handles,
>> +			      struct net_shaper_info *leaves,
>> +			      struct net_shaper_handle *node_handle,
>> +			      struct net_shaper_info *node,
>> +			      struct netlink_ext_ack *extack)
>> +{
>> +	const struct net_shaper_ops *ops = net_shaper_binding_ops(binding);
>> +	struct net_shaper_info *parent = NULL;
>> +	struct net_shaper_handle leaf_handle;
>> +	int i, ret;
>> +
>> +	if (node_handle->scope == NET_SHAPER_SCOPE_NODE) {
>> +		if (node_handle->id != NET_SHAPER_ID_UNSPEC &&
>> +		    !net_shaper_cache_lookup(binding, node_handle)) {
>> +			NL_SET_ERR_MSG_FMT(extack, "Node shaper %d:%d does not exists",
>> +					   node_handle->scope, node_handle->id);
> 
> BAD_ATTR would do?

We can reach here from the delete() op (next patch), there will be no 
paired attribute is such case. Even for the group() operation it will 
need to push here towards several callers additional context to identify 
the attribute, it should be quite ugly, can we keep with ERR_MSG_FMT here?

>> +	if (ret < 0)
>> +		goto free_shapers;
>> +
>> +	ret = net_shaper_group_send_reply(info, &node_handle);
>> +	if (ret) {
>> +		/* Error on reply is not fatal to avoid rollback a successful
>> +		 * configuration.
> 
> Slight issues with the grammar here, but I think it should be fatal.
> The sender will most likely block until they get a response.
> Not to mention that the caller will not know what the handle
> we allocated is.

You mean we should return a negative error code, and _not_ that we 
should additionally attempt a rollback, right? The rollback will be very 
difficult at best: at this point destructive action have taken place.

Thanks,

Paolo