gre: minor cleanups in netlink interface

gre: minor cleanups in netlink interface

[Patrick McHardy]

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

Some minor cleanups, there doesn't seem to be a reason for not
using the typeful helpers everywhere.

I noticed the interface expects to always get a full configuration
on change requests. Is there are particular reason for not
supporting incremental changes, lets say

"ip link change dev gre0 type gre remote <new remote>"

? It looks easy enough to change, so I could take care of this.

[-- Attachment #2: 02.diff --]
[-- Type: text/x-patch, Size: 2454 bytes --]

commit 5a00d521ffc35f3168bf72b9f0e398ae60793c50
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Oct 10 17:58:17 2008 +0200

    gre: minor cleanups in netlink interface
    
    - use typeful helpers for IFLA_GRE_LOCAL/IFLA_GRE_REMOTE
    - replace magic value by FIELD_SIZEOF
    - use MODULE_ALIAS_RTNL_LINK macro
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index c0755e9..05ebce2 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1368,10 +1368,10 @@ static void ipgre_netlink_parms(struct nlattr *data[],
 		parms->o_key = nla_get_be32(data[IFLA_GRE_OKEY]);
 
 	if (data[IFLA_GRE_LOCAL])
-		memcpy(&parms->iph.saddr, nla_data(data[IFLA_GRE_LOCAL]), 4);
+		parms->iph.saddr = nla_get_be32(data[IFLA_GRE_LOCAL]);
 
 	if (data[IFLA_GRE_REMOTE])
-		memcpy(&parms->iph.daddr, nla_data(data[IFLA_GRE_REMOTE]), 4);
+		parms->iph.daddr = nla_get_be32(data[IFLA_GRE_REMOTE]);
 
 	if (data[IFLA_GRE_TTL])
 		parms->iph.ttl = nla_get_u8(data[IFLA_GRE_TTL]);
@@ -1541,8 +1541,8 @@ static int ipgre_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	NLA_PUT_BE16(skb, IFLA_GRE_OFLAGS, p->o_flags);
 	NLA_PUT_BE32(skb, IFLA_GRE_IKEY, p->i_key);
 	NLA_PUT_BE32(skb, IFLA_GRE_OKEY, p->o_key);
-	NLA_PUT(skb, IFLA_GRE_LOCAL, 4, &p->iph.saddr);
-	NLA_PUT(skb, IFLA_GRE_REMOTE, 4, &p->iph.daddr);
+	NLA_PUT_BE32(skb, IFLA_GRE_LOCAL, p->iph.saddr);
+	NLA_PUT_BE32(skb, IFLA_GRE_REMOTE, p->iph.daddr);
 	NLA_PUT_U8(skb, IFLA_GRE_TTL, p->iph.ttl);
 	NLA_PUT_U8(skb, IFLA_GRE_TOS, p->iph.tos);
 	NLA_PUT_U8(skb, IFLA_GRE_PMTUDISC, !!(p->iph.frag_off & htons(IP_DF)));
@@ -1559,8 +1559,8 @@ static const struct nla_policy ipgre_policy[IFLA_GRE_MAX + 1] = {
 	[IFLA_GRE_OFLAGS]	= { .type = NLA_U16 },
 	[IFLA_GRE_IKEY]		= { .type = NLA_U32 },
 	[IFLA_GRE_OKEY]		= { .type = NLA_U32 },
-	[IFLA_GRE_LOCAL]	= { .len = 4 },
-	[IFLA_GRE_REMOTE]	= { .len = 4 },
+	[IFLA_GRE_LOCAL]	= { .len = FIELD_SIZEOF(struct iphdr, saddr) },
+	[IFLA_GRE_REMOTE]	= { .len = FIELD_SIZEOF(struct iphdr, daddr) },
 	[IFLA_GRE_TTL]		= { .type = NLA_U8 },
 	[IFLA_GRE_TOS]		= { .type = NLA_U8 },
 	[IFLA_GRE_PMTUDISC]	= { .type = NLA_U8 },
@@ -1643,5 +1643,5 @@ static void __exit ipgre_fini(void)
 module_init(ipgre_init);
 module_exit(ipgre_fini);
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("rtnl-link-gre");
-MODULE_ALIAS("rtnl-link-gretap");
+MODULE_ALIAS_RTNL_LINK("gre");
+MODULE_ALIAS_RTNL_LINK("gretap");

Re: gre: minor cleanups in netlink interface

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 10 Oct 2008 18:04:52 +0200

>     gre: minor cleanups in netlink interface
>     
>     - use typeful helpers for IFLA_GRE_LOCAL/IFLA_GRE_REMOTE
>     - replace magic value by FIELD_SIZEOF
>     - use MODULE_ALIAS_RTNL_LINK macro
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied, thanks Patrick.

Re: gre: minor cleanups in netlink interface

[Herbert Xu]

On Fri, Oct 10, 2008 at 06:04:52PM +0200, Patrick McHardy wrote:
> Some minor cleanups, there doesn't seem to be a reason for not
> using the typeful helpers everywhere.

The reason for that was future IPv6 support.  But I suppose having
it as a be32 for now doesn't prevent us from changing it later, or
does it?

> I noticed the interface expects to always get a full configuration
> on change requests. Is there are particular reason for not
> supporting incremental changes, lets say
>
> "ip link change dev gre0 type gre remote <new remote>"
>
> ? It looks easy enough to change, so I could take care of this.

I think this should be done in iproute.  That way the user (or
rather the user-space programmer) gets to choose the behaviour.
This is also how the existing interface works too.

I'll update the iproute change to do this.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: gre: minor cleanups in netlink interface

[Herbert Xu]

On Sat, Oct 11, 2008 at 06:39:35PM +0800, Herbert Xu wrote:
> 
> I think this should be done in iproute.  That way the user (or
> rather the user-space programmer) gets to choose the behaviour.
> This is also how the existing interface works too.

But there is a stupid bug in my code that causes garbage to
show up in the configuration.

gre: Initialise rtnl_link tunnel parameters properly

Brown paper bag error of calling memset with sizeof(p) instead
of sizeof(*p).

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 0d5e35b..482abda 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -1345,7 +1345,7 @@ out:
 static void ipgre_netlink_parms(struct nlattr *data[],
 				struct ip_tunnel_parm *parms)
 {
-	memset(parms, 0, sizeof(parms));
+	memset(parms, 0, sizeof(*parms));
 
 	parms->iph.protocol = IPPROTO_GRE;
 

Re: gre: minor cleanups in netlink interface

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 11 Oct 2008 20:43:48 +0800

> gre: Initialise rtnl_link tunnel parameters properly
> 
> Brown paper bag error of calling memset with sizeof(p) instead
> of sizeof(*p).
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied to net-2.6, thanks Herbert.

Re: gre: minor cleanups in netlink interface

[Patrick McHardy]

Herbert Xu wrote:
> On Fri, Oct 10, 2008 at 06:04:52PM +0200, Patrick McHardy wrote:
>> Some minor cleanups, there doesn't seem to be a reason for not
>> using the typeful helpers everywhere.
> 
> The reason for that was future IPv6 support.  But I suppose having
> it as a be32 for now doesn't prevent us from changing it later, or
> does it?

No, its equivalant to using memcpy.

>> I noticed the interface expects to always get a full configuration
>> on change requests. Is there are particular reason for not
>> supporting incremental changes, lets say
>>
>> "ip link change dev gre0 type gre remote <new remote>"
>>
>> ? It looks easy enough to change, so I could take care of this.
> 
> I think this should be done in iproute.  That way the user (or
> rather the user-space programmer) gets to choose the behaviour.
> This is also how the existing interface works too.

We don't have much precedent for rtnl_link besides VLAN (which
does support incremental changes), but actually all other route
netlink interfaces do support incremental changes by sending only
a subset of the attributes. A reason for supporting this in the
interface is that incremental userspace changes will always be
racy because you need two seperate operations.

Re: gre: minor cleanups in netlink interface

[Patrick McHardy]

Patrick McHardy wrote:
> Herbert Xu wrote:
>>> I noticed the interface expects to always get a full configuration
>>> on change requests. Is there are particular reason for not
>>> supporting incremental changes, lets say
>>>
>>> "ip link change dev gre0 type gre remote <new remote>"
>>>
>>> ? It looks easy enough to change, so I could take care of this.
>>
>> I think this should be done in iproute.  That way the user (or
>> rather the user-space programmer) gets to choose the behaviour.
>> This is also how the existing interface works too.
> 
> We don't have much precedent for rtnl_link besides VLAN (which
> does support incremental changes), but actually all other route
> netlink interfaces do support incremental changes by sending only
> a subset of the attributes. A reason for supporting this in the
> interface is that incremental userspace changes will always be
> racy because you need two seperate operations.


Its actually also what defines the difference between using
NLM_F_REPLACE (ip link replace) and no flags (ip link change).
The former replaces the replaces the object, the later changes
an existing object.

Re: gre: minor cleanups in netlink interface

[Herbert Xu]

On Sat, Oct 11, 2008 at 04:45:48PM +0200, Patrick McHardy wrote:
>
> Its actually also what defines the difference between using
> NLM_F_REPLACE (ip link replace) and no flags (ip link change).
> The former replaces the replaces the object, the later changes
> an existing object.

That wasn't the case historically.  For routing NLM_F_REPLACE
had a real meaning because you can have routes that differ only
by scope.  So if you considered routes that are otherwise the
same to be duplicate routes, then NLM_F_REPLACE tells you whether
you're allowed to create a new route or you must modify one
of the existing ones.  Of course completely duplicate routes
where everything is equal is not allowed.

Of course NLM_F_REPLACE has since been seconded for other purposes
so I suppose it can mean whatever you want for rtnl link :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: gre: minor cleanups in netlink interface

[Patrick McHardy]

Herbert Xu wrote:
> On Sat, Oct 11, 2008 at 04:45:48PM +0200, Patrick McHardy wrote:
>> Its actually also what defines the difference between using
>> NLM_F_REPLACE (ip link replace) and no flags (ip link change).
>> The former replaces the replaces the object, the later changes
>> an existing object.
> 
> That wasn't the case historically.  For routing NLM_F_REPLACE
> had a real meaning because you can have routes that differ only
> by scope.  So if you considered routes that are otherwise the
> same to be duplicate routes, then NLM_F_REPLACE tells you whether
> you're allowed to create a new route or you must modify one
> of the existing ones.  Of course completely duplicate routes
> where everything is equal is not allowed.

Interesting, I wasn't aware of that.

> Of course NLM_F_REPLACE has since been seconded for other purposes
> so I suppose it can mean whatever you want for rtnl link :)

It actually not supported currently because replacement of links
(while keeping routes etc.) would be highly complicated.

Re: gre: minor cleanups in netlink interface

[Herbert Xu]

On Sat, Oct 11, 2008 at 04:43:03PM +0200, Patrick McHardy wrote:
>
> No, its equivalant to using memcpy.

Great.

> We don't have much precedent for rtnl_link besides VLAN (which
> does support incremental changes), but actually all other route
> netlink interfaces do support incremental changes by sending only
> a subset of the attributes. A reason for supporting this in the
> interface is that incremental userspace changes will always be
> racy because you need two seperate operations.

