On Wed, 2020-09-30 at 12:46 -0700, Jakub Kicinski wrote:

This builds (I think) - around 100 extra LoC:

Looks good to me, couple of comments below.

+/**
+ * struct genl_light_ops - generic netlink operations (small version)
+ * @cmd: command identifier
+ * @internal_flags: flags used by the family
+ * @flags: flags
+ * @validate: validation flags from enum genl_validate_flags
+ * @doit: standard command callback
+ * @dumpit: callback for dumpers
+ *
+ * This is a cut-down version of struct genl_ops for users who don't need
+ * most of the ancillary infra and want to save space.
+ */
+struct genl_light_ops {
+	int	(*doit)(struct sk_buff *skb, struct genl_info *info);
+	int	(*dumpit)(struct sk_buff *skb, struct netlink_callback *cb);

Even dumpit is pretty rare (e.g. 10 out of 107 in nl80211) - maybe
remove that even? It's a bit more juggling in nl80211 to actually use
it, but I'm certainly happy to do that myself.

+static void genl_op_from_full(const struct genl_family *family,
+			      unsigned int i, struct genl_ops *op)
+{
+	memcpy(op, &family->ops[i], sizeof(*op));

What's wrong with struct assignment? :)

	*op = family->ops[i];

+	if (!op->maxattr)
+		op->maxattr = family->maxattr;
+	if (!op->policy)
+		op->policy = family->policy;

That doesn't build as is, I think? Or did you have some other patch
below it?

 static int genl_validate_ops(const struct genl_family *family)
 {

[...]

+	n_ops = genl_get_cmd_cnt(family);
 	if (!n_ops)
 		return 0;

Come to think of it, that check is kinda pointless, the loop won't run
if it's 0 and then we return 0 immediately anyway... whatever :)

 	for (i = 0; i < n_ops; i++) {
-		if (ops[i].dumpit == NULL && ops[i].doit == NULL)
+		struct genl_ops op;
+
+		if (genl_get_cmd_by_index(i, family, &op))
 			return -EINVAL;

Maybe WARN_ON() or something? It really ought to not be possible for
that to fail, since you're only iterating to n_ops, so you'd have to
have some consistency issues if that happens.

-		for (j = i + 1; j < n_ops; j++)
-			if (ops[i].cmd == ops[j].cmd)
+		if (op.dumpit == NULL && op.doit == NULL)
+			return -EINVAL;
+		for (j = i + 1; j < n_ops; j++) {
+			struct genl_ops op2;
+
+			if (genl_get_cmd_by_index(j, family, &op2))
 				return -EINVAL;

same here

+		for (i = 0; i < genl_get_cmd_cnt(family); i++) {
 			struct nlattr *nest;
-			const struct genl_ops *ops = &family->ops[i];
-			u32 op_flags = ops->flags;
+			struct genl_ops op;
+			u32 op_flags;
+
+			if (genl_get_cmd_by_index(i, family, &op))
+				goto nla_put_failure;

but actually, same here, so maybe it should just not even be able to
return an error but WARN_ON instead and clear the op, so you have
everything NULL in that case?

I don't really see a case where you'd have the index coming from
userspace and would have to protect against it being bad, or something?

johannes