[RFC] genetlink custom attribute type

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;

* Johannes Berg [off-list ref] 2006-09-14 09:44

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.

Looks good, we have to watch the size of struct nla_policy though.
This bumps the size from 4 bytes to 16 bytes on 64bit architectures
which might become a problem since we always use ATTR_MAX sized
arrays.

On Thu, 2006-09-14 at 10:14 +0200, Thomas Graf wrote:

Looks good, we have to watch the size of struct nla_policy though.
This bumps the size from 4 bytes to 16 bytes on 64bit architectures
which might become a problem since we always use ATTR_MAX sized
arrays.

Yes, I'm aware of that, but I couldn't think of a way to handle it well.

I thought about using a second array containing just the check
functions, and then (ab)using `len' to index into it but that didn't
seem clean enough.

Can you think of something better?

johannes

* Johannes Berg [off-list ref] 2006-09-14 11:21

On Thu, 2006-09-14 at 10:14 +0200, Thomas Graf wrote:

quoted

Looks good, we have to watch the size of struct nla_policy though.
This bumps the size from 4 bytes to 16 bytes on 64bit architectures
which might become a problem since we always use ATTR_MAX sized
arrays.

Yes, I'm aware of that, but I couldn't think of a way to handle it well.

I thought about using a second array containing just the check
functions, and then (ab)using `len' to index into it but that didn't
seem clean enough.

I agree, I talked about this with various people and some ideas
came up.

Always use function pointers to define the validation policy, i.e.
there would be nla_validate_u32() etc. The problem with this approach
is that for every string attribute with different length a separate
validation function is required which simply adds to code what it
saved from text. It also makes exporting the policy to userspace
impossible.

Following up on your idea, we could save a bit by using ERR_PTR() to
store both the callback pointer and type/len tuple in the very same
pointer but that gets very ugly.

I think its best to use your patch for now and see where this leads to.

On Wed, 2006-09-20 at 08:24 +0200, Thomas Graf wrote:

I think its best to use your patch for now and see where this leads to.

Alright, should I repost with a proper [PATCH] subject or is it good to
take as-is? :)

johannes

From: Johannes Berg <johannes@sipsolutions.net>
Date: Mon, 25 Sep 2006 16:58:28 +0200

On Wed, 2006-09-20 at 08:24 +0200, Thomas Graf wrote:

quoted

I think its best to use your patch for now and see where this leads to.

Alright, should I repost with a proper [PATCH] subject or is it good to
take as-is? :)

Please resend it as I've deleted the copy that was in my inbox.