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
