| 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
| ||
|
Message-ID: <20240829190445.7bb3a569@kernel.org>
Date: Thu, 29 Aug 2024 19:04:45 -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 Thu, 29 Aug 2024 17:16:57 +0200 Paolo Abeni wrote:
> Allow grouping multiple leaves shaper under the given root.
> The root and the leaves shapers are created, if needed, otherwise
> the existing shapers are re-linked as requested.
>
> Try hard to pre-allocated the needed resources, to avoid non
> trivial H/W configuration rollbacks in case of any failure.
Need to s/root/parent/ the commit message?
> +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?
> + return -ENOENT;
> + }
> +
> + /* When unspecified, the node parent scope is inherited from
> + * the leaves.
> + */
> + if (node->parent.scope == NET_SHAPER_SCOPE_UNSPEC) {
> + for (i = 1; i < leaves_count; ++i) {
> + if (leaves[i].parent.scope !=
> + leaves[0].parent.scope ||
> + leaves[i].parent.id !=
> + leaves[0].parent.id) {
memcmp() ? put a BUILD_BUG_ON(sizeof() != 8) to make sure we double
check it if the struct grows?
> + NL_SET_ERR_MSG_FMT(extack, "All the leaves shapers must have the same old parent");
> + return -EINVAL;
5 indents is too many indents :( maybe make the for loop a helper?
> + }
> + }
> +
> + if (leaves_count > 0)
how can we get here and not have leaves? :o
> + node->parent = leaves[0].parent;
> + }
> +
> + } else {
> + net_shaper_default_parent(node_handle, &node->parent);
> + }
> +static int net_shaper_group_send_reply(struct genl_info *info,
> + struct net_shaper_handle *handle)
> +{
> + struct net_shaper_binding *binding = info->user_ptr[0];
> + struct sk_buff *msg;
> + int ret = -EMSGSIZE;
> + void *hdr;
> +
> + /* Prepare the msg reply in advance, to avoid device operation
> + * rollback.
> + */
> + msg = genlmsg_new(net_shaper_handle_size(), GFP_KERNEL);
> + if (!msg)
> + return ret;
return -ENOMEM;
> +
> + hdr = genlmsg_iput(msg, info);
> + if (!hdr)
> + goto free_msg;
> +
> + if (net_shaper_fill_binding(msg, binding, NET_SHAPER_A_IFINDEX))
> + goto free_msg;
> +
> + if (net_shaper_fill_handle(msg, handle, NET_SHAPER_A_HANDLE))
you can combine the two fill ifs into one with ||
> + goto free_msg;
> +
> + genlmsg_end(msg, hdr);
> +
> + ret = genlmsg_reply(msg, info);
> + if (ret)
> + goto free_msg;
reply always eats the skb, just:
return genlmsg_reply(msg, info);
> +
> + return ret;
> +
> +free_msg:
> + nlmsg_free(msg);
> + return ret;
return -EMSGSIZE;
> +}
> +
> +int net_shaper_nl_group_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct net_shaper_handle *leaves_handles, node_handle;
> + struct net_shaper_info *leaves, node;
> + struct net_shaper_binding *binding;
> + int i, ret, rem, leaves_count;
> + struct nlattr *attr;
> +
> + if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_LEAVES) ||
> + GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_NODE))
> + return -EINVAL;
> +
> + binding = net_shaper_binding_from_ctx(info->user_ptr[0]);
> + leaves_count = net_shaper_list_len(info, NET_SHAPER_A_LEAVES);
> + leaves = kcalloc(leaves_count, sizeof(struct net_shaper_info) +
> + sizeof(struct net_shaper_handle), GFP_KERNEL);
> + if (!leaves) {
> + GENL_SET_ERR_MSG_FMT(info, "Can't allocate memory for %d leaves shapers",
> + leaves_count);
> + return -ENOMEM;
> + }
> + leaves_handles = (struct net_shaper_handle *)&leaves[leaves_count];
> +
> + ret = net_shaper_parse_node(binding, info->attrs[NET_SHAPER_A_NODE],
> + info, &node_handle, &node);
> + if (ret)
> + goto free_shapers;
> +
> + i = 0;
> + nla_for_each_attr_type(attr, NET_SHAPER_A_LEAVES,
> + genlmsg_data(info->genlhdr),
> + genlmsg_len(info->genlhdr), rem) {
> + if (WARN_ON_ONCE(i >= leaves_count))
> + goto free_shapers;
> +
> + ret = net_shaper_parse_info_nest(binding, attr, info,
> + NET_SHAPER_SCOPE_QUEUE,
> + &leaves_handles[i],
Wouldn't it be convenient to store the handle in the "info" object?
AFAIU the handle is forever for an info, so no risk of it being out
of sync...
> + &leaves[i]);
> + if (ret)
> + goto free_shapers;
> + i++;
> + }
> +
> + ret = net_shaper_group(binding, leaves_count, leaves_handles, leaves,
> + &node_handle, &node, info->extack);
...and it'd be nice if group had 5 rather than 7 params
> + 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.
> + */
> + GENL_SET_ERR_MSG_FMT(info, "Can't send reply %d", ret);
> + ret = 0;
> + }
> +
> +free_shapers:
> + kfree(leaves);
> + return ret;
> +}
Powered by blists - more mailing lists