[PATCH net-next 3/3] netlink: add validation of NLA_F_NESTED flag

[PATCH net-next 3/3] netlink: add validation of NLA_F_NESTED flag

[Michal Kubecek]

Add new validation flag NL_VALIDATE_NESTED which adds three consistency
checks of NLA_F_NESTED_FLAG:

  - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
  - the flag is not set on attributes with other policies except NLA_UNSPEC
  - the flag is set on attribute passed to nla_parse_nested()

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 include/net/netlink.h | 10 +++++++++-
 lib/nlattr.c          | 15 +++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 679f649748d4..55f68e00fb6e 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -401,6 +401,8 @@ struct nl_info {
  *	are enforced going forward.
  * @NL_VALIDATE_STRICT_ATTRS: strict attribute policy parsing (e.g.
  *	U8, U16, U32 must have exact size, etc.)
+ * @NL_VALIDATE_NESTED: Check that NLA_F_NESTED is set for NLA_NESTED(_ARRAY)
+ *	and unset for other policies.
  */
 enum netlink_validation {
 	NL_VALIDATE_LIBERAL = 0,
@@ -408,6 +410,7 @@ enum netlink_validation {
 	NL_VALIDATE_MAXTYPE = BIT(1),
 	NL_VALIDATE_UNSPEC = BIT(2),
 	NL_VALIDATE_STRICT_ATTRS = BIT(3),
+	NL_VALIDATE_NESTED = BIT(4),
 };
 
 #define NL_VALIDATE_DEPRECATED_STRICT (NL_VALIDATE_TRAILING |\
@@ -415,7 +418,8 @@ enum netlink_validation {
 #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
 			    NL_VALIDATE_MAXTYPE |\
 			    NL_VALIDATE_UNSPEC |\
-			    NL_VALIDATE_STRICT_ATTRS)
+			    NL_VALIDATE_STRICT_ATTRS |\
+			    NL_VALIDATE_NESTED)
 
 int netlink_rcv_skb(struct sk_buff *skb,
 		    int (*cb)(struct sk_buff *, struct nlmsghdr *,
@@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
 				   const struct nla_policy *policy,
 				   struct netlink_ext_ack *extack)
 {
+	if (!(nla->nla_type & NLA_F_NESTED)) {
+		NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");
+		return -EINVAL;
+	}
 	return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
 			   NL_VALIDATE_STRICT, extack);
 }
diff --git a/lib/nlattr.c b/lib/nlattr.c
index adc919b32bf9..92da65cb6637 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -184,6 +184,21 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 		}
 	}
 
