Skip to contentSkip to conversation

: Fix ICMP tempsel

XFRM Patrick McHardy 4 participants 14 messages Feb 19, 2005 – Mar 10, 2005

Subsystems (3)
Networking Network drivers Networking IPsec
lore
Share & tools
lore

Patrick McHardy

author
#0 lore raw

The selector ports are initialized to fl_ip_sport/fl_ip_dport instead of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP, type and code should be stored in sport and dport, in struct flowi both are contained in fl_ip_sport.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2005/02/19 12:35:18+01:00 [email protected]
# [XFRM]: Fix ICMP tempsel
#
# Signed-off-by: Patrick McHardy <[email protected]>
#

# net/ipv6/xfrm6_state.c
#   2005/02/19 12:35:06+01:00 [email protected] +2 -2
#   [XFRM]: Fix ICMP tempsel
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# net/ipv4/xfrm4_state.c
#   2005/02/19 12:35:06+01:00 [email protected] +2 -2
#   [XFRM]: Fix ICMP tempsel
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
diff -Nru a/net/ipv4/xfrm4_state.c b/net/ipv4/xfrm4_state.c
--- a/net/ipv4/xfrm4_state.c	2005-02-19 12:36:31 +01:00
+++ b/net/ipv4/xfrm4_state.c	2005-02-19 12:36:31 +01:00
@@ -20,9 +20,9 @@
 {
 	x->sel.daddr.a4 = fl->fl4_dst;
 	x->sel.saddr.a4 = fl->fl4_src;
-	x->sel.dport = fl->fl_ip_dport;
+	x->sel.dport = xfrm_flowi_dport(fl);
 	x->sel.dport_mask = ~0;
-	x->sel.sport = fl->fl_ip_sport;
+	x->sel.sport = xfrm_flowi_sport(fl);
 	x->sel.sport_mask = ~0;
 	x->sel.prefixlen_d = 32;
 	x->sel.prefixlen_s = 32;
diff -Nru a/net/ipv6/xfrm6_state.c b/net/ipv6/xfrm6_state.c
--- a/net/ipv6/xfrm6_state.c	2005-02-19 12:36:31 +01:00
+++ b/net/ipv6/xfrm6_state.c	2005-02-19 12:36:31 +01:00
@@ -27,9 +27,9 @@
 	 * to current session. */
 	ipv6_addr_copy((struct in6_addr *)&x->sel.daddr, &fl->fl6_dst);
 	ipv6_addr_copy((struct in6_addr *)&x->sel.saddr, &fl->fl6_src);
-	x->sel.dport = fl->fl_ip_dport;
+	x->sel.dport = xfrm_flowi_dport(fl);
 	x->sel.dport_mask = ~0;
-	x->sel.sport = fl->fl_ip_sport;
+	x->sel.sport = xfrm_flowi_sport(fl);
 	x->sel.sport_mask = ~0;
 	x->sel.prefixlen_d = 128;
 	x->sel.prefixlen_s = 128;
#1 lore raw

In article <[email protected]> (at Sat, 19 Feb 2005 12:43:43 +0100), Patrick McHardy <[email protected]> says:

Quoted 4 lines

The selector ports are initialized to fl_ip_sport/fl_ip_dport instead of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP, type and code should be stored in sport and dport, in struct flowi both are contained in fl_ip_sport.

I agree.

--yoshfuji

maintainer
#2 lore raw
In reply to Patrick McHardy
Quoted 4 lines · Patrick McHardy

The selector ports are initialized to fl_ip_sport/fl_ip_dport instead of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP, type and code should be stored in sport and dport, in struct flowi both are contained in fl_ip_sport.

I know this comment is probably a bit late but why didn't we simply put type/code into sport/dport in struct flowi instead of introducing the monstrosities of xfrm_flowi_sport/xfrm_flowi_dport?

Something like

struct {
	__u16	type;
	__u16	code;
} icmpt;

would've done (and still would do) the trick, no?

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#3 lore raw
Quoted 4 lines · Herbert Xu

I know this comment is probably a bit late but why didn't we simply put type/code into sport/dport in struct flowi instead of introducing the monstrosities of xfrm_flowi_sport/xfrm_flowi_dport?

Quoted 9 lines

Something like

struct {
	__u16	type;
	__u16	code;
} icmpt;

would've done (and still would do) the trick, no?

I agree, that is better.

Regards
Patrick

author
#4 lore raw
In reply to Herbert Xu
Quoted 4 lines · Herbert Xu

I know this comment is probably a bit late but why didn't we simply put type/code into sport/dport in struct flowi instead of introducing the monstrosities of xfrm_flowi_sport/xfrm_flowi_dport?

Quoted 8 lines

Something like

struct {
	__u16	type;
	__u16	code;
} icmpt;

would've done (and still would do) the trick, no?

Here is an updated patch that kills xfrm_flowi_{sport,dport}. I've checked around, there seems to be nothing that relies on type and code beeing u8.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2005/02/20 06:19:59+01:00 [email protected]
# [XFRM]: Fix ICMP tempsel
#

#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#   
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport
#   and kills xfrm_flowi_sport/dport instead of changing xfrm_init_tempsel(),
#   as suggested by Herbert Xu.
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# include/net/xfrm.h
#   2005/02/20 06:19:50+01:00 [email protected] +4 -44
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#   
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport
#   and kills xfrm_flowi_sport/dport instead of changing xfrm_init_tempsel(),
#   as suggested by Herbert Xu.
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# include/net/flow.h
#   2005/02/20 06:19:50+01:00 [email protected] +2 -2
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#   
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport
#   and kills xfrm_flowi_sport/dport instead of changing xfrm_init_tempsel(),
#   as suggested by Herbert Xu.
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
diff -Nru a/include/net/flow.h b/include/net/flow.h
--- a/include/net/flow.h	2005-02-20 06:20:34 +01:00
+++ b/include/net/flow.h	2005-02-20 06:20:34 +01:00
@@ -58,8 +58,8 @@
 		} ports;

