netdev - Re: Genetlink per cmd policies

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]

Date:   Wed, 30 Sep 2020 20:36:24 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Jiri Pirko <jiri@...nulli.us>, Michal Kubecek <mkubecek@...e.cz>,
        dsahern@...nel.org, pablo@...filter.org, netdev@...r.kernel.org
Subject: Re: Genetlink per cmd policies

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

> I started with a get_policy() callback, but I didn't like it much.
> Static data is much more pleasant for a client of the API IMHO.

Yeah, true.

> What do you think about "ops light"? Insufficiently flexible?

TBH, I'm not really sure how you'd do it?

Admittedly, it _would_ be nice to reduce struct genl_ops further, I
could imagine, assuming that doit is far more common than anything else,
perhaps something like

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b9eb92f3fe86..a5abab50673c 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -125,23 +125,34 @@ genl_dumpit_info(struct netlink_callback *cb)
 	return cb->data;
 }
 
+/**
+ * struct genl_ops_ext - full generic netlink operations
+ * @start: start callback for dumps
+ * @dumpit: callback for dumpers
+ * @done: completion callback for dumps
+ * @policy: policy if different from the family policy
+ * @maxattr: max attr for the policy
+ */
+struct genl_ops_ext {
+	int (*start)(struct netlink_callback *cb);
+	int (*dumpit)(struct sk_buff *skb,
+		      struct netlink_callback *cb);
+	int (*done)(struct netlink_callback *cb);
+	const struct nla_policy *policy;
+	unsigned int maxattr;
+};
+
 /**
  * struct genl_ops - generic netlink operations
  * @cmd: command identifier
  * @internal_flags: flags used by the family
  * @flags: flags
  * @doit: standard command callback
- * @start: start callback for dumps
- * @dumpit: callback for dumpers
- * @done: completion callback for dumps
  */
 struct genl_ops {
 	int		       (*doit)(struct sk_buff *skb,
 				       struct genl_info *info);
-	int		       (*start)(struct netlink_callback *cb);
-	int		       (*dumpit)(struct sk_buff *skb,
-					 struct netlink_callback *cb);
-	int		       (*done)(struct netlink_callback *cb);
+	struct genl_ops_ext	*extops;
 	u8			cmd;
 	u8			internal_flags;
 	u8			flags;

... 

or perhaps even

diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index b9eb92f3fe86..9be3fc051400 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -125,29 +125,45 @@ genl_dumpit_info(struct netlink_callback *cb)
 	return cb->data;
 }
 
+/**
+ * struct genl_ops_ext - full generic netlink operations
+ * @start: start callback for dumps
+ * @dumpit: callback for dumpers
+ * @done: completion callback for dumps
+ * @doit: standard command callback
+ * @policy: policy if different from the family policy
+ * @maxattr: max attr for the policy
+ */
+struct genl_ops_ext {
+	int (*start)(struct netlink_callback *cb);
+	int (*dumpit)(struct sk_buff *skb, struct netlink_callback *cb);
+	int (*done)(struct netlink_callback *cb);
+	int (*doit)(struct sk_buff *skb, struct genl_info *info);
+	const struct nla_policy *policy;
+	unsigned int maxattr;
+};
+
 /**
  * struct genl_ops - generic netlink operations
  * @cmd: command identifier
  * @internal_flags: flags used by the family
  * @flags: flags
  * @doit: standard command callback
- * @start: start callback for dumps
- * @dumpit: callback for dumpers
- * @done: completion callback for dumps
+ * @extops: extended ops if needed, must use GENL_EXTOPS()
  */
 struct genl_ops {
-	int		       (*doit)(struct sk_buff *skb,
-				       struct genl_info *info);
-	int		       (*start)(struct netlink_callback *cb);
-	int		       (*dumpit)(struct sk_buff *skb,
-					 struct netlink_callback *cb);
-	int		       (*done)(struct netlink_callback *cb);
+	union {
+		int (*doit)(struct sk_buff *skb, struct genl_info *info);
+		struct genl_ops_ext *extops;
+	};
 	u8			cmd;
 	u8			internal_flags;
 	u8			flags;
 	u8			validate;
 };
 
+#define GENL_EXT_OPS(ptr) ((struct genl_ops_ext *)((uintptr_t)(ptr) | 1))
+
 int genl_register_family(struct genl_family *family);
 int genl_unregister_family(const struct genl_family *family);
 void genl_notify(const struct genl_family *family, struct sk_buff *skb,




But both sort of feel awkward, you have to declare another structure,
and link it manually to the right place?

There isn't really a _good_ way to express such a thing easily in C?

johannes