[PATCH] genetlink custom attribute type

[PATCH] genetlink custom attribute type

[Johannes Berg]

This patch adds an NLA_CUSTOM_CHECK type for netlink attributes
in order to be able to centrally define new attribute structures
instead of having to check these special types in each function
that uses such an attribute.

nl80211 will benefit from this because it needs an information
element attribute with a fixed type|length|value structure defined
in IEEE 802.11 which ought to be checked when passing an IE from
userspace.

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

--- wireless-dev.orig/include/net/netlink.h	2006-09-13 22:06:09.979647141 +0200
+++ wireless-dev/include/net/netlink.h	2006-09-13 22:06:11.329647141 +0200
@@ -159,6 +159,7 @@ enum {
 	NLA_MSECS,
 	NLA_NESTED,
 	NLA_NUL_STRING,
+	NLA_CUSTOM_CHECK,
 	__NLA_TYPE_MAX,
 };
 
@@ -176,6 +177,8 @@ enum {
  *    NLA_STRING           Maximum length of string
  *    NLA_NUL_STRING       Maximum length of string (excluding NUL)
  *    NLA_FLAG             Unused
+ *    NLA_CUSTOM_CHECK     Unused, check function must be assigned,
+ *                         it must return 0 if the attribute is valid
  *    All other            Exact length of attribute payload
  *
  * Example:
@@ -183,11 +186,13 @@ enum {
  * 	[ATTR_FOO] = { .type = NLA_U16 },
  *	[ATTR_BAR] = { .type = NLA_STRING, len = BARSIZ },
  *	[ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ *	[ATTR_CST] = { .type = NLA_CUSTOM_CHECK, .check = my_check },
  * };
  */
 struct nla_policy {
 	u16		type;
 	u16		len;
+	int		(*check)(struct nlattr *nla);
 };
 
 extern void		netlink_run_queue(struct sock *sk, unsigned int *qlen,
--- wireless-dev.orig/net/netlink/attr.c	2006-09-13 22:06:05.889647141 +0200
+++ wireless-dev/net/netlink/attr.c	2006-09-13 22:06:11.329647141 +0200
@@ -67,6 +67,9 @@ static int validate_nla(struct nlattr *n
 		}
 		break;
 
+	case NLA_CUSTOM_CHECK:
+		BUG_ON(!pt->check);
+		return pt->check(nla);
 	default:
 		if (pt->len)
 			minlen = pt->len;

Re: [PATCH] genetlink custom attribute type

[Thomas Graf]

* Johannes Berg <johannes@sipsolutions.net> 2006-09-26 09:25
> This patch adds an NLA_CUSTOM_CHECK type for netlink attributes
> in order to be able to centrally define new attribute structures
> instead of having to check these special types in each function
> that uses such an attribute.

Thinking it over I'm still not completely happy with this. A
small subset of all the validation tasks is simply too complex
to be put into the policy. The validation of your type value
array is such a case, address fields with variable length based
on their family is another. I think it's just not worth to
blow up struct nla_policy by 12 bytes per entry just to save
some code.

Re: [PATCH] genetlink custom attribute type

[Johannes Berg]

On Tue, 2006-09-26 at 11:44 +0200, Thomas Graf wrote:

> Thinking it over I'm still not completely happy with this. A
> small subset of all the validation tasks is simply too complex
> to be put into the policy. The validation of your type value
> array is such a case, address fields with variable length based
> on their family is another. I think it's just not worth to
> blow up struct nla_policy by 12 bytes per entry just to save
> some code.

Alright, I can instead just add the validation function call wherever
that attribute is going to be used. Just thought it might be nice to
have this generically, I'm not really too attached to it :)

I'll change it in the next nl80211 iteration.

johannes

Re: [PATCH] genetlink custom attribute type

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 26 Sep 2006 12:04:34 +0200

> On Tue, 2006-09-26 at 11:44 +0200, Thomas Graf wrote:
> 
> > Thinking it over I'm still not completely happy with this. A
> > small subset of all the validation tasks is simply too complex
> > to be put into the policy. The validation of your type value
> > array is such a case, address fields with variable length based
> > on their family is another. I think it's just not worth to
> > blow up struct nla_policy by 12 bytes per entry just to save
> > some code.
> 
> Alright, I can instead just add the validation function call wherever
> that attribute is going to be used. Just thought it might be nice to
> have this generically, I'm not really too attached to it :)
> 
> I'll change it in the next nl80211 iteration.

Thanks Johannes.

Re: [PATCH] genetlink custom attribute type

[Johannes Berg]

On Tue, 2006-09-26 at 11:44 +0200, Thomas Graf wrote:

> Thinking it over I'm still not completely happy with this. A
> small subset of all the validation tasks is simply too complex
> to be put into the policy. The validation of your type value
> array is such a case, address fields with variable length based
> on their family is another. I think it's just not worth to
> blow up struct nla_policy by 12 bytes per entry just to save
> some code.

Oh, I just had another idea... Feel free to ignore me if you think that
having this done in some generic way isn't worth it though. As I said,
it doesn't really make a difference to me in the end :)


Currently, we always pass a "struct nla_policy *policy" into things,
which really is an array. We could instead pass in a new

struct nla_validation {
	int (*custom_validate)(struct nlattr *nla);
	struct nla_policy *policy;
};

and call custom_validate() whenever we encounter something in the policy
that has type NLA_CUSTOM_VALIDATE. Downsides of this approach are that
it requires changing all current users, and introduces 16 bytes constant
overhead on 64-bit platforms, the size of nla_validation.

johannes

Re: [PATCH] genetlink custom attribute type

[Thomas Graf]

* Johannes Berg <johannes@sipsolutions.net> 2006-09-27 14:18
> On Tue, 2006-09-26 at 11:44 +0200, Thomas Graf wrote:
> 
> > Thinking it over I'm still not completely happy with this. A
> > small subset of all the validation tasks is simply too complex
> > to be put into the policy. The validation of your type value
> > array is such a case, address fields with variable length based
> > on their family is another. I think it's just not worth to
> > blow up struct nla_policy by 12 bytes per entry just to save
> > some code.
> 
> Oh, I just had another idea... Feel free to ignore me if you think that
> having this done in some generic way isn't worth it though. As I said,
> it doesn't really make a difference to me in the end :)
> 
> 
> Currently, we always pass a "struct nla_policy *policy" into things,
> which really is an array. We could instead pass in a new
> 
> struct nla_validation {
> 	int (*custom_validate)(struct nlattr *nla);
> 	struct nla_policy *policy;
> };
> 
> and call custom_validate() whenever we encounter something in the policy
> that has type NLA_CUSTOM_VALIDATE. Downsides of this approach are that
> it requires changing all current users, and introduces 16 bytes constant
> overhead on 64-bit platforms, the size of nla_validation.

Sorry for the delay.

That's not a bad idea, although it seems cleaner to just allow defining
a callback function which gets called foreach unknown attribute. As for
generic netlink, this callback could be defined on a per command basis
in struct genl_ops.

Re: [PATCH] genetlink custom attribute type

[Johannes Berg]

On Mon, 2006-10-02 at 15:49 +0200, Thomas Graf wrote:

> That's not a bad idea, although it seems cleaner to just allow defining
> a callback function which gets called foreach unknown attribute.

Hm, that'd work too, but it'd force me to leave these 'unknown
attributes' at the end of the attribute space, no? Otherwise, something
with .len=0,.type=0 is sort of valid as well, not?

> As for
> generic netlink, this callback could be defined on a per command basis
> in struct genl_ops.

I'd have to look at it more closely again, I removed the custom stuff
and it's not immediately important to me at this time.

johannes