+	if (validate & NL_VALIDATE_NESTED) {
+		if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
+		    !(nla->nla_type & NLA_F_NESTED)) {
+			NL_SET_ERR_MSG_ATTR(extack, nla,
+					    "nested attribute expected");
+			return -EINVAL;
+		}
+		if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
+		    pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
+			NL_SET_ERR_MSG_ATTR(extack, nla,
+					    "nested attribute not expected");
+			return -EINVAL;
+		}
+	}
+
 	switch (pt->type) {
 	case NLA_EXACT_LEN:
 		if (attrlen != pt->len)
-- 
2.21.0

[PATCH net-next 0/3] netlink: strict attribute checking follow-up

[Michal Kubecek]

Three follow-up patches for recent strict netlink validation series.

Patch 1 fixes dump handling for genetlink families which validate and parse
messages themselves (e.g. because they need different policies for diferent
commands).

Patch 2 sets bad_attr in extack in one place where this was omitted.

Patch 3 adds new NL_VALIDATE_NESTED flags for strict validation to enable
checking that NLA_F_NESTED value in received messages matches expectations
and includes this flag in NL_VALIDATE_STRICT. This would change userspace
visible behavior but the previous switching to NL_VALIDATE_STRICT for new
code is still only in net-next at the moment.

Michal Kubecek (3):
  genetlink: do not validate dump requests if there is no policy
  netlink: set bad attribute also on maxtype check
  netlink: add validation of NLA_F_NESTED flag

 include/net/netlink.h   | 10 +++++++++-
 lib/nlattr.c            | 18 +++++++++++++++++-
 net/netlink/genetlink.c | 24 ++++++++++++++----------
 3 files changed, 40 insertions(+), 12 deletions(-)

-- 
2.21.0

[PATCH net-next 1/3] genetlink: do not validate dump requests if there is no policy

[Michal Kubecek]

Unlike do requests, dump genetlink requests now perform strict validation
by default even if the genetlink family does not set policy and maxtype
because it does validation and parsing on its own (e.g. because it wants to
allow different message format for different commands). While the null
policy will be ignored, maxtype (which would be zero) is still checked so
that any attribute will fail validation.

The solution is to only call __nla_validate() from genl_family_rcv_msg()
if family->maxtype is set.

Fixes: ef6243acb478 ("genetlink: optionally validate strictly/dumps")
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 net/netlink/genetlink.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 72668759cd2b..9814d6dbd2d6 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -537,21 +537,25 @@ static int genl_family_rcv_msg(const struct genl_family *family,
 			return -EOPNOTSUPP;
 
 		if (!(ops->validate & GENL_DONT_VALIDATE_DUMP)) {
-			unsigned int validate = NL_VALIDATE_STRICT;
 			int hdrlen = GENL_HDRLEN + family->hdrsize;
 
-			if (ops->validate & GENL_DONT_VALIDATE_DUMP_STRICT)
-				validate = NL_VALIDATE_LIBERAL;
-
 			if (nlh->nlmsg_len < nlmsg_msg_size(hdrlen))
 				return -EINVAL;
 
-			rc = __nla_validate(nlmsg_attrdata(nlh, hdrlen),
-					    nlmsg_attrlen(nlh, hdrlen),
-					    family->maxattr, family->policy,
-					    validate, extack);
-			if (rc)
-				return rc;
+			if (family->maxattr) {
+				unsigned int validate = NL_VALIDATE_STRICT;
+
+				if (ops->validate &
+				    GENL_DONT_VALIDATE_DUMP_STRICT)
+					validate = NL_VALIDATE_LIBERAL;
+				rc = __nla_validate(nlmsg_attrdata(nlh, hdrlen),
+						    nlmsg_attrlen(nlh, hdrlen),
+						    family->maxattr,
+						    family->policy,
+						    validate, extack);
+				if (rc)
+					return rc;
+			}
 		}
 
 		if (!family->parallel_ops) {
-- 
2.21.0

[PATCH net-next 2/3] netlink: set bad attribute also on maxtype check

[Michal Kubecek]

The check that attribute type is within 0...maxtype range in
__nla_validate_parse() sets only error message but not bad_attr in extack.
Set also bad_attr to tell userspace which attribute failed validation.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 lib/nlattr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/nlattr.c b/lib/nlattr.c
index 29f6336e2422..adc919b32bf9 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -356,7 +356,8 @@ static int __nla_validate_parse(const struct nlattr *head, int len, int maxtype,
 
 		if (type == 0 || type > maxtype) {
 			if (validate & NL_VALIDATE_MAXTYPE) {
-				NL_SET_ERR_MSG(extack, "Unknown attribute type");
+				NL_SET_ERR_MSG_ATTR(extack, nla,
+						    "Unknown attribute type");
 				return -EINVAL;
 			}
 			continue;
-- 
2.21.0

Re: [PATCH net-next 1/3] genetlink: do not validate dump requests if there is no policy

[Johannes Berg]

On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> Unlike do requests, dump genetlink requests now perform strict validation
> by default even if the genetlink family does not set policy and maxtype
> because it does validation and parsing on its own (e.g. because it wants to
> allow different message format for different commands). While the null
> policy will be ignored, maxtype (which would be zero) is still checked so
> that any attribute will fail validation.
> 
> The solution is to only call __nla_validate() from genl_family_rcv_msg()
> if family->maxtype is set.

D'oh. Which family was it that you found this on? I checked only ones
with policy I guess.

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes

Re: [PATCH net-next 2/3] netlink: set bad attribute also on maxtype check

[Johannes Berg]

On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> The check that attribute type is within 0...maxtype range in
> __nla_validate_parse() sets only error message but not bad_attr in extack.
> Set also bad_attr to tell userspace which attribute failed validation.

Good catch, we actually do have an attribute in this case.

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes

Re: [PATCH net-next 3/3] netlink: add validation of NLA_F_NESTED flag

[Johannes Berg]

On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> Add new validation flag NL_VALIDATE_NESTED which adds three consistency
> checks of NLA_F_NESTED_FLAG:
> 
>   - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
>   - the flag is not set on attributes with other policies except NLA_UNSPEC
>   - the flag is set on attribute passed to nla_parse_nested()

Looks good to me!

> @@ -415,7 +418,8 @@ enum netlink_validation {
>  #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
>  			    NL_VALIDATE_MAXTYPE |\
>  			    NL_VALIDATE_UNSPEC |\
> -			    NL_VALIDATE_STRICT_ATTRS)
> +			    NL_VALIDATE_STRICT_ATTRS |\
> +			    NL_VALIDATE_NESTED)

This is fine _right now_, but in general we cannot keep adding here
after the next release :-)

>  int netlink_rcv_skb(struct sk_buff *skb,
>  		    int (*cb)(struct sk_buff *, struct nlmsghdr *,
> @@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
>  				   const struct nla_policy *policy,
>  				   struct netlink_ext_ack *extack)
>  {
> +	if (!(nla->nla_type & NLA_F_NESTED)) {
> +		NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");

Maybe reword that to say "NLA_F_NESTED is missing" or so? The "nested
attribute expected" could result in a lot of headscratching (without
looking at the code) because it looks nested if you do nla_nest_start()
etc.

> +		return -EINVAL;
> +	}
>  	return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
>  			   NL_VALIDATE_STRICT, extack);

I'd probably put a blank line there but ymmv.

>  }
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index adc919b32bf9..92da65cb6637 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -184,6 +184,21 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
>  		}
>  	}
>  
> +	if (validate & NL_VALIDATE_NESTED) {
> +		if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
> +		    !(nla->nla_type & NLA_F_NESTED)) {
> +			NL_SET_ERR_MSG_ATTR(extack, nla,
> +					    "nested attribute expected");
> +			return -EINVAL;
> +		}
> +		if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
> +		    pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
> +			NL_SET_ERR_MSG_ATTR(extack, nla,
> +					    "nested attribute not expected");
> +			return -EINVAL;

