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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20240830114851.58cd02c4@kernel.org>
Date: Fri, 30 Aug 2024 11:48:51 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
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 Fri, 30 Aug 2024 18:48:41 +0200 Paolo Abeni wrote:
> >> +	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?

Alright. But TBH I haven't grasped the semantics of how you use UNSPEC.
So I reserve the right to complain again in v6 if I think of a better
way ;)

> >> +	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.

net_shaper_group_send_reply() does a bit too much, TBH.
Given the rollback complexity propagating the failure just
from genlmsg_reply() is fine. I think the only case it fails
is if the socket is congested, which is in senders control.
But genlmsg_new() can be done before we start. And we should 
size the skb so that nla_puts sever fail (just pop a WARN_ON()
on their error path, to make sure we catch it if they grow,
I don't think they can fail with your current code).