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:42:10 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Johannes Berg <johannes@...solutions.net>
Cc:     Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>,
        dsahern@...nel.org, pablo@...filter.org, netdev@...r.kernel.org
Subject: Re: Genetlink per cmd policies

On Wed, Sep 30, 2020 at 08:36:24PM +0200, Johannes Berg wrote:
> 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?

Not really good either but how about embedding struct genl_ops as first
member of struct genl_ops_ext like we do with sockets?

Michal