Same comment here wrt. the messages, I think they should more explicitly
refer to the flag.

johannes

(PS: if you CC me on this address I generally can respond quicker)

Re: [PATCH net-next 1/3] genetlink: do not validate dump requests if there is no policy

[Michal Kubecek]

On Thu, May 02, 2019 at 02:51:33PM +0200, Johannes Berg wrote:
> On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > Unlike do requests, dump genetlink requests now perform strict validation
> > by default even if the genetlink family does not set policy and maxtype
> > because it does validation and parsing on its own (e.g. because it wants to
> > allow different message format for different commands). While the null
> > policy will be ignored, maxtype (which would be zero) is still checked so
> > that any attribute will fail validation.
> > 
> > The solution is to only call __nla_validate() from genl_family_rcv_msg()
> > if family->maxtype is set.
> 
> D'oh. Which family was it that you found this on? I checked only ones
> with policy I guess.

It was with my ethtool netlink series (still work in progress).

Michal

> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>
> 
> johannes
> 

Re: [PATCH net-next 1/3] genetlink: do not validate dump requests if there is no policy

[Johannes Berg]

On Thu, 2019-05-02 at 15:10 +0200, Michal Kubecek wrote:
> On Thu, May 02, 2019 at 02:51:33PM +0200, Johannes Berg wrote:
> > On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > > Unlike do requests, dump genetlink requests now perform strict validation
> > > by default even if the genetlink family does not set policy and maxtype
> > > because it does validation and parsing on its own (e.g. because it wants to
> > > allow different message format for different commands). While the null
> > > policy will be ignored, maxtype (which would be zero) is still checked so
> > > that any attribute will fail validation.
> > > 
> > > The solution is to only call __nla_validate() from genl_family_rcv_msg()
> > > if family->maxtype is set.
> > 
> > D'oh. Which family was it that you found this on? I checked only ones
> > with policy I guess.
> 
> It was with my ethtool netlink series (still work in progress).