struct {
- __u8 type;
- __u8 code;
+ __u16 type;
+ __u16 code;
} icmpt;

struct {

include/net/xfrm.h +4 -44
diff -Nru a/include/net/xfrm.h b/include/net/xfrm.h
@@ -417,53 +417,13 @@
 	return 1;
 }
 
-static __inline__
-u16 xfrm_flowi_sport(struct flowi *fl)
-{
-	u16 port;
-	switch(fl->proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_SCTP:
-		port = fl->fl_ip_sport;
-		break;
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6:
-		port = htons(fl->fl_icmp_type);
-		break;
-	default:
-		port = 0;	/*XXX*/
-	}
-	return port;
-}
-
-static __inline__
-u16 xfrm_flowi_dport(struct flowi *fl)
-{
-	u16 port;
-	switch(fl->proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_SCTP:
-		port = fl->fl_ip_dport;
-		break;
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6:
-		port = htons(fl->fl_icmp_code);
-		break;
-	default:
-		port = 0;	/*XXX*/
-	}
-	return port;
-}
-
 static inline int
 __xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
 {
 	return  addr_match(&fl->fl4_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl4_src, &sel->saddr, sel->prefixlen_s) &&
-		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
-		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
+		!((fl->fl_ip_dport ^ sel->dport) & sel->dport_mask) &&
+		!((fl->fl_ip_sport ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }

@@ -473,8 +433,8 @@
 {
 	return  addr_match(&fl->fl6_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl6_src, &sel->saddr, sel->prefixlen_s) &&
-		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
-		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
+		!((fl->fl_ip_dport ^ sel->dport) & sel->dport_mask) &&
+		!((fl->fl_ip_sport ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }
#5 lore raw

In article <[email protected]> (at Sun, 20 Feb 2005 06:30:42 +0100), Patrick McHardy <[email protected]> says:

Quoted 13 lines
Quoted 11 lines · Herbert Xu

I know this comment is probably a bit late but why didn't we simply put type/code into sport/dport in struct flowi instead of introducing the monstrosities of xfrm_flowi_sport/xfrm_flowi_dport?

Something like

struct {
	__u16	type;
	__u16	code;
} icmpt;
Quoted 5 lines
Quoted 1 line

would've done (and still would do) the trick, no?

Here is an updated patch that kills xfrm_flowi_{sport,dport}. I've checked around, there seems to be nothing that relies on type and code beeing u8.

I didn't this because there are several places which depend on u8. If we go this way, we need to fix other places as well. e.g.

net/ipv4/raw.c:raw_probe_proto_opt()
net/ipv4/xfrm4_policy.c:_decode_session4()
net/ipv6/raw.c:rawv6_probe_proto_opt()
net/ipv6/netfilter/ip6t_REJECT.c:send_unreach()
net/ipv6/xfrm6_policy.c:_decode_session6()
net/ipv6/ndisc.c:ndisc_flow_init()
net/ipv6/icmp.c:icmpv6_send()
net/ipv6/icmp.c:icmpv6_echo_reply()

(Note that type and code are stored in network-byte order.)

--yoshfuji

author
#6 lore raw
Quoted 4 lines · YOSHIFUJI Hideaki / $B5HF#1QL@ wrote:

I didn't this because there are several places which depend on u8. If we go this way, we need to fix other places as well. e.g.

net/ipv4/raw.c:raw_probe_proto_opt()

                                get_user(fl->fl_icmp_type, type);
                                __get_user(fl->fl_icmp_code, code);

On x86_64 both care only about the pointer type, not the target type.

Quoted 1 line

net/ipv4/xfrm4_policy.c:_decode_session4()

                                u8 *icmp = xprth;

                                fl->fl_icmp_type = icmp[0];
                                fl->fl_icmp_code = icmp[1];

No problem here.

Quoted 1 line

net/ipv6/raw.c:rawv6_probe_proto_opt()

Same as IPv4.

Quoted 1 line

net/ipv6/netfilter/ip6t_REJECT.c:send_unreach()

Not in mainline :)

Quoted 1 line

net/ipv6/xfrm6_policy.c:_decode_session6()

Same as IPv4.

Quoted 1 line

net/ipv6/ndisc.c:ndisc_flow_init()

        fl->fl_icmp_type        = type;
        fl->fl_icmp_code        = 0;

No problem.

Quoted 1 line

net/ipv6/icmp.c:icmpv6_send()

        fl.fl_icmp_type = type;
        fl.fl_icmp_code = code;

Also ok.

Quoted 1 line

net/ipv6/icmp.c:icmpv6_echo_reply()

        fl.fl_icmp_type = ICMPV6_ECHO_REPLY;

Same here.

Quoted 2 lines

(Note that type and code are stored in network-byte order.)

Both are u8 so there is no byte order.

Regards
Patrick

maintainer
#7 lore raw
In reply to YOSHIFUJI Hideaki / 吉藤英明
Quoted 3 lines · On Sun, Feb 20, 2005 at 03:55:27PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:

I didn't this because there are several places which depend on u8. If we go this way, we need to fix other places as well. e.g.

Good point. However, I think it's better to fix things up once when we cross from user-space to the kernel rather than converting it for every single packet.

So here we can do something like this. Please note that this patch needs to be used with Patrick's earlier work. Patrick, if you're OK with it please merge it in with your patch.

By all means give xfrm_selector_fixup a better name :)

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

===== net/key/af_key.c 1.69 vs edited =====

net/key/af_key.c +3 -0
@@ -1914,6 +1914,8 @@
 	if (xp->selector.dport)
 		xp->selector.dport_mask = ~0;
 
+	xfrm_selector_fixup(&xp->selector);
+
 	xp->lft.soft_byte_limit = XFRM_INF;
 	xp->lft.hard_byte_limit = XFRM_INF;
 	xp->lft.soft_packet_limit = XFRM_INF;

@@ -2004,6 +2006,7 @@
 	if (sel.dport)
 		sel.dport_mask = ~0;
 
+	xfrm_selector_fixup(&sel);
 	xp = xfrm_policy_bysel(pol->sadb_x_policy_dir-1, &sel, 1);
 	if (xp == NULL)
 		return -ENOENT;

===== net/xfrm/xfrm_user.c 1.52 vs edited =====

net/xfrm/xfrm_user.c +7 -1
@@ -204,6 +204,7 @@
 {
 	memcpy(&x->id, &p->id, sizeof(x->id));
 	memcpy(&x->sel, &p->sel, sizeof(x->sel));
+	xfrm_selector_fixup(&x->sel);
 	memcpy(&x->lft, &p->lft, sizeof(x->lft));
 	x->props.mode = p->mode;
 	x->props.replay_window = p->replay_window;

@@ -626,6 +627,7 @@
 	xp->priority = p->priority;
 	xp->index = p->index;
 	memcpy(&xp->selector, &p->sel, sizeof(xp->selector));
+	xfrm_selector_fixup(&xp->selector);
 	memcpy(&xp->lft, &p->lft, sizeof(xp->lft));
 	xp->action = p->action;
 	xp->flags = p->flags;

@@ -808,6 +810,7 @@
 	struct xfrm_userpolicy_id *p;
 	int err;
 	int delete;
+	struct xfrm_selector sel;
 
 	p = NLMSG_DATA(nlh);
 	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;

@@ -818,8 +821,11 @@
 
 	if (p->index)
 		xp = xfrm_policy_byid(p->dir, p->index, delete);
-	else
+	else {
+		memcpy(&sel, &p->sel, sizeof(sel));
+		xfrm_selector_fixup(&sel);
 		xp = xfrm_policy_bysel(p->dir, &p->sel, delete);
+	}
 	if (xp == NULL)
 		return -ENOENT;

===== include/net/flow.h 1.11 vs edited =====

include/net/flow.h +2 -0
@@ -58,7 +58,9 @@
 		} ports;
 
 		struct {
+			__u8	pad1;
 			__u8	type;
+			__u8	pad2;
 			__u8	code;
 		} icmpt;

===== include/net/xfrm.h 1.74 vs edited =====

include/net/xfrm.h +11 -0
@@ -492,6 +492,17 @@
 	return 0;
 }
 
+static inline void xfrm_selector_fixup(struct xfrm_selector *sel)
+{
+	switch (sel->proto) {
+	case IPPROTO_ICMP:
+	case IPPROTO_ICMPV6:
+		sel->sport_mask &= htons(0xff);
+		sel->dport_mask &= htons(0xff);
+		break;
+	}
+}
+
 /* A struct encoding bundle of transformations to apply to some set of flow.
  *
  * dst->child points to the next element of bundle.
author
#8 lore raw
Quoted 3 lines · Herbert Xu

So here we can do something like this. Please note that this patch needs to be used with Patrick's earlier work. Patrick, if you're OK with it please merge it in with your patch.

Attached. Now I understand what Yoshifuji meant with byteorder :)

Quoted 2 lines

By all means give xfrm_selector_fixup a better name :)

I'm not very talented with choosing good names myself, so I kept it.

On second thought .. isn't there a risk of confusing userspace by changing the masks ?

Regards
Patrick

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2005/02/20 09:29:12+01:00 [email protected]
# [XFRM]: Fix ICMP tempsel
#

#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.

#
# Mostly done by Herbert Xu <[email protected]>
#
# Signed-off-by: Patrick McHardy <[email protected]>
#

# net/xfrm/xfrm_user.c
#   2005/02/20 09:29:04+01:00 [email protected] +7 -1
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <[email protected]>
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# net/key/af_key.c
#   2005/02/20 09:29:04+01:00 [email protected] +3 -0
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <[email protected]>
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# include/net/xfrm.h
#   2005/02/20 09:29:04+01:00 [email protected] +15 -44
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <[email protected]>
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
# include/net/flow.h
#   2005/02/20 09:29:04+01:00 [email protected] +2 -0
#   [XFRM]: Fix ICMP tempsel
#   
#   The selector ports are initialized to fl_ip_sport/fl_ip_dport instead
#   of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP,
#   type and code should be stored in sport and dport, in struct flowi both
#   are contained in fl_ip_sport.
#      
#   This patch adjusts struct flowi to store ICMP type/code in sport/dport,
#   kills xfrm_flowi_{sport,dport} and converts the selector values only once
#   when they enter the kernel.
#   
#   Mostly done by Herbert Xu <[email protected]>
#   
#   Signed-off-by: Patrick McHardy <[email protected]>
# 
diff -Nru a/include/net/flow.h b/include/net/flow.h
--- a/include/net/flow.h	2005-02-20 09:30:04 +01:00
+++ b/include/net/flow.h	2005-02-20 09:30:04 +01:00
@@ -58,7 +58,9 @@
 		} ports;

struct {
+ __u8 pad1;
__u8 type;
+ __u8 pad2;
__u8 code;
} icmpt;

include/net/xfrm.h +15 -44
diff -Nru a/include/net/xfrm.h b/include/net/xfrm.h
@@ -417,53 +417,13 @@
 	return 1;
 }
 
-static __inline__
-u16 xfrm_flowi_sport(struct flowi *fl)
-{
-	u16 port;
-	switch(fl->proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_SCTP:
-		port = fl->fl_ip_sport;
-		break;
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6:
-		port = htons(fl->fl_icmp_type);
-		break;
-	default:
-		port = 0;	/*XXX*/
-	}
-	return port;
-}
-
-static __inline__
-u16 xfrm_flowi_dport(struct flowi *fl)
-{
-	u16 port;
-	switch(fl->proto) {
-	case IPPROTO_TCP:
-	case IPPROTO_UDP:
-	case IPPROTO_SCTP:
-		port = fl->fl_ip_dport;
-		break;
-	case IPPROTO_ICMP:
-	case IPPROTO_ICMPV6:
-		port = htons(fl->fl_icmp_code);
-		break;
-	default:
-		port = 0;	/*XXX*/
-	}
-	return port;
-}
-
 static inline int
 __xfrm4_selector_match(struct xfrm_selector *sel, struct flowi *fl)
 {
 	return  addr_match(&fl->fl4_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl4_src, &sel->saddr, sel->prefixlen_s) &&
-		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
-		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
+		!((fl->fl_ip_dport ^ sel->dport) & sel->dport_mask) &&
+		!((fl->fl_ip_sport ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }

@@ -473,8 +433,8 @@
 {
 	return  addr_match(&fl->fl6_dst, &sel->daddr, sel->prefixlen_d) &&
 		addr_match(&fl->fl6_src, &sel->saddr, sel->prefixlen_s) &&
-		!((xfrm_flowi_dport(fl) ^ sel->dport) & sel->dport_mask) &&
-		!((xfrm_flowi_sport(fl) ^ sel->sport) & sel->sport_mask) &&
+		!((fl->fl_ip_dport ^ sel->dport) & sel->dport_mask) &&
+		!((fl->fl_ip_sport ^ sel->sport) & sel->sport_mask) &&
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }

@@ -490,6 +450,17 @@
 		return __xfrm6_selector_match(sel, fl);
 	}
 	return 0;
+}
+
+static inline void xfrm_selector_fixup(struct xfrm_selector *sel)
+{
+	switch (sel->proto) {
+	case IPPROTO_ICMP:
+	case IPPROTO_ICMPV6:
+		sel->sport_mask &= htons(0xff);
+		sel->dport_mask &= htons(0xff);
+		break;
+	}
 }
 
 /* A struct encoding bundle of transformations to apply to some set of flow.
net/key/af_key.c +3 -0
diff -Nru a/net/key/af_key.c b/net/key/af_key.c
@@ -1909,6 +1909,8 @@
 	if (xp->selector.dport)
 		xp->selector.dport_mask = ~0;
 
+	xfrm_selector_fixup(&xp->selector);
+
 	xp->lft.soft_byte_limit = XFRM_INF;
 	xp->lft.hard_byte_limit = XFRM_INF;
 	xp->lft.soft_packet_limit = XFRM_INF;

@@ -1999,6 +2001,7 @@
 	if (sel.dport)
 		sel.dport_mask = ~0;
 
+	xfrm_selector_fixup(&sel);
 	xp = xfrm_policy_bysel(pol->sadb_x_policy_dir-1, &sel, 1);
 	if (xp == NULL)
 		return -ENOENT;
net/xfrm/xfrm_user.c +7 -1
diff -Nru a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
@@ -204,6 +204,7 @@
 {
 	memcpy(&x->id, &p->id, sizeof(x->id));
 	memcpy(&x->sel, &p->sel, sizeof(x->sel));
+	xfrm_selector_fixup(&x->sel);
 	memcpy(&x->lft, &p->lft, sizeof(x->lft));
 	x->props.mode = p->mode;
 	x->props.replay_window = p->replay_window;

@@ -626,6 +627,7 @@
 	xp->priority = p->priority;
 	xp->index = p->index;
 	memcpy(&xp->selector, &p->sel, sizeof(xp->selector));
+	xfrm_selector_fixup(&xp->selector);
 	memcpy(&xp->lft, &p->lft, sizeof(xp->lft));
 	xp->action = p->action;
 	xp->flags = p->flags;

@@ -808,6 +810,7 @@
 	struct xfrm_userpolicy_id *p;
 	int err;
 	int delete;
+	struct xfrm_selector sel;
 
 	p = NLMSG_DATA(nlh);
 	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;

@@ -818,8 +821,11 @@
 
 	if (p->index)
 		xp = xfrm_policy_byid(p->dir, p->index, delete);
-	else
+	else {
+		memcpy(&sel, &p->sel, sizeof(sel));
+		xfrm_selector_fixup(&sel);
 		xp = xfrm_policy_bysel(p->dir, &p->sel, delete);
+	}
 	if (xp == NULL)
 		return -ENOENT;
#9 lore raw

In article <[email protected]> (at Sun, 20 Feb 2005 09:35:14 +0100), Patrick McHardy <[email protected]> says:

Quoted 6 lines
Quoted 3 lines · Herbert Xu

So here we can do something like this. Please note that this patch needs to be used with Patrick's earlier work. Patrick, if you're OK with it please merge it in with your patch.

Attached. Now I understand what Yoshifuji meant with byteorder :)

Sorry, still it seems wrong.
Please allow me several hours.

--yoshfuji @ on the way home...

maintainer
#10 lore raw
In reply to Patrick McHardy
Quoted 3 lines · Patrick McHardy

On second thought .. isn't there a risk of confusing userspace by changing the masks ?

It only affects xfrm_user users, of which I only know two -- Openswan and ip(8). Neither of which should care about it.

However, I forgot to do the fixup in __xfrm[46]_init_tempsel and that is going to confuse the kernel itself :)

It is looking more and more like a nasty hack though so maybe it's not worth it since the policy checks are still bloated enough even after we do this.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#11 lore raw
Quoted 3 lines · Herbert Xu

It is looking more and more like a nasty hack though so maybe it's not worth it since the policy checks are still bloated enough even after we do this.

Agreed. Let's just take the first patch.

Regards
Patrick

#12 lore raw

In article <[email protected]> (at Sun, 20 Feb 2005 12:20:58 +0100), Patrick McHardy <[email protected]> says:

Quoted 6 lines
Quoted 3 lines · Herbert Xu

It is looking more and more like a nasty hack though so maybe it's not worth it since the policy checks are still bloated enough even after we do this.

Agreed. Let's just take the first patch.

I totally agree. I belive it is the safest fix.

--yoshfuji

maintainer
#13 lore raw
In reply to Patrick McHardy
Quoted 4 lines · On Sat, 19 Feb 2005 12:43:43 +0100 Patrick McHardy <[email protected]> wrote:

The selector ports are initialized to fl_ip_sport/fl_ip_dport instead of xfrm_flowi_sport(fl)/xfrm_flowi_dport(fl). This is wrong for ICMP, type and code should be stored in sport and dport, in struct flowi both are contained in fl_ip_sport.

This resulted in a long thread, many newer versions of the patch trying to clean this up in other ways, but then we determined that this original patch was the best and safest fix for now.

So I applied this first patch.

Thanks everyone.