[PATCH 0/4] can: gw: Netlink related fixes

[PATCH 0/4] can: gw: Netlink related fixes

[Thomas Graf]

Hi

These patches fix some Netlink quirks in the CAN gateway code.
Unfortunately I do not own any CAN hardware so this is compile
tested only. I'm fairly confident though as the changes are
mostly straight forward. I would appreciate if someone could
run them through a test suite or something though.


Thomas Graf (4):
  can: Don't bump nlmsg_len manually
  can: Use nla_policy to validate netlink attributes
  can: Properly fill the netlink header when responding to RTM_GETROUTE
  can: Remove pointless casts

 net/can/gw.c |   90 +++++++++++++++++++++------------------------------------
 1 files changed, 33 insertions(+), 57 deletions(-)

-- 
1.7.7.6

[PATCH 1/4] can: Don't bump nlmsg_len manually

[Thomas Graf]

nlmsg_end() will take care of this when we finalize the message.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/can/gw.c |   27 +--------------------------
 1 files changed, 1 insertions(+), 26 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index b41acf2..a3ff980 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -462,15 +462,11 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 	if (gwj->handled_frames) {
 		if (nla_put_u32(skb, CGW_HANDLED, gwj->handled_frames) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(u32));
 	}
 
 	if (gwj->dropped_frames) {
 		if (nla_put_u32(skb, CGW_DROPPED, gwj->dropped_frames) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(u32));
 	}
 
 	/* check non default settings of attributes */
@@ -480,8 +476,6 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 		mb.modtype = gwj->mod.modtype.and;
 		if (nla_put(skb, CGW_MOD_AND, sizeof(mb), &mb) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(mb));
 	}
 
 	if (gwj->mod.modtype.or) {
@@ -489,8 +483,6 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 		mb.modtype = gwj->mod.modtype.or;
 		if (nla_put(skb, CGW_MOD_OR, sizeof(mb), &mb) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(mb));
 	}
 
 	if (gwj->mod.modtype.xor) {
@@ -498,8 +490,6 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 		mb.modtype = gwj->mod.modtype.xor;
 		if (nla_put(skb, CGW_MOD_XOR, sizeof(mb), &mb) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(mb));
 	}
 
 	if (gwj->mod.modtype.set) {
@@ -507,26 +497,18 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 		mb.modtype = gwj->mod.modtype.set;
 		if (nla_put(skb, CGW_MOD_SET, sizeof(mb), &mb) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(mb));
 	}
 
 	if (gwj->mod.csumfunc.crc8) {
 		if (nla_put(skb, CGW_CS_CRC8, CGW_CS_CRC8_LEN,
 			    &gwj->mod.csum.crc8) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + \
-				NLA_ALIGN(CGW_CS_CRC8_LEN);
 	}
 
 	if (gwj->mod.csumfunc.xor) {
 		if (nla_put(skb, CGW_CS_XOR, CGW_CS_XOR_LEN,
 			    &gwj->mod.csum.xor) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + \
-				NLA_ALIGN(CGW_CS_XOR_LEN);
 	}
 
 	if (gwj->gwtype == CGW_TYPE_CAN_CAN) {
@@ -535,23 +517,16 @@ static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
 			if (nla_put(skb, CGW_FILTER, sizeof(struct can_filter),
 				    &gwj->ccgw.filter) < 0)
 				goto cancel;
-			else
-				nlh->nlmsg_len += NLA_HDRLEN +
-					NLA_ALIGN(sizeof(struct can_filter));
 		}
 
 		if (nla_put_u32(skb, CGW_SRC_IF, gwj->ccgw.src_idx) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(u32));
 
 		if (nla_put_u32(skb, CGW_DST_IF, gwj->ccgw.dst_idx) < 0)
 			goto cancel;
-		else
-			nlh->nlmsg_len += NLA_HDRLEN + NLA_ALIGN(sizeof(u32));
 	}
 
-	return skb->len;
+	return nlmsg_end(skb, nlh);
 
 cancel:
 	nlmsg_cancel(skb, nlh);
-- 
1.7.7.6

[PATCH 2/4] can: Use nla_policy to validate netlink attributes

[Thomas Graf]

Also use nla_get_u32() instead of nla_memcpy() to access u32 attribtues.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/can/gw.c |   47 +++++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index a3ff980..a1c639c 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -558,6 +558,18 @@ cont:
 	return skb->len;
 }
 