Then you should probably *have* a policy to get all the other goodies
like automatic policy export (once I repost those patches)

johannes

Re: [PATCH net-next 3/3] netlink: add validation of NLA_F_NESTED flag

[Michal Kubecek]

On Thu, May 02, 2019 at 02:54:56PM +0200, Johannes Berg wrote:
> On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > Add new validation flag NL_VALIDATE_NESTED which adds three consistency
> > checks of NLA_F_NESTED_FLAG:
> > 
> >   - the flag is set on attributes with NLA_NESTED{,_ARRAY} policy
> >   - the flag is not set on attributes with other policies except NLA_UNSPEC
> >   - the flag is set on attribute passed to nla_parse_nested()
> 
> Looks good to me!
> 
> > @@ -415,7 +418,8 @@ enum netlink_validation {
> >  #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
> >  			    NL_VALIDATE_MAXTYPE |\
> >  			    NL_VALIDATE_UNSPEC |\
> > -			    NL_VALIDATE_STRICT_ATTRS)
> > +			    NL_VALIDATE_STRICT_ATTRS |\
> > +			    NL_VALIDATE_NESTED)
> 
> This is fine _right now_, but in general we cannot keep adding here
> after the next release :-)

Right, that's why I would like to get this into the same cycle as your
series.

> >  int netlink_rcv_skb(struct sk_buff *skb,
> >  		    int (*cb)(struct sk_buff *, struct nlmsghdr *,
> > @@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
> >  				   const struct nla_policy *policy,
> >  				   struct netlink_ext_ack *extack)
> >  {
> > +	if (!(nla->nla_type & NLA_F_NESTED)) {
> > +		NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");
> 
> Maybe reword that to say "NLA_F_NESTED is missing" or so? The "nested
> attribute expected" could result in a lot of headscratching (without
> looking at the code) because it looks nested if you do nla_nest_start()
> etc.

How about "NLA_F_NESTED is missing" and "NLA_F_NESTED not expected"?

> 
> > +		return -EINVAL;
> > +	}
> >  	return __nla_parse(tb, maxtype, nla_data(nla), nla_len(nla), policy,
> >  			   NL_VALIDATE_STRICT, extack);
> 
> I'd probably put a blank line there but ymmv.

OK

> >  }
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index adc919b32bf9..92da65cb6637 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -184,6 +184,21 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> >  		}
> >  	}
> >  
> > +	if (validate & NL_VALIDATE_NESTED) {
> > +		if ((pt->type == NLA_NESTED || pt->type == NLA_NESTED_ARRAY) &&
> > +		    !(nla->nla_type & NLA_F_NESTED)) {
> > +			NL_SET_ERR_MSG_ATTR(extack, nla,
> > +					    "nested attribute expected");
> > +			return -EINVAL;
> > +		}
> > +		if (pt->type != NLA_NESTED && pt->type != NLA_NESTED_ARRAY &&
> > +		    pt->type != NLA_UNSPEC && (nla->nla_type & NLA_F_NESTED)) {
> > +			NL_SET_ERR_MSG_ATTR(extack, nla,
> > +					    "nested attribute not expected");
> > +			return -EINVAL;
> 
> Same comment here wrt. the messages, I think they should more explicitly
> refer to the flag.
> 
> johannes
> 
> (PS: if you CC me on this address I generally can respond quicker)

I'll try to keep that in mind.

Michal

Re: [PATCH net-next 1/3] genetlink: do not validate dump requests if there is no policy

[Michal Kubecek]

On Thu, May 02, 2019 at 03:13:00PM +0200, Johannes Berg wrote:
> On Thu, 2019-05-02 at 15:10 +0200, Michal Kubecek wrote:
> > On Thu, May 02, 2019 at 02:51:33PM +0200, Johannes Berg wrote:
> > > On Thu, 2019-05-02 at 12:48 +0000, Michal Kubecek wrote:
> > > > Unlike do requests, dump genetlink requests now perform strict validation
> > > > by default even if the genetlink family does not set policy and maxtype
> > > > because it does validation and parsing on its own (e.g. because it wants to
> > > > allow different message format for different commands). While the null
> > > > policy will be ignored, maxtype (which would be zero) is still checked so
> > > > that any attribute will fail validation.
> > > > 
> > > > The solution is to only call __nla_validate() from genl_family_rcv_msg()
> > > > if family->maxtype is set.
> > > 
> > > D'oh. Which family was it that you found this on? I checked only ones
> > > with policy I guess.
> > 
> > It was with my ethtool netlink series (still work in progress).
> 
> Then you should probably *have* a policy to get all the other goodies
> like automatic policy export (once I repost those patches)

Wouldn't it mean effecitvely ending up with only one command (in
genetlink sense) and having to distinguish actual commands with
atributes? Even if I wanted to have just "get" and "set" command, common
policy wouldn't allow me to say which attributes are allowed for each of
them.

Michal

Re: [PATCH net-next 1/3] genetlink: do not validate dump requests if there is no policy

[David Ahern]

On 5/2/19 7:32 AM, Michal Kubecek wrote:
> Wouldn't it mean effecitvely ending up with only one command (in
> genetlink sense) and having to distinguish actual commands with
> atributes? Even if I wanted to have just "get" and "set" command, common
> policy wouldn't allow me to say which attributes are allowed for each of
> them.