It is true that it is going to be racy when done in user-space,
however that's easily solved with locking.  Even if we did the
incremental change in the kernel it only helps certain kinds of
usage scenarios.  For instance, if the race is between two updates
to the local address you're still going to need synchronisation
in user-space.

Having said that, I'm certainly not against changing the interface
since you do have precedence with the other two :)

Do be warned that doing this for GRE is going to be less trivial
than the existing rtnl link interfaces.  For example, we'll need
to break down the iflags/oflags into individual bits as otherwise
you'll be back in the same situation.  It's a good thing that
there aren't too many bits in use :)

Also, for ikey/okey we'll need to introduce another attribute to
indicate their presence as well as their value.

Hmm, it seems that there is a bug in how we treat a zero key.
You can't have a tunnel with a zero key and one with no key at
the same time.

In fact my latest iproute patch has a similar problem.  You
can't unset the ikey/okey except by deleting the tunnel.  On
the other hand the old ip tunnel interface has the same bug :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: gre: minor cleanups in netlink interface

[Patrick McHardy]

Herbert Xu wrote:
> On Sat, Oct 11, 2008 at 04:43:03PM +0200, Patrick McHardy wrote:
>> We don't have much precedent for rtnl_link besides VLAN (which
>> does support incremental changes), but actually all other route
>> netlink interfaces do support incremental changes by sending only
>> a subset of the attributes. A reason for supporting this in the
>> interface is that incremental userspace changes will always be
>> racy because you need two seperate operations.
> 
> It is true that it is going to be racy when done in user-space,
> however that's easily solved with locking.  Even if we did the
> incremental change in the kernel it only helps certain kinds of
> usage scenarios.  For instance, if the race is between two updates
> to the local address you're still going to need synchronisation
> in user-space.