+static const struct nla_policy cgw_policy[CGW_MAX+1] = {
+	[CGW_MOD_AND]	= { .len = sizeof(struct cgw_frame_mod) },
+	[CGW_MOD_OR]	= { .len = sizeof(struct cgw_frame_mod) },
+	[CGW_MOD_XOR]	= { .len = sizeof(struct cgw_frame_mod) },
+	[CGW_MOD_SET]	= { .len = sizeof(struct cgw_frame_mod) },
+	[CGW_CS_XOR]	= { .len = sizeof(struct cgw_csum_xor) },
+	[CGW_CS_CRC8]	= { .len = sizeof(struct cgw_csum_crc8) },
+	[CGW_SRC_IF]	= { .type = NLA_U32 },
+	[CGW_DST_IF]	= { .type = NLA_U32 },
+	[CGW_FILTER]	= { .len = sizeof(struct can_filter) },
+};
+
 /* check for common and gwtype specific attributes */
 static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 			  u8 gwtype, void *gwtypeattr)
@@ -570,14 +582,14 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 	/* initialize modification & checksum data space */
 	memset(mod, 0, sizeof(*mod));
 
-	err = nlmsg_parse(nlh, sizeof(struct rtcanmsg), tb, CGW_MAX, NULL);
+	err = nlmsg_parse(nlh, sizeof(struct rtcanmsg), tb, CGW_MAX,
+			  cgw_policy);
 	if (err < 0)
 		return err;
 
 	/* check for AND/OR/XOR/SET modifications */
 