yes, I have been stuck on that as well.

There are a number of RTA attributes that are only valid for GET
requests or only used in the response or only valid in NEW requests.
Right now there is no discriminator when validating policies and the
patch set to expose the policies to userspace

Re: [PATCH net-next 2/3] netlink: set bad attribute also on maxtype check

[David Ahern]

On 5/2/19 6:48 AM, Michal Kubecek wrote:
> The check that attribute type is within 0...maxtype range in
> __nla_validate_parse() sets only error message but not bad_attr in extack.
> Set also bad_attr to tell userspace which attribute failed validation.
> 
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
> ---
>  lib/nlattr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [PATCH net-next 3/3] netlink: add validation of NLA_F_NESTED flag

[David Ahern]

On 5/2/19 7:14 AM, Michal Kubecek wrote:
>>> @@ -1132,6 +1136,10 @@ static inline int nla_parse_nested(struct nlattr *tb[], int maxtype,
>>>  				   const struct nla_policy *policy,
>>>  				   struct netlink_ext_ack *extack)
>>>  {
>>> +	if (!(nla->nla_type & NLA_F_NESTED)) {
>>> +		NL_SET_ERR_MSG_ATTR(extack, nla, "nested attribute expected");
>>
>> Maybe reword that to say "NLA_F_NESTED is missing" or so? The "nested
>> attribute expected" could result in a lot of headscratching (without
>> looking at the code) because it looks nested if you do nla_nest_start()
>> etc.
> 
> How about "NLA_F_NESTED is missing" and "NLA_F_NESTED not expected"?
> 

That is much better to me.

Re: [PATCH net-next 3/3] netlink: add validation of NLA_F_NESTED flag

[Johannes Berg]

On Thu, 2019-05-02 at 15:14 +0200, Michal Kubecek wrote:
> 
> > > @@ -415,7 +418,8 @@ enum netlink_validation {
> > >  #define NL_VALIDATE_STRICT (NL_VALIDATE_TRAILING |\
> > >  			    NL_VALIDATE_MAXTYPE |\
> > >  			    NL_VALIDATE_UNSPEC |\
> > > -			    NL_VALIDATE_STRICT_ATTRS)
> > > +			    NL_VALIDATE_STRICT_ATTRS |\
> > > +			    NL_VALIDATE_NESTED)
> > 
> > This is fine _right now_, but in general we cannot keep adding here
> > after the next release :-)
> 
> Right, that's why I would like to get this into the same cycle as your
> series.