Thats true.

> Having said that, I'm certainly not against changing the interface
> since you do have precedence with the other two :)
> 
> Do be warned that doing this for GRE is going to be less trivial
> than the existing rtnl link interfaces.  For example, we'll need
> to break down the iflags/oflags into individual bits as otherwise
> you'll be back in the same situation.  It's a good thing that
> there aren't too many bits in use :)

We usually use two values (value + mask) for flags.

> Also, for ikey/okey we'll need to introduce another attribute to
> indicate their presence as well as their value.

The flags already indicate whether keys should be used, don't
they? So if you want to unset them, you can simply unset the
GRE_KEY flag.

I'll give it a shot and will post a patch, probably tommorrow.

> Hmm, it seems that there is a bug in how we treat a zero key.
> You can't have a tunnel with a zero key and one with no key at
> the same time.
> 
> In fact my latest iproute patch has a similar problem.  You
> can't unset the ikey/okey except by deleting the tunnel.  On
> the other hand the old ip tunnel interface has the same bug :)

I can't find it :)

Re: gre: minor cleanups in netlink interface

[Herbert Xu]

On Sat, Oct 11, 2008 at 05:15:40PM +0200, Patrick McHardy wrote:
>
> We usually use two values (value + mask) for flags.

Aha that should handle it nicely.

>> Also, for ikey/okey we'll need to introduce another attribute to
>> indicate their presence as well as their value.
>
> The flags already indicate whether keys should be used, don't
> they? So if you want to unset them, you can simply unset the
> GRE_KEY flag.

Right if you have the flag then it's not an issue.

> I'll give it a shot and will post a patch, probably tommorrow.

Thanks!

>> Hmm, it seems that there is a bug in how we treat a zero key.
>> You can't have a tunnel with a zero key and one with no key at
>> the same time.
>>
>> In fact my latest iproute patch has a similar problem.  You
>> can't unset the ikey/okey except by deleting the tunnel.  On
>> the other hand the old ip tunnel interface has the same bug :)
>
> I can't find it :)

If you mean the patch, then you're on the cc list :)

If you mean the bug in ip tunnel, I was going by the fact that
there is nothing that unsets the GRE_KEY flag.  Actually let's
not worry about this one too much because you can't unset the
checksum flags either and nobody has complained about either
of these problems for a decade.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt