Re: [XFRM]: Fix ICMP tempsel

[Patrick McHardy <kaber@xxxxxxxxx>]

Herbert Xu wrote:

> 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?

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 kaber@xxxxxxxxxxxx 
#   [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 <kaber@xxxxxxxxx>
# 
# include/net/xfrm.h
#   2005/02/20 06:19:50+01:00 kaber@xxxxxxxxxxxx +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 <kaber@xxxxxxxxx>
# 
# include/net/flow.h
#   2005/02/20 06:19:50+01:00 kaber@xxxxxxxxxxxx +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 <kaber@xxxxxxxxx>
# 
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 {
diff -Nru a/include/net/xfrm.h b/include/net/xfrm.h
--- a/include/net/xfrm.h        2005-02-20 06:20:34 +01:00
+++ b/include/net/xfrm.h        2005-02-20 06:20:34 +01:00
@@ -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);
 }