Re: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 29 Aug 2024 17:16:55 +0200 Paolo Abeni wrote:
> +static int net_shaper_fill_handle(struct sk_buff *msg,
> +				  const struct net_shaper_handle *handle,
> +				  u32 type)
> +{
> +	struct nlattr *handle_attr;
> +
> +	if (handle->scope == NET_SHAPER_SCOPE_UNSPEC)
> +		return 0;
> +
> +	handle_attr = nla_nest_start_noflag(msg, type);

_noflag() is deprecated

> +	if (!handle_attr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_u32(msg, NET_SHAPER_A_HANDLE_SCOPE, handle->scope) ||
> +	    (handle->scope >= NET_SHAPER_SCOPE_QUEUE &&
> +	     nla_put_u32(msg, NET_SHAPER_A_HANDLE_ID, handle->id)))
> +		goto handle_nest_cancel;
> +
> +	nla_nest_end(msg, handle_attr);
> +	return 0;
> +
> +handle_nest_cancel:
> +	nla_nest_cancel(msg, handle_attr);
> +	return -EMSGSIZE;
> +}

> +/* Initialize the context fetching the relevant device and
> + * acquiring a reference to it.
> + */
> +static int net_shaper_ctx_init(const struct genl_info *info, int type,
> +			       struct net_shaper_nl_ctx *ctx)
> +{
> +	struct net *ns = genl_info_net(info);
> +	struct net_device *dev;
> +	int ifindex;
> +
> +	memset(ctx, 0, sizeof(*ctx));
> +	if (GENL_REQ_ATTR_CHECK(info, type))
> +		return -EINVAL;
> +
> +	ifindex = nla_get_u32(info->attrs[type]);

Let's limit the 'binding' thing to just driver call sites, we can
redo the rest easily later. This line and next pretends to take
"arbitrary" type but clearly wants a ifindex/netdev, right?

> +	dev = netdev_get_by_index(ns, ifindex, &ctx->dev_tracker, GFP_KERNEL);
> +	if (!dev) {
> +		NL_SET_BAD_ATTR(info->extack, info->attrs[type]);
> +		return -ENOENT;
> +	}

> +static int net_shaper_parse_handle(const struct nlattr *attr,
> +				   const struct genl_info *info,
> +				   struct net_shaper_handle *handle)
> +{
> +	struct nlattr *tb[NET_SHAPER_A_HANDLE_MAX + 1];
> +	struct nlattr *scope_attr, *id_attr;
> +	u32 id = 0;
> +	int ret;
> +
> +	ret = nla_parse_nested(tb, NET_SHAPER_A_HANDLE_MAX, attr,
> +			       net_shaper_handle_nl_policy, info->extack);
> +	if (ret < 0)
> +		return ret;
> +
> +	scope_attr = tb[NET_SHAPER_A_HANDLE_SCOPE];
> +	if (!scope_attr) {

NL_REQ_ATTR_CHECK()

> +		NL_SET_BAD_ATTR(info->extack,
> +				tb[NET_SHAPER_A_HANDLE_SCOPE]);
> +		return -EINVAL;
> +	}
> +
> +	handle->scope = nla_get_u32(scope_attr);
> +
> +	/* The default id for NODE scope shapers is an invalid one
> +	 * to help the 'group' operation discriminate between new
> +	 * NODE shaper creation (ID_UNSPEC) and reuse of existing
> +	 * shaper (any other value).
> +	 */
> +	id_attr = tb[NET_SHAPER_A_HANDLE_ID];
> +	if (id_attr)
> +		id = nla_get_u32(id_attr);
> +	else if (handle->scope == NET_SHAPER_SCOPE_NODE)
> +		id = NET_SHAPER_ID_UNSPEC;
> +
> +	handle->id = id;
> +	return 0;
> +}
> +
> +static int net_shaper_generic_pre(struct genl_info *info, int type)
> +{
> +	struct net_shaper_nl_ctx *ctx;
> +	int ret;
> +
> +	ctx = kmalloc(sizeof(*ctx), GFP_KERNEL);

Maybe send a patch like this, to avoid having to allocate this space,
and special casing dump vs doit:

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 9ab49bfeae78..7658f0885178 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -124,7 +124,8 @@ struct genl_family {
  * @genlhdr: generic netlink message header
  * @attrs: netlink attributes
  * @_net: network namespace
- * @user_ptr: user pointers
+ * @ctx: storage space for the use by the family
+ * @user_ptr: user pointers (deprecated, use ctx instead)
  * @extack: extended ACK report struct
  */
 struct genl_info {
@@ -135,7 +136,10 @@ struct genl_info {
 	struct genlmsghdr *	genlhdr;
 	struct nlattr **	attrs;
 	possible_net_t		_net;
-	void *			user_ptr[2];
+	union {
+		u8		ctx[48];
+		void *		user_ptr[2];
+	};
 	struct netlink_ext_ack *extack;
 };
 
diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index feb54c63a116..29387b605f3e 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -997,7 +997,7 @@ static int genl_start(struct netlink_callback *cb)
 	info->info.attrs	= attrs;
 	genl_info_net_set(&info->info, sock_net(cb->skb->sk));
 	info->info.extack	= cb->extack;
-	memset(&info->info.user_ptr, 0, sizeof(info->info.user_ptr));
+	memset(&info->info.ctx, 0, sizeof(info->info.ctx));
 
 	cb->data = info;
 	if (ops->start) {
@@ -1104,7 +1104,7 @@ static int genl_family_rcv_msg_doit(const struct genl_family *family,
 	info.attrs = attrbuf;
 	info.extack = extack;
 	genl_info_net_set(&info, net);
-	memset(&info.user_ptr, 0, sizeof(info.user_ptr));
+	memset(&info.ctx, 0, sizeof(info.ctx));
 
 	if (ops->pre_doit) {
 		err = ops->pre_doit(ops, skb, &info);

> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ret = net_shaper_ctx_init(info, type, ctx);
> +	if (ret) {
> +		kfree(ctx);
> +		return ret;
> +	}
> +
> +	info->user_ptr[0] = ctx;
> +	return 0;
> +}
> +
>  int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info)
>  {
> -	return -EOPNOTSUPP;
> +	struct net_shaper_binding *binding;
> +	struct net_shaper_handle handle;
> +	struct net_shaper_info *shaper;
> +	struct sk_buff *msg;
> +	int ret;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, NET_SHAPER_A_HANDLE))
> +		return -EINVAL;
> +
> +	binding = net_shaper_binding_from_ctx(info->user_ptr[0]);

This 'binding' has the same meaning as 'binding' in TCP ZC? :(

> +	shaper = net_shaper_cache_lookup(binding, &handle);

Why call the stored info "cache"? It's the authoritative version of
user configuration, isn't it?

> +	if (!shaper) {
> +		NL_SET_BAD_ATTR(info->extack,
> +				info->attrs[NET_SHAPER_A_HANDLE]);
> +		return -ENOENT;
> +	}
> +
> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	ret = net_shaper_fill_one(msg, binding, &handle, shaper, info);
> +	if (ret)
> +		goto free_msg;
> +
> +	ret =  genlmsg_reply(msg, info);

double space