Yeah, I know you know, just wanted state it again :-)

> How about "NLA_F_NESTED is missing" and "NLA_F_NESTED not expected"?

Looks good to me.

johannes

Re: [PATCH net-next 1/3] genetlink: do not validate dump requests if there is no policy

[Johannes Berg]

On Thu, 2019-05-02 at 07:36 -0600, David Ahern wrote:
> On 5/2/19 7:32 AM, Michal Kubecek wrote:
> > Wouldn't it mean effecitvely ending up with only one command (in
> > genetlink sense) and having to distinguish actual commands with
> > atributes? Even if I wanted to have just "get" and "set" command, common
> > policy wouldn't allow me to say which attributes are allowed for each of
> > them.
> 
> yes, I have been stuck on that as well.
> 
> There are a number of RTA attributes that are only valid for GET
> requests or only used in the response or only valid in NEW requests.
> Right now there is no discriminator when validating policies and the
> patch set to expose the policies to userspace

Yeah. As I've been discussing with Pablo in various threads recently,
this is definitely something we're missing.

As I said there though, I think it's something we should treat as
orthogonal to the policies.

I haven't looked at your ethtool patches really now (if you have a git
tree that'd be nice), but I saw e.g.

+When appropriate, network device is identified by a nested attribute named
+ETHA_*_DEV. This attribute can contain
+
+    ETHA_DEV_INDEX	(u32)		device ifindex
+    ETHA_DEV_NAME	(string)	device name

Presumably, this is valid for each and every command, right?

I'm not sure I understand the "ETHA_*_DEV" part, but splitting the
policy per command means that things like this that are available/valid
for each command need to be stated over and over again. This opens up
the very easy possibility that you have one command that takes an
ETHA_DEV_INDEX as u32, and another that - for some reason - takes a u64
for example, or similar confusion between the same attribute stated in
different policies.

This is why I believe that when we have a flat namespace for attributes,
like ETHA_*, we should also have a flat policy for those attributes, and
that's why I made the genetlink to have a single policy.

At the same time, I do realize that this is not ideal. So far I've sort
of pushed this to be something that we should treat orthogonally to the
validation for the above reasons, i.e. *not* state this specifically in
the policy.

If we were able to express this in C, I'd probably say we should have
something like

static const struct genl_ops ops[] = {
        {
                .cmd = MY_CMD,
                .doit = my_cmd_doit,
		.valid_attrs = { MY_ATTR_A, MY_ATTR_B },
        },

	...
};

However, there's no way to express this in C code, except for

static const u16 my_cmd_valid_attrs[] = { MY_ATTR_A, MY_ATTR_B, 0 };

static const struct genl_ops ops[] = {
        {
                .cmd = MY_CMD,
                .doit = my_cmd_doit,
		.valid_attrs = my_cmd_valid_attrs,
        },

	...
};

which is clearly ugly to write. We could generate this code from a
domain-specific language like Pablo suggested, but I'm not really sure
that's ideal either.


Regardless, I think we should solve this problem orthogonally from the
policy, given that otherwise we can end up with the same attribute
meaning different things in different commands.

johannes