| Message ID | 1290524559-22086-1-git-send-email-timo.teras@iki.fi |
|---|---|
| State | Superseded, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On Tue, 23 Nov 2010 17:02:39 +0200 Timo Teräs <timo.teras@iki.fi> wrote: > + case IPPROTO_GRE: > + if (sel->sport_mask || sel->dport_mask) { > + struct in_addr key; > + key.s_addr = htonl((ntohs(sel->sport) << 16) + ntohs(sel->dport)); > + inet_ntop(AF_INET, &key, abuf, sizeof(abuf)); > + fprintf(fp, "key %s ", abuf); > + } The GRE key is not really an IPv4 address. Why should the utilities use IPv4 address manipulation to format/scan it. It makes more sense to me to just use u32 an do the necessary ntohl.
On 11/23/2010 06:24 PM, Stephen Hemminger wrote: > On Tue, 23 Nov 2010 17:02:39 +0200 > Timo Teräs <timo.teras@iki.fi> wrote: > >> + case IPPROTO_GRE: >> + if (sel->sport_mask || sel->dport_mask) { >> + struct in_addr key; >> + key.s_addr = htonl((ntohs(sel->sport) << 16) + ntohs(sel->dport)); >> + inet_ntop(AF_INET, &key, abuf, sizeof(abuf)); >> + fprintf(fp, "key %s ", abuf); >> + } > > The GRE key is not really an IPv4 address. Why should the utilities > use IPv4 address manipulation to format/scan it. It makes more sense > to me to just use u32 an do the necessary ntohl. This is pretty much how iptunnel.c does it, so I copied the code. Would you prefer to format it as single u32 number? Or use something else for formatting it similar to IPv4? In either case, we should change iptunnel.c to match ipxfrm.c. It'll be easier if both parts handling the gre key treat it equivalently. I think Cisco does indeed treat it as u32 number in the configurations. So I'm okay updating this patch, and fixing iptunnel.c side too. We might still want to keep the parsing of ipv4 format to keep backwards compatibility. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 23 Nov 2010 18:44:44 +0200 Timo Teräs <timo.teras@iki.fi> wrote: > On 11/23/2010 06:24 PM, Stephen Hemminger wrote: > > On Tue, 23 Nov 2010 17:02:39 +0200 > > Timo Teräs <timo.teras@iki.fi> wrote: > > > >> + case IPPROTO_GRE: > >> + if (sel->sport_mask || sel->dport_mask) { > >> + struct in_addr key; > >> + key.s_addr = htonl((ntohs(sel->sport) << 16) + ntohs(sel->dport)); > >> + inet_ntop(AF_INET, &key, abuf, sizeof(abuf)); > >> + fprintf(fp, "key %s ", abuf); > >> + } > > > > The GRE key is not really an IPv4 address. Why should the utilities > > use IPv4 address manipulation to format/scan it. It makes more sense > > to me to just use u32 an do the necessary ntohl. > > This is pretty much how iptunnel.c does it, so I copied the code. Would > you prefer to format it as single u32 number? Or use something else for > formatting it similar to IPv4? > > In either case, we should change iptunnel.c to match ipxfrm.c. It'll be > easier if both parts handling the gre key treat it equivalently. > > I think Cisco does indeed treat it as u32 number in the configurations. > So I'm okay updating this patch, and fixing iptunnel.c side too. We > might still want to keep the parsing of ipv4 format to keep backwards > compatibility. My preference would be to take both dotted quad and a single number.
On 11/23/2010 08:13 PM, Stephen Hemminger wrote: > On Tue, 23 Nov 2010 18:44:44 +0200 > Timo Teräs <timo.teras@iki.fi> wrote: >> On 11/23/2010 06:24 PM, Stephen Hemminger wrote: >>> The GRE key is not really an IPv4 address. Why should the utilities >>> use IPv4 address manipulation to format/scan it. It makes more sense >>> to me to just use u32 an do the necessary ntohl. >> >> This is pretty much how iptunnel.c does it, so I copied the code. Would >> you prefer to format it as single u32 number? Or use something else for >> formatting it similar to IPv4? >> >> In either case, we should change iptunnel.c to match ipxfrm.c. It'll be >> easier if both parts handling the gre key treat it equivalently. >> >> I think Cisco does indeed treat it as u32 number in the configurations. >> So I'm okay updating this patch, and fixing iptunnel.c side too. We >> might still want to keep the parsing of ipv4 format to keep backwards >> compatibility. > > My preference would be to take both dotted quad and a single > number. And I assume that when dumping stuff, it should be output as single number. Or make that configurable with some switch? I'll refresh my patch and fix iptunnel.c output tomorrow. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 23 Nov 2010 20:38:58 +0200 Timo Teräs <timo.teras@iki.fi> wrote: > On 11/23/2010 08:13 PM, Stephen Hemminger wrote: > > On Tue, 23 Nov 2010 18:44:44 +0200 > > Timo Teräs <timo.teras@iki.fi> wrote: > >> On 11/23/2010 06:24 PM, Stephen Hemminger wrote: > >>> The GRE key is not really an IPv4 address. Why should the utilities > >>> use IPv4 address manipulation to format/scan it. It makes more sense > >>> to me to just use u32 an do the necessary ntohl. > >> > >> This is pretty much how iptunnel.c does it, so I copied the code. Would > >> you prefer to format it as single u32 number? Or use something else for > >> formatting it similar to IPv4? > >> > >> In either case, we should change iptunnel.c to match ipxfrm.c. It'll be > >> easier if both parts handling the gre key treat it equivalently. > >> > >> I think Cisco does indeed treat it as u32 number in the configurations. > >> So I'm okay updating this patch, and fixing iptunnel.c side too. We > >> might still want to keep the parsing of ipv4 format to keep backwards > >> compatibility. > > > > My preference would be to take both dotted quad and a single > > number. > > And I assume that when dumping stuff, it should be output as single > number. Or make that configurable with some switch? > > I'll refresh my patch and fix iptunnel.c output tomorrow. Just show it as a decimal number. That is what Cisco and Juniper do.
On Tue, 2010-11-23 at 10:13 -0800, Stephen Hemminger wrote: > On Tue, 23 Nov 2010 18:44:44 +0200 > Timo Teräs <timo.teras@iki.fi> wrote: > > > On 11/23/2010 06:24 PM, Stephen Hemminger wrote: > > > On Tue, 23 Nov 2010 17:02:39 +0200 > > > Timo Teräs <timo.teras@iki.fi> wrote: > > > > > >> + case IPPROTO_GRE: > > >> + if (sel->sport_mask || sel->dport_mask) { > > >> + struct in_addr key; > > >> + key.s_addr = htonl((ntohs(sel->sport) << 16) + ntohs(sel->dport)); > > >> + inet_ntop(AF_INET, &key, abuf, sizeof(abuf)); > > >> + fprintf(fp, "key %s ", abuf); > > >> + } > > > > > > The GRE key is not really an IPv4 address. Why should the utilities > > > use IPv4 address manipulation to format/scan it. It makes more sense > > > to me to just use u32 an do the necessary ntohl. > > > > This is pretty much how iptunnel.c does it, so I copied the code. Would > > you prefer to format it as single u32 number? Or use something else for > > formatting it similar to IPv4? > > > > In either case, we should change iptunnel.c to match ipxfrm.c. It'll be > > easier if both parts handling the gre key treat it equivalently. > > > > I think Cisco does indeed treat it as u32 number in the configurations. > > So I'm okay updating this patch, and fixing iptunnel.c side too. We > > might still want to keep the parsing of ipv4 format to keep backwards > > compatibility. > > My preference would be to take both dotted quad and a single > number. inet_aton() covers that. Ben.
diff --git a/ip/ipxfrm.c b/ip/ipxfrm.c index 99a6756..cf4b7c7 100644 --- a/ip/ipxfrm.c +++ b/ip/ipxfrm.c @@ -35,6 +35,7 @@ #include <linux/netlink.h> #include <linux/rtnetlink.h> #include <linux/xfrm.h> +#include <arpa/inet.h> #include "utils.h" #include "xfrm.h" @@ -483,6 +484,14 @@ void xfrm_selector_print(struct xfrm_selector *sel, __u16 family, if (sel->dport_mask) fprintf(fp, "code %u ", ntohs(sel->dport)); break; + case IPPROTO_GRE: + if (sel->sport_mask || sel->dport_mask) { + struct in_addr key; + key.s_addr = htonl((ntohs(sel->sport) << 16) + ntohs(sel->dport)); + inet_ntop(AF_INET, &key, abuf, sizeof(abuf)); + fprintf(fp, "key %s ", abuf); + } + break; case IPPROTO_MH: if (sel->sport_mask) fprintf(fp, "type %u ", ntohs(sel->sport)); @@ -1086,6 +1095,7 @@ static int xfrm_selector_upspec_parse(struct xfrm_selector *sel, char *dportp = NULL; char *typep = NULL; char *codep = NULL; + char *grekey = NULL; while (1) { if (strcmp(*argv, "proto") == 0) { @@ -1162,6 +1172,29 @@ static int xfrm_selector_upspec_parse(struct xfrm_selector *sel, filter.upspec_dport_mask = XFRM_FILTER_MASK_FULL; + } else if (strcmp(*argv, "key") == 0) { + unsigned key; + + grekey = *argv; + + NEXT_ARG(); + + if (strchr(*argv, '.')) + key = htonl(get_addr32(*argv)); + else { + if (get_unsigned(&key, *argv, 0)<0) { + fprintf(stderr, "invalid value of \"key\"\n"); + exit(-1); + } + } + + sel->sport = htons(key >> 16); + sel->dport = htons(key & 0xffff); + sel->sport_mask = ~((__u16)0); + sel->dport_mask = ~((__u16)0); + + filter.upspec_dport_mask = XFRM_FILTER_MASK_FULL; + } else { PREV_ARG(); /* back track */ break; @@ -1196,6 +1229,15 @@ static int xfrm_selector_upspec_parse(struct xfrm_selector *sel, exit(1); } } + if (grekey) { + switch (sel->proto) { + case IPPROTO_GRE: + break; + default: + fprintf(stderr, "\"key\" is invalid with proto=%s\n", strxf_proto(sel->proto)); + exit(1); + } + } *argcp = argc; *argvp = argv; diff --git a/ip/xfrm_policy.c b/ip/xfrm_policy.c index 121afa1..dcb3da4 100644 --- a/ip/xfrm_policy.c +++ b/ip/xfrm_policy.c @@ -66,7 +66,8 @@ static void usage(void) fprintf(stderr, "SELECTOR := src ADDR[/PLEN] dst ADDR[/PLEN] [ UPSPEC ] [ dev DEV ]\n"); fprintf(stderr, "UPSPEC := proto PROTO [ [ sport PORT ] [ dport PORT ] |\n"); - fprintf(stderr, " [ type NUMBER ] [ code NUMBER ] ]\n"); + fprintf(stderr, " [ type NUMBER ] [ code NUMBER ] |\n"); + fprintf(stderr, " [ key { DOTTED_QUAD | NUMBER } ] ]\n"); //fprintf(stderr, "DEV - device name(default=none)\n"); diff --git a/man/man8/ip.8 b/man/man8/ip.8 index 1a73efa..c1e03f3 100644 --- a/man/man8/ip.8 +++ b/man/man8/ip.8 @@ -547,7 +547,10 @@ throw " | " unreachable " | " prohibit " | " blackhole " | " nat " ]" .RB " [ " type .IR NUMBER " ] " .RB " [ " code -.IR NUMBER " ]] " +.IR NUMBER " ] | " +.br +.RB " [ " key +.IR KEY " ]] " .ti -8 .IR LIMIT-LIST " := [ " LIMIT-LIST " ] |" @@ -642,7 +645,10 @@ throw " | " unreachable " | " prohibit " | " blackhole " | " nat " ]" .RB " [ " type .IR NUMBER " ] " .RB " [ " code -.IR NUMBER " ] ] " +.IR NUMBER " ] | " +.br +.RB " [ " key +.IR KEY " ] ] " .ti -8 .IR ACTION " := " @@ -2487,9 +2493,11 @@ is defined by source port .BR sport ", " destination port .BR dport ", " type -as number and +as number, .B code -also number. +also number and +.BR key +as dotted-quad or number. .TP .BI dev " DEV " @@ -2556,11 +2564,10 @@ and the other choice is .TP .IR UPSPEC is specified by -.BR sport ", " -.BR dport ", " type -and -.B code -(NUMBER). +.BR sport " and " dport " (for UDP/TCP), " +.BR type " and " code " (for ICMP; as number) or " +.BR key " (for GRE; as dotted-quad or number)." +. .SS ip xfrm monitor - is used for listing all objects or defined group of them. The
The gre key handling is consistent with ip tunnel side: both dotted-quad and number are accepted, but dotted-quad is used for printing. Signed-off-by: Timo Teräs <timo.teras@iki.fi> --- This is the userland part for: http://git.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commitdiff;h=cc9ff19da9bf76a2f70bcb80225a1c587c162e52 However, that commit is flawed, and for this patch to work properly, the following patch is needed: http://patchwork.ozlabs.org/patch/72668/ ip/ipxfrm.c | 42 ++++++++++++++++++++++++++++++++++++++++++ ip/xfrm_policy.c | 3 ++- man/man8/ip.8 | 25 ++++++++++++++++--------- 3 files changed, 60 insertions(+), 10 deletions(-)