-	if (tb[CGW_MOD_AND] &&
-	    nla_len(tb[CGW_MOD_AND]) == CGW_MODATTR_LEN) {
+	if (tb[CGW_MOD_AND]) {
 		nla_memcpy(&mb, tb[CGW_MOD_AND], CGW_MODATTR_LEN);
 
 		canframecpy(&mod->modframe.and, &mb.cf);
@@ -593,8 +605,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 			mod->modfunc[modidx++] = mod_and_data;
 	}
 
-	if (tb[CGW_MOD_OR] &&
-	    nla_len(tb[CGW_MOD_OR]) == CGW_MODATTR_LEN) {
+	if (tb[CGW_MOD_OR]) {
 		nla_memcpy(&mb, tb[CGW_MOD_OR], CGW_MODATTR_LEN);
 
 		canframecpy(&mod->modframe.or, &mb.cf);
@@ -610,8 +621,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 			mod->modfunc[modidx++] = mod_or_data;
 	}
 
-	if (tb[CGW_MOD_XOR] &&
-	    nla_len(tb[CGW_MOD_XOR]) == CGW_MODATTR_LEN) {
+	if (tb[CGW_MOD_XOR]) {
 		nla_memcpy(&mb, tb[CGW_MOD_XOR], CGW_MODATTR_LEN);
 
 		canframecpy(&mod->modframe.xor, &mb.cf);
@@ -627,8 +637,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 			mod->modfunc[modidx++] = mod_xor_data;
 	}
 
-	if (tb[CGW_MOD_SET] &&
-	    nla_len(tb[CGW_MOD_SET]) == CGW_MODATTR_LEN) {
+	if (tb[CGW_MOD_SET]) {
 		nla_memcpy(&mb, tb[CGW_MOD_SET], CGW_MODATTR_LEN);
 
 		canframecpy(&mod->modframe.set, &mb.cf);
@@ -647,9 +656,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 	/* check for checksum operations after CAN frame modifications */
 	if (modidx) {
 
-		if (tb[CGW_CS_CRC8] &&
-		    nla_len(tb[CGW_CS_CRC8]) == CGW_CS_CRC8_LEN) {
-
+		if (tb[CGW_CS_CRC8]) {
 			struct cgw_csum_crc8 *c = (struct cgw_csum_crc8 *)\
 				nla_data(tb[CGW_CS_CRC8]);
 
@@ -674,9 +681,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 				mod->csumfunc.crc8 = cgw_csum_crc8_neg;
 		}
 
-		if (tb[CGW_CS_XOR] &&
-		    nla_len(tb[CGW_CS_XOR]) == CGW_CS_XOR_LEN) {
-
+		if (tb[CGW_CS_XOR]) {
 			struct cgw_csum_xor *c = (struct cgw_csum_xor *)\
 				nla_data(tb[CGW_CS_XOR]);
 
@@ -710,8 +715,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 		memset(ccgw, 0, sizeof(*ccgw));
 
 		/* check for can_filter in attributes */
-		if (tb[CGW_FILTER] &&
-		    nla_len(tb[CGW_FILTER]) == sizeof(struct can_filter))
+		if (tb[CGW_FILTER])
 			nla_memcpy(&ccgw->filter, tb[CGW_FILTER],
 				   sizeof(struct can_filter));
 
@@ -721,13 +725,8 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 		if (!tb[CGW_SRC_IF] || !tb[CGW_DST_IF])
 			return err;
 
-		if (nla_len(tb[CGW_SRC_IF]) == sizeof(u32))
-			nla_memcpy(&ccgw->src_idx, tb[CGW_SRC_IF],
-				   sizeof(u32));
-
-		if (nla_len(tb[CGW_DST_IF]) == sizeof(u32))
-			nla_memcpy(&ccgw->dst_idx, tb[CGW_DST_IF],
-				   sizeof(u32));
+		ccgw->src_idx = nla_get_u32(tb[CGW_SRC_IF]);
+		ccgw->dst_idx = nla_get_u32(tb[CGW_DST_IF]);
 
 		/* both indices set to 0 for flushing all routing entries */
 		if (!ccgw->src_idx && !ccgw->dst_idx)
-- 
1.7.7.6

[PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE

[Thomas Graf]

 - set message type to RTM_NEWROUTE
 - relate to original request by inheriting the sequence and port number.
 - set NLM_F_MULTI because it's a dump and more messages will follow

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/can/gw.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index a1c639c..20c36e1 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -444,11 +444,14 @@ static int cgw_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
+static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
+		       u32 pid, u32 seq, int flags)
 {
 	struct cgw_frame_mod mb;
 	struct rtcanmsg *rtcan;
-	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
+	struct nlmsghdr *nlh;
+
+	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);
 	if (!nlh)
 		return -EMSGSIZE;
 
@@ -546,7 +549,8 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
 		if (idx < s_idx)
 			goto cont;
 
-		if (cgw_put_job(skb, gwj) < 0)
+		if (cgw_put_job(skb, gwj, RTM_NEWROUTE, NETLINK_CB(cb->skb).pid,
+		    cb->nlh->nlmsg_seq, NLM_F_MULTI) < 0)
 			break;
 cont:
 		idx++;
-- 
1.7.7.6

Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE

[Oliver Hartkopp]

On 05.07.2012 14:19, Thomas Graf wrote:

>  - set message type to RTM_NEWROUTE
>  - relate to original request by inheriting the sequence and port number.
>  - set NLM_F_MULTI because it's a dump and more messages will follow
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  net/can/gw.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index a1c639c..20c36e1 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -444,11 +444,14 @@ static int cgw_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> -static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
> +static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
> +		       u32 pid, u32 seq, int flags)
>  {
>  	struct cgw_frame_mod mb;
>  	struct rtcanmsg *rtcan;
> -	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
> +	struct nlmsghdr *nlh;
> +
> +	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> @@ -546,7 +549,8 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
>  		if (idx < s_idx)
>  			goto cont;
>  
> -		if (cgw_put_job(skb, gwj) < 0)
> +		if (cgw_put_job(skb, gwj, RTM_NEWROUTE, NETLINK_CB(cb->skb).pid,
> +		    cb->nlh->nlmsg_seq, NLM_F_MULTI) < 0)
>  			break;
>  cont:
>  		idx++;


Hello Thomas,

what about this approach which reduces the number of parameters and stack
space when calling cgw_put_job() ??


diff --git a/net/can/gw.c b/net/can/gw.c
index b41acf2..b6b9e0a 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -444,11 +444,15 @@ static int cgw_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
-static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
+static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj,
+		       struct netlink_callback *cb)
 {
 	struct cgw_frame_mod mb;
 	struct rtcanmsg *rtcan;
-	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
+	struct nlmsghdr *nlh;
+
+	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq,
+			RTM_NEWROUTE, sizeof(*rtcan), NLM_F_MULTI);
 	if (!nlh)
 		return -EMSGSIZE;
 
@@ -571,7 +575,7 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
 		if (idx < s_idx)
 			goto cont;
 
-		if (cgw_put_job(skb, gwj) < 0)
+		if (cgw_put_job(skb, gwj, cb) < 0)
 			break;
 cont:
 		idx++;


Regards,
Oliver

Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE

[Thomas Graf]

On Fri, Jul 06, 2012 at 10:39:05PM +0200, Oliver Hartkopp wrote:
> what about this approach which reduces the number of parameters and stack
> space when calling cgw_put_job() ??
> 
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index b41acf2..b6b9e0a 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -444,11 +444,15 @@ static int cgw_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> -static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
> +static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj,
> +		       struct netlink_callback *cb)
>  {
>  	struct cgw_frame_mod mb;
>  	struct rtcanmsg *rtcan;
> -	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
> +	struct nlmsghdr *nlh;
> +
> +	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq,
> +			RTM_NEWROUTE, sizeof(*rtcan), NLM_F_MULTI);
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> @@ -571,7 +575,7 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
>  		if (idx < s_idx)
>  			goto cont;
>  
> -		if (cgw_put_job(skb, gwj) < 0)
> +		if (cgw_put_job(skb, gwj, cb) < 0)
>  			break;
>  cont:
>  		idx++;

The format I've chosen allows to implement a non-dump RTM_GETROUTE using
cgw_put_job() if needed and is in line with how such filling functions
are typically prototyped in other netlink protocol implementations.

~Thomas

Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE

[Oliver Hartkopp]

On 09.07.2012 11:43, Thomas Graf wrote:

> On Fri, Jul 06, 2012 at 10:39:05PM +0200, Oliver Hartkopp wrote:
>> what about this approach which reduces the number of parameters and stack
>> space when calling cgw_put_job() ??
>>
>>
>> diff --git a/net/can/gw.c b/net/can/gw.c
>> index b41acf2..b6b9e0a 100644
>> --- a/net/can/gw.c
>> +++ b/net/can/gw.c
>> @@ -444,11 +444,15 @@ static int cgw_notifier(struct notifier_block *nb,
>>  	return NOTIFY_DONE;
>>  }
>>  
>> -static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
>> +static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj,
>> +		       struct netlink_callback *cb)
>>  {
>>  	struct cgw_frame_mod mb;
>>  	struct rtcanmsg *rtcan;
>> -	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
>> +	struct nlmsghdr *nlh;
>> +
>> +	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq,
>> +			RTM_NEWROUTE, sizeof(*rtcan), NLM_F_MULTI);
>>  	if (!nlh)
>>  		return -EMSGSIZE;
>>  
>> @@ -571,7 +575,7 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
>>  		if (idx < s_idx)
>>  			goto cont;
>>  
>> -		if (cgw_put_job(skb, gwj) < 0)
>> +		if (cgw_put_job(skb, gwj, cb) < 0)
>>  			break;
>>  cont:
>>  		idx++;
> 
> The format I've chosen allows to implement a non-dump RTM_GETROUTE using
> cgw_put_job() if needed and is in line with how such filling functions
> are typically prototyped in other netlink protocol implementations.
> 


Yes, i also checked that the format was commonly used, but i did not find any
use-case for cgw_put_job() besides the full dump ...

Btw. both approaches are fine to me then.

Many thanks,
Oliver

Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE

[Oliver Hartkopp]

Hello Thomas,

besides the fact that i would like to simplify this patch (3/4) as shown
below, i was able to test it now.

You may add my

Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

to the next post.

Do you have an URL to pull this patch set for Marc?

Thanks for your effort!

Best regards,
Oliver



On 05.07.2012 14:19, Thomas Graf wrote:

>  - set message type to RTM_NEWROUTE
>  - relate to original request by inheriting the sequence and port number.
>  - set NLM_F_MULTI because it's a dump and more messages will follow
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
>  net/can/gw.c |   10 +++++++---
>  1 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/can/gw.c b/net/can/gw.c
> index a1c639c..20c36e1 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -444,11 +444,14 @@ static int cgw_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> -static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
> +static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj, int type,
> +		       u32 pid, u32 seq, int flags)
>  {
>  	struct cgw_frame_mod mb;
>  	struct rtcanmsg *rtcan;
> -	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
> +	struct nlmsghdr *nlh;
> +
> +	nlh = nlmsg_put(skb, pid, seq, type, sizeof(*rtcan), flags);
>  	if (!nlh)
>  		return -EMSGSIZE;
>  
> @@ -546,7 +549,8 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
>  		if (idx < s_idx)
>  			goto cont;
>  
> -		if (cgw_put_job(skb, gwj) < 0)
> +		if (cgw_put_job(skb, gwj, RTM_NEWROUTE, NETLINK_CB(cb->skb).pid,
> +		    cb->nlh->nlmsg_seq, NLM_F_MULTI) < 0)
>  			break;
>  cont:
>  		idx++;




Hello Thomas,

what about this approach which reduces the number of parameters and stack
space when calling cgw_put_job() ??


diff --git a/net/can/gw.c b/net/can/gw.c
index b41acf2..b6b9e0a 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -444,11 +444,15 @@ static int cgw_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
-static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj)
+static int cgw_put_job(struct sk_buff *skb, struct cgw_job *gwj,
+		       struct netlink_callback *cb)
 {
 	struct cgw_frame_mod mb;
 	struct rtcanmsg *rtcan;
-	struct nlmsghdr *nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*rtcan), 0);
+	struct nlmsghdr *nlh;
+
+	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq,
+			RTM_NEWROUTE, sizeof(*rtcan), NLM_F_MULTI);
 	if (!nlh)
 		return -EMSGSIZE;
@@ -571,7 +575,7 @@ static int cgw_dump_jobs(struct sk_buff *skb, struct netlink_callback *cb)
 		if (idx < s_idx)
 			goto cont;
-		if (cgw_put_job(skb, gwj) < 0)
+		if (cgw_put_job(skb, gwj, cb) < 0)
 			break;
 cont:
 		idx++;


Regards,
Oliver

Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On 07/07/2012 11:18 AM, Oliver Hartkopp wrote:
> Hello Thomas,
> 
> besides the fact that i would like to simplify this patch (3/4) as shown
> below, i was able to test it now.
> 
> You may add my
> 
> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> to the next post.
> 
> Do you have an URL to pull this patch set for Marc?

I can take this series as is and add your Tested-by and Acked-by to each
patch. Is this okay?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH 3/4] can: Properly fill the netlink header when responding to RTM_GETROUTE

[Oliver Hartkopp]

On 10.07.2012 17:29, Marc Kleine-Budde wrote:

> On 07/07/2012 11:18 AM, Oliver Hartkopp wrote:
>> Hello Thomas,
>>
>> besides the fact that i would like to simplify this patch (3/4) as shown
>> below, i was able to test it now.
>>
>> You may add my
>>
>> Tested-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>
>> to the next post.
>>
>> Do you have an URL to pull this patch set for Marc?
> 
> I can take this series as is and add your Tested-by and Acked-by to each
> patch. Is this okay?


Yes. That would be fine!

Tnx,
Oliver

[PATCH 4/4] can: Remove pointless casts

[Thomas Graf]

No need to cast return value of nla_data()

Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
 net/can/gw.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/can/gw.c b/net/can/gw.c
index 20c36e1..b54d5e6 100644
--- a/net/can/gw.c
+++ b/net/can/gw.c
@@ -661,8 +661,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 	if (modidx) {
 
 		if (tb[CGW_CS_CRC8]) {
-			struct cgw_csum_crc8 *c = (struct cgw_csum_crc8 *)\
-				nla_data(tb[CGW_CS_CRC8]);
+			struct cgw_csum_crc8 *c = nla_data(tb[CGW_CS_CRC8]);
 
 			err = cgw_chk_csum_parms(c->from_idx, c->to_idx,
 						 c->result_idx);
@@ -686,8 +685,7 @@ static int cgw_parse_attr(struct nlmsghdr *nlh, struct cf_mod *mod,
 		}
 
 		if (tb[CGW_CS_XOR]) {
-			struct cgw_csum_xor *c = (struct cgw_csum_xor *)\
-				nla_data(tb[CGW_CS_XOR]);
+			struct cgw_csum_xor *c = nla_data(tb[CGW_CS_XOR]);
 
 			err = cgw_chk_csum_parms(c->from_idx, c->to_idx,
 						 c->result_idx);
-- 
1.7.7.6

Re: [PATCH 0/4] can: gw: Netlink related fixes

[Oliver Hartkopp]

On 05.07.2012 14:19, Thomas Graf wrote:


> These patches fix some Netlink quirks in the CAN gateway code.
> Unfortunately I do not own any CAN hardware so this is compile
> tested only. I'm fairly confident though as the changes are
> mostly straight forward. I would appreciate if someone could
> run them through a test suite or something though.
> 
> 
> Thomas Graf (4):
>   can: Don't bump nlmsg_len manually
>   can: Use nla_policy to validate netlink attributes
>   can: Properly fill the netlink header when responding to RTM_GETROUTE
>   can: Remove pointless casts
> 
>  net/can/gw.c |   90 +++++++++++++++++++++------------------------------------
>  1 files changed, 33 insertions(+), 57 deletions(-)
> 


Hi Thomas,

thanks for the cleanups!

The can-gw was my first netlink based project and i did the best i knew.

From the first sight it looks good. I now also got behind the nla_policy
trick, which checks the minimal length too.

I'll test the patches this weekend with the virtual CAN interfaces (no CAN HW
needed), so that we probably catch the current development window.

Best regards,
Oliver

Re: [PATCH 0/4] can: gw: Netlink related fixes

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]

On 07/05/2012 02:19 PM, Thomas Graf wrote:
> Hi
> 
> These patches fix some Netlink quirks in the CAN gateway code.
> Unfortunately I do not own any CAN hardware so this is compile
> tested only. I'm fairly confident though as the changes are
> mostly straight forward. I would appreciate if someone could
> run them through a test suite or something though.
> 
> 
> Thomas Graf (4):
>   can: Don't bump nlmsg_len manually
>   can: Use nla_policy to validate netlink attributes
>   can: Properly fill the netlink header when responding to RTM_GETROUTE
>   can: Remove pointless casts

Thanks applied to linux-can-next.

Thanks, Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]