* [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector
@ 2023-10-04 16:09 Florian Westphal
2023-10-04 16:09 ` [PATCH ipsec-next v3 1/3] xfrm: pass struct net to xfrm_decode_session wrappers Florian Westphal
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Florian Westphal @ 2023-10-04 16:09 UTC (permalink / raw)
To: netdev; +Cc: steffen.klassert, herbert, Florian Westphal
Remove the ipv4+ipv6 session decode functions and use generic flow
dissector to populate the flowi for the policy lookup.
Changes since v2:
- first patch broke CONFIG_XFRM=n builds
Changes since v1:
- Can't use skb_flow_dissect(), we might see skbs that have neither
skb->sk nor skb->dev set. Flow dissector WARN()s in this case, it
tries to check for a bpf program assigned in that net namespace.
Add a preparation patch to pass down 'struct net' in
xfrm_decode_session so its available for use in patch 3.
Changes since RFC:
- Drop mobility header support. I don't think that anyone uses
this. MOBIKE doesn't appear to need this either.
- Drop fl6->flowlabel assignment, original code leaves it as 0.
There is no reason for this change other than to remove code.
Florian Westphal (3):
xfrm: pass struct net to xfrm_decode_session wrappers
xfrm: move mark and oif flowi decode into common code
xfrm: policy: replace session decode with flow dissector
include/net/xfrm.h | 12 +-
net/ipv4/icmp.c | 2 +-
net/ipv4/ip_vti.c | 4 +-
net/ipv4/netfilter.c | 2 +-
net/ipv6/icmp.c | 2 +-
net/ipv6/ip6_vti.c | 4 +-
net/ipv6/netfilter.c | 2 +-
net/netfilter/nf_nat_proto.c | 2 +-
net/xfrm/xfrm_interface_core.c | 4 +-
net/xfrm/xfrm_policy.c | 287 +++++++++++++--------------------
10 files changed, 129 insertions(+), 192 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH ipsec-next v3 1/3] xfrm: pass struct net to xfrm_decode_session wrappers 2023-10-04 16:09 [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector Florian Westphal @ 2023-10-04 16:09 ` Florian Westphal 2023-10-05 12:05 ` Simon Horman 2023-10-04 16:09 ` [PATCH ipsec-next v3 2/3] xfrm: move mark and oif flowi decode into common code Florian Westphal ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Florian Westphal @ 2023-10-04 16:09 UTC (permalink / raw) To: netdev; +Cc: steffen.klassert, herbert, Florian Westphal, kernel test robot Preparation patch, extra arg is not used. No functional changes intended. This is needed to replace the xfrm session decode functions with the flow dissector. skb_flow_dissect() cannot be used as-is, because it attempts to deduce the 'struct net' to use for bpf program fetch from skb->sk or skb->dev, but xfrm code path can see skbs that have neither sk or dev filled in. So either flow dissector needs to try harder, e.g. by also trying skb->dst->dev, or we have to pass the struct net explicitly. Passing the struct net doesn't look too bad to me, most places already have it available or can derive it from the output device. Reported-by: kernel test robot <oliver.sang@intel.com> Link: https://lore.kernel.org/netdev/202309271628.27fd2187-oliver.sang@intel.com/ Signed-off-by: Florian Westphal <fw@strlen.de> --- v3: adjust xfrm_decode_session_reverse() CONFIG_XFRM=n stub too, else build breakage. include/net/xfrm.h | 12 ++++++------ net/ipv4/icmp.c | 2 +- net/ipv4/ip_vti.c | 4 ++-- net/ipv4/netfilter.c | 2 +- net/ipv6/icmp.c | 2 +- net/ipv6/ip6_vti.c | 4 ++-- net/ipv6/netfilter.c | 2 +- net/netfilter/nf_nat_proto.c | 2 +- net/xfrm/xfrm_interface_core.c | 4 ++-- net/xfrm/xfrm_policy.c | 10 +++++----- 10 files changed, 22 insertions(+), 22 deletions(-) diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 35749a672cd1..70de919d9213 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1207,20 +1207,20 @@ static inline int xfrm6_policy_check_reverse(struct sock *sk, int dir, return __xfrm_policy_check2(sk, dir, skb, AF_INET6, 1); } -int __xfrm_decode_session(struct sk_buff *skb, struct flowi *fl, +int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl, unsigned int family, int reverse); -static inline int xfrm_decode_session(struct sk_buff *skb, struct flowi *fl, +static inline int xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl, unsigned int family) { - return __xfrm_decode_session(skb, fl, family, 0); + return __xfrm_decode_session(net, skb, fl, family, 0); } -static inline int xfrm_decode_session_reverse(struct sk_buff *skb, +static inline int xfrm_decode_session_reverse(struct net *net, struct sk_buff *skb, struct flowi *fl, unsigned int family) { - return __xfrm_decode_session(skb, fl, family, 1); + return __xfrm_decode_session(net, skb, fl, family, 1); } int __xfrm_route_forward(struct sk_buff *skb, unsigned short family); @@ -1296,7 +1296,7 @@ static inline int xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *sk { return 1; } -static inline int xfrm_decode_session_reverse(struct sk_buff *skb, +static inline int xfrm_decode_session_reverse(struct net *net, struct sk_buff *skb, struct flowi *fl, unsigned int family) { diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index b8607763d113..e63a3bf99617 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -517,7 +517,7 @@ static struct rtable *icmp_route_lookup(struct net *net, } else return rt; - err = xfrm_decode_session_reverse(skb_in, flowi4_to_flowi(&fl4_dec), AF_INET); + err = xfrm_decode_session_reverse(net, skb_in, flowi4_to_flowi(&fl4_dec), AF_INET); if (err) goto relookup_failed; diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c index d1e7d0ceb7ed..9ab9b3ebe0cd 100644 --- a/net/ipv4/ip_vti.c +++ b/net/ipv4/ip_vti.c @@ -288,11 +288,11 @@ static netdev_tx_t vti_tunnel_xmit(struct sk_buff *skb, struct net_device *dev) switch (skb->protocol) { case htons(ETH_P_IP): memset(IPCB(skb), 0, sizeof(*IPCB(skb))); - xfrm_decode_session(skb, &fl, AF_INET); + xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET); break; case htons(ETH_P_IPV6): memset(IP6CB(skb), 0, sizeof(*IP6CB(skb))); - xfrm_decode_session(skb, &fl, AF_INET6); + xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET6); break; default: goto tx_err; diff --git a/net/ipv4/netfilter.c b/net/ipv4/netfilter.c index bd135165482a..591a2737808e 100644 --- a/net/ipv4/netfilter.c +++ b/net/ipv4/netfilter.c @@ -62,7 +62,7 @@ int ip_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb, un #ifdef CONFIG_XFRM if (!(IPCB(skb)->flags & IPSKB_XFRM_TRANSFORMED) && - xfrm_decode_session(skb, flowi4_to_flowi(&fl4), AF_INET) == 0) { + xfrm_decode_session(net, skb, flowi4_to_flowi(&fl4), AF_INET) == 0) { struct dst_entry *dst = skb_dst(skb); skb_dst_set(skb, NULL); dst = xfrm_lookup(net, dst, flowi4_to_flowi(&fl4), sk, 0); diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c index 8fb4a791881a..f62427097126 100644 --- a/net/ipv6/icmp.c +++ b/net/ipv6/icmp.c @@ -385,7 +385,7 @@ static struct dst_entry *icmpv6_route_lookup(struct net *net, return dst; } - err = xfrm_decode_session_reverse(skb, flowi6_to_flowi(&fl2), AF_INET6); + err = xfrm_decode_session_reverse(net, skb, flowi6_to_flowi(&fl2), AF_INET6); if (err) goto relookup_failed; diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c index 73c85d4e0e9c..e550240c85e1 100644 --- a/net/ipv6/ip6_vti.c +++ b/net/ipv6/ip6_vti.c @@ -569,11 +569,11 @@ vti6_tnl_xmit(struct sk_buff *skb, struct net_device *dev) goto tx_err; memset(IP6CB(skb), 0, sizeof(*IP6CB(skb))); - xfrm_decode_session(skb, &fl, AF_INET6); + xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET6); break; case htons(ETH_P_IP): memset(IPCB(skb), 0, sizeof(*IPCB(skb))); - xfrm_decode_session(skb, &fl, AF_INET); + xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET); break; default: goto tx_err; diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c index 857713d7a38a..53d255838e6a 100644 --- a/net/ipv6/netfilter.c +++ b/net/ipv6/netfilter.c @@ -61,7 +61,7 @@ int ip6_route_me_harder(struct net *net, struct sock *sk_partial, struct sk_buff #ifdef CONFIG_XFRM if (!(IP6CB(skb)->flags & IP6SKB_XFRM_TRANSFORMED) && - xfrm_decode_session(skb, flowi6_to_flowi(&fl6), AF_INET6) == 0) { + xfrm_decode_session(net, skb, flowi6_to_flowi(&fl6), AF_INET6) == 0) { skb_dst_set(skb, NULL); dst = xfrm_lookup(net, dst, flowi6_to_flowi(&fl6), sk, 0); if (IS_ERR(dst)) diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c index 48cc60084d28..c77963517bf8 100644 --- a/net/netfilter/nf_nat_proto.c +++ b/net/netfilter/nf_nat_proto.c @@ -668,7 +668,7 @@ static int nf_xfrm_me_harder(struct net *net, struct sk_buff *skb, unsigned int struct flowi fl; int err; - err = xfrm_decode_session(skb, &fl, family); + err = xfrm_decode_session(net, skb, &fl, family); if (err < 0) return err; diff --git a/net/xfrm/xfrm_interface_core.c b/net/xfrm/xfrm_interface_core.c index b86474084690..656f437f5f53 100644 --- a/net/xfrm/xfrm_interface_core.c +++ b/net/xfrm/xfrm_interface_core.c @@ -538,7 +538,7 @@ static netdev_tx_t xfrmi_xmit(struct sk_buff *skb, struct net_device *dev) switch (skb->protocol) { case htons(ETH_P_IPV6): memset(IP6CB(skb), 0, sizeof(*IP6CB(skb))); - xfrm_decode_session(skb, &fl, AF_INET6); + xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET6); if (!dst) { fl.u.ip6.flowi6_oif = dev->ifindex; fl.u.ip6.flowi6_flags |= FLOWI_FLAG_ANYSRC; @@ -553,7 +553,7 @@ static netdev_tx_t xfrmi_xmit(struct sk_buff *skb, struct net_device *dev) break; case htons(ETH_P_IP): memset(IPCB(skb), 0, sizeof(*IPCB(skb))); - xfrm_decode_session(skb, &fl, AF_INET); + xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET); if (!dst) { struct rtable *rt; diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index c4c4fc29ccf5..064d1744fa36 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -2853,7 +2853,7 @@ static void xfrm_policy_queue_process(struct timer_list *t) /* Fixup the mark to support VTI. */ skb_mark = skb->mark; skb->mark = pol->mark.v; - xfrm_decode_session(skb, &fl, dst->ops->family); + xfrm_decode_session(net, skb, &fl, dst->ops->family); skb->mark = skb_mark; spin_unlock(&pq->hold_queue.lock); @@ -2889,7 +2889,7 @@ static void xfrm_policy_queue_process(struct timer_list *t) /* Fixup the mark to support VTI. */ skb_mark = skb->mark; skb->mark = pol->mark.v; - xfrm_decode_session(skb, &fl, skb_dst(skb)->ops->family); + xfrm_decode_session(net, skb, &fl, skb_dst(skb)->ops->family); skb->mark = skb_mark; dst_hold(xfrm_dst_path(skb_dst(skb))); @@ -3554,7 +3554,7 @@ decode_session6(struct sk_buff *skb, struct flowi *fl, bool reverse) } #endif -int __xfrm_decode_session(struct sk_buff *skb, struct flowi *fl, +int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl, unsigned int family, int reverse) { switch (family) { @@ -3618,7 +3618,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, reverse = dir & ~XFRM_POLICY_MASK; dir &= XFRM_POLICY_MASK; - if (__xfrm_decode_session(skb, &fl, family, reverse) < 0) { + if (__xfrm_decode_session(net, skb, &fl, family, reverse) < 0) { XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR); return 0; } @@ -3774,7 +3774,7 @@ int __xfrm_route_forward(struct sk_buff *skb, unsigned short family) struct dst_entry *dst; int res = 1; - if (xfrm_decode_session(skb, &fl, family) < 0) { + if (xfrm_decode_session(net, skb, &fl, family) < 0) { XFRM_INC_STATS(net, LINUX_MIB_XFRMFWDHDRERROR); return 0; } -- 2.41.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH ipsec-next v3 1/3] xfrm: pass struct net to xfrm_decode_session wrappers 2023-10-04 16:09 ` [PATCH ipsec-next v3 1/3] xfrm: pass struct net to xfrm_decode_session wrappers Florian Westphal @ 2023-10-05 12:05 ` Simon Horman 0 siblings, 0 replies; 13+ messages in thread From: Simon Horman @ 2023-10-05 12:05 UTC (permalink / raw) To: Florian Westphal; +Cc: netdev, steffen.klassert, herbert, kernel test robot On Wed, Oct 04, 2023 at 06:09:51PM +0200, Florian Westphal wrote: > Preparation patch, extra arg is not used. > No functional changes intended. > > This is needed to replace the xfrm session decode functions with > the flow dissector. > > skb_flow_dissect() cannot be used as-is, because it attempts to deduce the > 'struct net' to use for bpf program fetch from skb->sk or skb->dev, but > xfrm code path can see skbs that have neither sk or dev filled in. > > So either flow dissector needs to try harder, e.g. by also trying > skb->dst->dev, or we have to pass the struct net explicitly. > > Passing the struct net doesn't look too bad to me, most places > already have it available or can derive it from the output device. > > Reported-by: kernel test robot <oliver.sang@intel.com> > Link: https://lore.kernel.org/netdev/202309271628.27fd2187-oliver.sang@intel.com/ > Signed-off-by: Florian Westphal <fw@strlen.de> Reviewed-by: Simon Horman <horms@kernel.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH ipsec-next v3 2/3] xfrm: move mark and oif flowi decode into common code 2023-10-04 16:09 [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector Florian Westphal 2023-10-04 16:09 ` [PATCH ipsec-next v3 1/3] xfrm: pass struct net to xfrm_decode_session wrappers Florian Westphal @ 2023-10-04 16:09 ` Florian Westphal 2023-10-04 16:09 ` [PATCH ipsec-next v3 3/3] xfrm: policy: replace session decode with flow dissector Florian Westphal 2023-10-10 7:51 ` [PATCH ipsec-next v3 0/3] " Steffen Klassert 3 siblings, 0 replies; 13+ messages in thread From: Florian Westphal @ 2023-10-04 16:09 UTC (permalink / raw) To: netdev; +Cc: steffen.klassert, herbert, Florian Westphal, Simon Horman flowi4_oif/flowi6_oif and mark are aliased to flowi_common field, i.e. all can be used interchangeably. Instead of duplicating place this in common code. No functional changes intended. Reviewed-by: Simon Horman <horms@kernel.org> Signed-off-by: Florian Westphal <fw@strlen.de> --- v3: no changes. net/xfrm/xfrm_policy.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 064d1744fa36..da29be0b002f 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -3373,14 +3373,8 @@ decode_session4(struct sk_buff *skb, struct flowi *fl, bool reverse) int ihl = iph->ihl; u8 *xprth = skb_network_header(skb) + ihl * 4; struct flowi4 *fl4 = &fl->u.ip4; - int oif = 0; - - if (skb_dst(skb) && skb_dst(skb)->dev) - oif = skb_dst(skb)->dev->ifindex; memset(fl4, 0, sizeof(struct flowi4)); - fl4->flowi4_mark = skb->mark; - fl4->flowi4_oif = reverse ? skb->skb_iif : oif; fl4->flowi4_proto = iph->protocol; fl4->daddr = reverse ? iph->saddr : iph->daddr; @@ -3451,7 +3445,6 @@ decode_session6(struct sk_buff *skb, struct flowi *fl, bool reverse) struct ipv6_opt_hdr *exthdr; const unsigned char *nh = skb_network_header(skb); u16 nhoff = IP6CB(skb)->nhoff; - int oif = 0; u8 nexthdr; if (!nhoff) @@ -3459,12 +3452,7 @@ decode_session6(struct sk_buff *skb, struct flowi *fl, bool reverse) nexthdr = nh[nhoff]; - if (skb_dst(skb) && skb_dst(skb)->dev) - oif = skb_dst(skb)->dev->ifindex; - memset(fl6, 0, sizeof(struct flowi6)); - fl6->flowi6_mark = skb->mark; - fl6->flowi6_oif = reverse ? skb->skb_iif : oif; fl6->daddr = reverse ? hdr->saddr : hdr->daddr; fl6->saddr = reverse ? hdr->daddr : hdr->saddr; @@ -3570,6 +3558,18 @@ int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl return -EAFNOSUPPORT; } + fl->flowi_mark = skb->mark; + if (reverse) { + fl->flowi_oif = skb->skb_iif; + } else { + int oif = 0; + + if (skb_dst(skb) && skb_dst(skb)->dev) + oif = skb_dst(skb)->dev->ifindex; + + fl->flowi_oif = oif; + } + return security_xfrm_decode_session(skb, &fl->flowi_secid); } EXPORT_SYMBOL(__xfrm_decode_session); -- 2.41.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH ipsec-next v3 3/3] xfrm: policy: replace session decode with flow dissector 2023-10-04 16:09 [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector Florian Westphal 2023-10-04 16:09 ` [PATCH ipsec-next v3 1/3] xfrm: pass struct net to xfrm_decode_session wrappers Florian Westphal 2023-10-04 16:09 ` [PATCH ipsec-next v3 2/3] xfrm: move mark and oif flowi decode into common code Florian Westphal @ 2023-10-04 16:09 ` Florian Westphal 2023-10-10 7:51 ` [PATCH ipsec-next v3 0/3] " Steffen Klassert 3 siblings, 0 replies; 13+ messages in thread From: Florian Westphal @ 2023-10-04 16:09 UTC (permalink / raw) To: netdev; +Cc: steffen.klassert, herbert, Florian Westphal, Simon Horman xfrm needs to populate ipv4/v6 flow struct for route lookup. In the past there were several bugs in this code: 1. callers that forget to reload header pointers after xfrm_decode_session() (it may pull headers). 2. bugs in decoding where accesses past skb->data occurred. Meanwhile network core gained a packet dissector as well. This switches xfrm to the flow dissector. Changes since RFC: Drop ipv6 mobiliy header support, AFAIU noone uses this. Drop extraction of flowlabel, replaced code doesn't set it either. Reviewed-by: Simon Horman <horms@kernel.org> Link: https://lore.kernel.org/netdev/20230908120628.26164-3-fw@strlen.de/ Signed-off-by: Florian Westphal <fw@strlen.de> --- v3: no changes. net/xfrm/xfrm_policy.c | 253 ++++++++++++++++------------------------- 1 file changed, 95 insertions(+), 158 deletions(-) diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index da29be0b002f..6aea8b2f45e0 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -149,6 +149,21 @@ struct xfrm_pol_inexact_candidates { struct hlist_head *res[XFRM_POL_CAND_MAX]; }; +struct xfrm_flow_keys { + struct flow_dissector_key_basic basic; + struct flow_dissector_key_control control; + union { + struct flow_dissector_key_ipv4_addrs ipv4; + struct flow_dissector_key_ipv6_addrs ipv6; + } addrs; + struct flow_dissector_key_ip ip; + struct flow_dissector_key_icmp icmp; + struct flow_dissector_key_ports ports; + struct flow_dissector_key_keyid gre; +}; + +static struct flow_dissector xfrm_session_dissector __ro_after_init; + static DEFINE_SPINLOCK(xfrm_if_cb_lock); static struct xfrm_if_cb const __rcu *xfrm_if_cb __read_mostly; @@ -3367,191 +3382,74 @@ xfrm_policy_ok(const struct xfrm_tmpl *tmpl, const struct sec_path *sp, int star } static void -decode_session4(struct sk_buff *skb, struct flowi *fl, bool reverse) +decode_session4(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reverse) { - const struct iphdr *iph = ip_hdr(skb); - int ihl = iph->ihl; - u8 *xprth = skb_network_header(skb) + ihl * 4; struct flowi4 *fl4 = &fl->u.ip4; memset(fl4, 0, sizeof(struct flowi4)); - fl4->flowi4_proto = iph->protocol; - fl4->daddr = reverse ? iph->saddr : iph->daddr; - fl4->saddr = reverse ? iph->daddr : iph->saddr; - fl4->flowi4_tos = iph->tos & ~INET_ECN_MASK; - - if (!ip_is_fragment(iph)) { - switch (iph->protocol) { - case IPPROTO_UDP: - case IPPROTO_UDPLITE: - case IPPROTO_TCP: - case IPPROTO_SCTP: - case IPPROTO_DCCP: - if (xprth + 4 < skb->data || - pskb_may_pull(skb, xprth + 4 - skb->data)) { - __be16 *ports; - - xprth = skb_network_header(skb) + ihl * 4; - ports = (__be16 *)xprth; - - fl4->fl4_sport = ports[!!reverse]; - fl4->fl4_dport = ports[!reverse]; - } - break; - case IPPROTO_ICMP: - if (xprth + 2 < skb->data || - pskb_may_pull(skb, xprth + 2 - skb->data)) { - u8 *icmp; - - xprth = skb_network_header(skb) + ihl * 4; - icmp = xprth; - - fl4->fl4_icmp_type = icmp[0]; - fl4->fl4_icmp_code = icmp[1]; - } - break; - case IPPROTO_GRE: - if (xprth + 12 < skb->data || - pskb_may_pull(skb, xprth + 12 - skb->data)) { - __be16 *greflags; - __be32 *gre_hdr; - - xprth = skb_network_header(skb) + ihl * 4; - greflags = (__be16 *)xprth; - gre_hdr = (__be32 *)xprth; - - if (greflags[0] & GRE_KEY) { - if (greflags[0] & GRE_CSUM) - gre_hdr++; - fl4->fl4_gre_key = gre_hdr[1]; - } - } - break; - default: - break; - } + if (reverse) { + fl4->saddr = flkeys->addrs.ipv4.dst; + fl4->daddr = flkeys->addrs.ipv4.src; + fl4->fl4_sport = flkeys->ports.dst; + fl4->fl4_dport = flkeys->ports.src; + } else { + fl4->saddr = flkeys->addrs.ipv4.src; + fl4->daddr = flkeys->addrs.ipv4.dst; + fl4->fl4_sport = flkeys->ports.src; + fl4->fl4_dport = flkeys->ports.dst; } + + fl4->flowi4_proto = flkeys->basic.ip_proto; + fl4->flowi4_tos = flkeys->ip.tos; + fl4->fl4_icmp_type = flkeys->icmp.type; + fl4->fl4_icmp_type = flkeys->icmp.code; + fl4->fl4_gre_key = flkeys->gre.keyid; } #if IS_ENABLED(CONFIG_IPV6) static void -decode_session6(struct sk_buff *skb, struct flowi *fl, bool reverse) +decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reverse) { struct flowi6 *fl6 = &fl->u.ip6; - int onlyproto = 0; - const struct ipv6hdr *hdr = ipv6_hdr(skb); - u32 offset = sizeof(*hdr); - struct ipv6_opt_hdr *exthdr; - const unsigned char *nh = skb_network_header(skb); - u16 nhoff = IP6CB(skb)->nhoff; - u8 nexthdr; - - if (!nhoff) - nhoff = offsetof(struct ipv6hdr, nexthdr); - - nexthdr = nh[nhoff]; memset(fl6, 0, sizeof(struct flowi6)); - fl6->daddr = reverse ? hdr->saddr : hdr->daddr; - fl6->saddr = reverse ? hdr->daddr : hdr->saddr; - - while (nh + offset + sizeof(*exthdr) < skb->data || - pskb_may_pull(skb, nh + offset + sizeof(*exthdr) - skb->data)) { - nh = skb_network_header(skb); - exthdr = (struct ipv6_opt_hdr *)(nh + offset); - - switch (nexthdr) { - case NEXTHDR_FRAGMENT: - onlyproto = 1; - fallthrough; - case NEXTHDR_ROUTING: - case NEXTHDR_HOP: - case NEXTHDR_DEST: - offset += ipv6_optlen(exthdr); - nexthdr = exthdr->nexthdr; - break; - case IPPROTO_UDP: - case IPPROTO_UDPLITE: - case IPPROTO_TCP: - case IPPROTO_SCTP: - case IPPROTO_DCCP: - if (!onlyproto && (nh + offset + 4 < skb->data || - pskb_may_pull(skb, nh + offset + 4 - skb->data))) { - __be16 *ports; - - nh = skb_network_header(skb); - ports = (__be16 *)(nh + offset); - fl6->fl6_sport = ports[!!reverse]; - fl6->fl6_dport = ports[!reverse]; - } - fl6->flowi6_proto = nexthdr; - return; - case IPPROTO_ICMPV6: - if (!onlyproto && (nh + offset + 2 < skb->data || - pskb_may_pull(skb, nh + offset + 2 - skb->data))) { - u8 *icmp; - - nh = skb_network_header(skb); - icmp = (u8 *)(nh + offset); - fl6->fl6_icmp_type = icmp[0]; - fl6->fl6_icmp_code = icmp[1]; - } - fl6->flowi6_proto = nexthdr; - return; - case IPPROTO_GRE: - if (!onlyproto && - (nh + offset + 12 < skb->data || - pskb_may_pull(skb, nh + offset + 12 - skb->data))) { - struct gre_base_hdr *gre_hdr; - __be32 *gre_key; - - nh = skb_network_header(skb); - gre_hdr = (struct gre_base_hdr *)(nh + offset); - gre_key = (__be32 *)(gre_hdr + 1); - - if (gre_hdr->flags & GRE_KEY) { - if (gre_hdr->flags & GRE_CSUM) - gre_key++; - fl6->fl6_gre_key = *gre_key; - } - } - fl6->flowi6_proto = nexthdr; - return; - -#if IS_ENABLED(CONFIG_IPV6_MIP6) - case IPPROTO_MH: - offset += ipv6_optlen(exthdr); - if (!onlyproto && (nh + offset + 3 < skb->data || - pskb_may_pull(skb, nh + offset + 3 - skb->data))) { - struct ip6_mh *mh; - - nh = skb_network_header(skb); - mh = (struct ip6_mh *)(nh + offset); - fl6->fl6_mh_type = mh->ip6mh_type; - } - fl6->flowi6_proto = nexthdr; - return; -#endif - default: - fl6->flowi6_proto = nexthdr; - return; - } + if (reverse) { + fl6->saddr = flkeys->addrs.ipv6.dst; + fl6->daddr = flkeys->addrs.ipv6.src; + fl6->fl6_sport = flkeys->ports.dst; + fl6->fl6_dport = flkeys->ports.src; + } else { + fl6->saddr = flkeys->addrs.ipv6.src; + fl6->daddr = flkeys->addrs.ipv6.dst; + fl6->fl6_sport = flkeys->ports.src; + fl6->fl6_dport = flkeys->ports.dst; } + + fl6->flowi6_proto = flkeys->basic.ip_proto; + fl6->fl6_icmp_type = flkeys->icmp.type; + fl6->fl6_icmp_type = flkeys->icmp.code; + fl6->fl6_gre_key = flkeys->gre.keyid; } #endif int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl, unsigned int family, int reverse) { + struct xfrm_flow_keys flkeys; + + memset(&flkeys, 0, sizeof(flkeys)); + __skb_flow_dissect(net, skb, &xfrm_session_dissector, &flkeys, + NULL, 0, 0, 0, FLOW_DISSECTOR_F_STOP_AT_ENCAP); + switch (family) { case AF_INET: - decode_session4(skb, fl, reverse); + decode_session4(&flkeys, fl, reverse); break; #if IS_ENABLED(CONFIG_IPV6) case AF_INET6: - decode_session6(skb, fl, reverse); + decode_session6(&flkeys, fl, reverse); break; #endif default: @@ -4253,8 +4151,47 @@ static struct pernet_operations __net_initdata xfrm_net_ops = { .exit = xfrm_net_exit, }; +static const struct flow_dissector_key xfrm_flow_dissector_keys[] = { + { + .key_id = FLOW_DISSECTOR_KEY_CONTROL, + .offset = offsetof(struct xfrm_flow_keys, control), + }, + { + .key_id = FLOW_DISSECTOR_KEY_BASIC, + .offset = offsetof(struct xfrm_flow_keys, basic), + }, + { + .key_id = FLOW_DISSECTOR_KEY_IPV4_ADDRS, + .offset = offsetof(struct xfrm_flow_keys, addrs.ipv4), + }, + { + .key_id = FLOW_DISSECTOR_KEY_IPV6_ADDRS, + .offset = offsetof(struct xfrm_flow_keys, addrs.ipv6), + }, + { + .key_id = FLOW_DISSECTOR_KEY_PORTS, + .offset = offsetof(struct xfrm_flow_keys, ports), + }, + { + .key_id = FLOW_DISSECTOR_KEY_GRE_KEYID, + .offset = offsetof(struct xfrm_flow_keys, gre), + }, + { + .key_id = FLOW_DISSECTOR_KEY_IP, + .offset = offsetof(struct xfrm_flow_keys, ip), + }, + { + .key_id = FLOW_DISSECTOR_KEY_ICMP, + .offset = offsetof(struct xfrm_flow_keys, icmp), + }, +}; + void __init xfrm_init(void) { + skb_flow_dissector_init(&xfrm_session_dissector, + xfrm_flow_dissector_keys, + ARRAY_SIZE(xfrm_flow_dissector_keys)); + register_pernet_subsys(&xfrm_net_ops); xfrm_dev_init(); xfrm_input_init(); -- 2.41.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector 2023-10-04 16:09 [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector Florian Westphal ` (2 preceding siblings ...) 2023-10-04 16:09 ` [PATCH ipsec-next v3 3/3] xfrm: policy: replace session decode with flow dissector Florian Westphal @ 2023-10-10 7:51 ` Steffen Klassert 2023-10-26 12:12 ` Antony Antony 3 siblings, 1 reply; 13+ messages in thread From: Steffen Klassert @ 2023-10-10 7:51 UTC (permalink / raw) To: Florian Westphal; +Cc: netdev, herbert On Wed, Oct 04, 2023 at 06:09:50PM +0200, Florian Westphal wrote: > Remove the ipv4+ipv6 session decode functions and use generic flow > dissector to populate the flowi for the policy lookup. > > Changes since v2: > - first patch broke CONFIG_XFRM=n builds > > Changes since v1: > - Can't use skb_flow_dissect(), we might see skbs that have neither > skb->sk nor skb->dev set. Flow dissector WARN()s in this case, it > tries to check for a bpf program assigned in that net namespace. > > Add a preparation patch to pass down 'struct net' in > xfrm_decode_session so its available for use in patch 3. > > Changes since RFC: > > - Drop mobility header support. I don't think that anyone uses > this. MOBIKE doesn't appear to need this either. > - Drop fl6->flowlabel assignment, original code leaves it as 0. > > There is no reason for this change other than to remove code. > > Florian Westphal (3): > xfrm: pass struct net to xfrm_decode_session wrappers > xfrm: move mark and oif flowi decode into common code > xfrm: policy: replace session decode with flow dissector Series applied, thanks a lot Florian! ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector 2023-10-10 7:51 ` [PATCH ipsec-next v3 0/3] " Steffen Klassert @ 2023-10-26 12:12 ` Antony Antony 2023-10-26 12:57 ` Florian Westphal 0 siblings, 1 reply; 13+ messages in thread From: Antony Antony @ 2023-10-26 12:12 UTC (permalink / raw) To: Steffen Klassert; +Cc: Florian Westphal, netdev, herbert On Tue, Oct 10, 2023 at 09:51:16AM +0200, Steffen Klassert wrote: > On Wed, Oct 04, 2023 at 06:09:50PM +0200, Florian Westphal wrote: > > Remove the ipv4+ipv6 session decode functions and use generic flow > > dissector to populate the flowi for the policy lookup. > > > > Changes since v2: > > - first patch broke CONFIG_XFRM=n builds > > > > Changes since v1: > > - Can't use skb_flow_dissect(), we might see skbs that have neither > > skb->sk nor skb->dev set. Flow dissector WARN()s in this case, it > > tries to check for a bpf program assigned in that net namespace. > > > > Add a preparation patch to pass down 'struct net' in > > xfrm_decode_session so its available for use in patch 3. > > > > Changes since RFC: > > > > - Drop mobility header support. I don't think that anyone uses > > this. MOBIKE doesn't appear to need this either. > > - Drop fl6->flowlabel assignment, original code leaves it as 0. > > > > There is no reason for this change other than to remove code. > > > > Florian Westphal (3): > > xfrm: pass struct net to xfrm_decode_session wrappers > > xfrm: move mark and oif flowi decode into common code > > xfrm: policy: replace session decode with flow dissector > > Series applied, thanks a lot Florian! > Hi Steffen, I would like to report a potential bug that I've encountered while working on a related patch involving xfrm and ICMP packet decoding using wrapped xfrm_decode_session. This issue came to my attention while testing my my patch "xfrm: introduce forwarding of ICMP Error messages" Here is the output from gdb after xfrm_decode_session. before this series applied p fl.u.ip4.uli $3 = {ports = {dport = 11, sport = 0}, icmpt = {type = 11 '\v', code = 0 '\000'}, gre_key = 11, mht = { type = 11 '\v'}} after this series applied. p fl.u.ip4.uli $11 = {ports = {dport = 0, sport = 0}, icmpt = {type = 0 '\000', code = 0 '\000'}, gre_key = 0, mht = { type = 0 '\000'}} I believe this discrepancy may indicate an issue with the decoding of ICMP packets following the application above patches. While I could further debug the issue and create a generic test case to replicate it without my patch, I wanted to bring it to your attention before the ipsec-next branch gets merged into net-next. regards, -antony ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector 2023-10-26 12:12 ` Antony Antony @ 2023-10-26 12:57 ` Florian Westphal 2023-10-26 14:33 ` Antony Antony 0 siblings, 1 reply; 13+ messages in thread From: Florian Westphal @ 2023-10-26 12:57 UTC (permalink / raw) To: Antony Antony; +Cc: Steffen Klassert, Florian Westphal, netdev, herbert Antony Antony <antony@phenome.org> wrote: > > > Florian Westphal (3): > > > xfrm: pass struct net to xfrm_decode_session wrappers > > > xfrm: move mark and oif flowi decode into common code > > > xfrm: policy: replace session decode with flow dissector > > > > Series applied, thanks a lot Florian! > > > > Hi Steffen, > > I would like to report a potential bug that I've encountered while working s/potential// Does this patch make things work for you again? Thanks! diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 6aea8b2f45e0..e8c406eba11b 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -3400,11 +3400,18 @@ decode_session4(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve fl4->fl4_dport = flkeys->ports.dst; } + switch (flkeys->basic.ip_proto) { + case IPPROTO_GRE: + fl4->fl4_gre_key = flkeys->gre.keyid; + break; + case IPPROTO_ICMP: + fl4->fl4_icmp_type = flkeys->icmp.type; + fl4->fl4_icmp_code = flkeys->icmp.code; + break; + } + fl4->flowi4_proto = flkeys->basic.ip_proto; fl4->flowi4_tos = flkeys->ip.tos; - fl4->fl4_icmp_type = flkeys->icmp.type; - fl4->fl4_icmp_type = flkeys->icmp.code; - fl4->fl4_gre_key = flkeys->gre.keyid; } #if IS_ENABLED(CONFIG_IPV6) @@ -3427,10 +3434,17 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve fl6->fl6_dport = flkeys->ports.dst; } + switch (flkeys->basic.ip_proto) { + case IPPROTO_GRE: + fl6->fl6_gre_key = flkeys->gre.keyid; + break; + case IPPROTO_ICMP: + fl6->fl6_icmp_type = flkeys->icmp.type; + fl6->fl6_icmp_code = flkeys->icmp.code; + break; + } + fl6->flowi6_proto = flkeys->basic.ip_proto; - fl6->fl6_icmp_type = flkeys->icmp.type; - fl6->fl6_icmp_type = flkeys->icmp.code; - fl6->fl6_gre_key = flkeys->gre.keyid; } #endif ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector 2023-10-26 12:57 ` Florian Westphal @ 2023-10-26 14:33 ` Antony Antony 2023-10-26 14:36 ` [PATCH ipsec-next] xfrm: policy: fix layer 4 flowi decoding Florian Westphal 2023-10-26 14:45 ` [PATCH ipsec-next v2] " Florian Westphal 0 siblings, 2 replies; 13+ messages in thread From: Antony Antony @ 2023-10-26 14:33 UTC (permalink / raw) To: Florian Westphal; +Cc: Antony Antony, Steffen Klassert, netdev, herbert On Thu, Oct 26, 2023 at 14:57:48 +0200, Florian Westphal wrote: > Antony Antony <antony@phenome.org> wrote: > > > > Florian Westphal (3): > > > > xfrm: pass struct net to xfrm_decode_session wrappers > > > > xfrm: move mark and oif flowi decode into common code > > > > xfrm: policy: replace session decode with flow dissector > > > > > > Series applied, thanks a lot Florian! > > > > > > > Hi Steffen, > > > > I would like to report a potential bug that I've encountered while working > > s/potential// > > Does this patch make things work for you again? Thanks! > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 6aea8b2f45e0..e8c406eba11b 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -3400,11 +3400,18 @@ decode_session4(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve > fl4->fl4_dport = flkeys->ports.dst; > } > > + switch (flkeys->basic.ip_proto) { > + case IPPROTO_GRE: > + fl4->fl4_gre_key = flkeys->gre.keyid; > + break; > + case IPPROTO_ICMP: > + fl4->fl4_icmp_type = flkeys->icmp.type; > + fl4->fl4_icmp_code = flkeys->icmp.code; > + break; > + } > + > fl4->flowi4_proto = flkeys->basic.ip_proto; > fl4->flowi4_tos = flkeys->ip.tos; > - fl4->fl4_icmp_type = flkeys->icmp.type; > - fl4->fl4_icmp_type = flkeys->icmp.code; > - fl4->fl4_gre_key = flkeys->gre.keyid; > } > > #if IS_ENABLED(CONFIG_IPV6) > @@ -3427,10 +3434,17 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve > fl6->fl6_dport = flkeys->ports.dst; > } > > + switch (flkeys->basic.ip_proto) { > + case IPPROTO_GRE: > + fl6->fl6_gre_key = flkeys->gre.keyid; > + break; > + case IPPROTO_ICMP: > + fl6->fl6_icmp_type = flkeys->icmp.type; > + fl6->fl6_icmp_code = flkeys->icmp.code; > + break; > + } > + > fl6->flowi6_proto = flkeys->basic.ip_proto; > - fl6->fl6_icmp_type = flkeys->icmp.type; > - fl6->fl6_icmp_type = flkeys->icmp.code; > - fl6->fl6_gre_key = flkeys->gre.keyid; > } > #endif > Tested-by: Antony Antony <antony.antony@secunet.com> Thanks, -antony ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH ipsec-next] xfrm: policy: fix layer 4 flowi decoding 2023-10-26 14:33 ` Antony Antony @ 2023-10-26 14:36 ` Florian Westphal 2023-10-26 14:38 ` Florian Westphal 2023-10-26 14:45 ` [PATCH ipsec-next v2] " Florian Westphal 1 sibling, 1 reply; 13+ messages in thread From: Florian Westphal @ 2023-10-26 14:36 UTC (permalink / raw) To: netdev; +Cc: steffen.klassert, herbert, Florian Westphal, Antony Antony The commit shiped with two bugs: fl4->fl4_icmp_type = flkeys->icmp.type; fl4->fl4_icmp_type = flkeys->icmp.code; ~~~~ should have been "code". But the more severe bug is that I got fooled by flowi member defines: fl4_icmp_type, fl4_gre_key and fl4_dport share the same union/address. Fix typo and make gre/icmp keys l4 protocol dependent. Fixes: 7a0207094f1b ("xfrm: policy: replace session decode with flow dissector") Reported-and-tested-by: Antony Antony <antony@phenome.org> Signed-off-by: Florian Westphal <fw@strlen.de> --- net/xfrm/xfrm_policy.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 6aea8b2f45e0..e8c406eba11b 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -3400,11 +3400,18 @@ decode_session4(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve fl4->fl4_dport = flkeys->ports.dst; } + switch (flkeys->basic.ip_proto) { + case IPPROTO_GRE: + fl4->fl4_gre_key = flkeys->gre.keyid; + break; + case IPPROTO_ICMP: + fl4->fl4_icmp_type = flkeys->icmp.type; + fl4->fl4_icmp_code = flkeys->icmp.code; + break; + } + fl4->flowi4_proto = flkeys->basic.ip_proto; fl4->flowi4_tos = flkeys->ip.tos; - fl4->fl4_icmp_type = flkeys->icmp.type; - fl4->fl4_icmp_type = flkeys->icmp.code; - fl4->fl4_gre_key = flkeys->gre.keyid; } #if IS_ENABLED(CONFIG_IPV6) @@ -3427,10 +3434,17 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve fl6->fl6_dport = flkeys->ports.dst; } + switch (flkeys->basic.ip_proto) { + case IPPROTO_GRE: + fl6->fl6_gre_key = flkeys->gre.keyid; + break; + case IPPROTO_ICMP: + fl6->fl6_icmp_type = flkeys->icmp.type; + fl6->fl6_icmp_code = flkeys->icmp.code; + break; + } + fl6->flowi6_proto = flkeys->basic.ip_proto; - fl6->fl6_icmp_type = flkeys->icmp.type; - fl6->fl6_icmp_type = flkeys->icmp.code; - fl6->fl6_gre_key = flkeys->gre.keyid; } #endif -- 2.41.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH ipsec-next] xfrm: policy: fix layer 4 flowi decoding 2023-10-26 14:36 ` [PATCH ipsec-next] xfrm: policy: fix layer 4 flowi decoding Florian Westphal @ 2023-10-26 14:38 ` Florian Westphal 0 siblings, 0 replies; 13+ messages in thread From: Florian Westphal @ 2023-10-26 14:38 UTC (permalink / raw) To: Florian Westphal; +Cc: netdev, steffen.klassert, herbert, Antony Antony Florian Westphal <fw@strlen.de> wrote: > + switch (flkeys->basic.ip_proto) { > + case IPPROTO_GRE: > + fl6->fl6_gre_key = flkeys->gre.keyid; > + break; > + case IPPROTO_ICMP: Sigh. This should be IPPROTO_ICMPV6. I'll send a fresh v2 in a minute. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH ipsec-next v2] xfrm: policy: fix layer 4 flowi decoding 2023-10-26 14:33 ` Antony Antony 2023-10-26 14:36 ` [PATCH ipsec-next] xfrm: policy: fix layer 4 flowi decoding Florian Westphal @ 2023-10-26 14:45 ` Florian Westphal 2023-10-28 8:31 ` Steffen Klassert 1 sibling, 1 reply; 13+ messages in thread From: Florian Westphal @ 2023-10-26 14:45 UTC (permalink / raw) To: netdev; +Cc: steffen.klassert, herbert, Florian Westphal, Antony Antony The commit shipped with two bugs: fl4->fl4_icmp_type = flkeys->icmp.type; fl4->fl4_icmp_type = flkeys->icmp.code; ~~~~ should have been "code". But the more severe bug is that I got fooled by flowi member defines: fl4_icmp_type, fl4_gre_key and fl4_dport share the same union/address. Fix typo and make gre/icmp key setting depend on the l4 protocol. Fixes: 7a0207094f1b ("xfrm: policy: replace session decode with flow dissector") Reported-and-tested-by: Antony Antony <antony@phenome.org> Signed-off-by: Florian Westphal <fw@strlen.de> --- v2: decode_session6 must use IPPROTO_ICMPV6, not IPPROTO_ICMP. I normally do not resend immediately but in this case it seems like the lesser evil. Alternative is to defer this until after -next merges have completed and then handle it via ipsec.git. net/xfrm/xfrm_policy.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 6aea8b2f45e0..d2dddc570f4f 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -3400,11 +3400,18 @@ decode_session4(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve fl4->fl4_dport = flkeys->ports.dst; } + switch (flkeys->basic.ip_proto) { + case IPPROTO_GRE: + fl4->fl4_gre_key = flkeys->gre.keyid; + break; + case IPPROTO_ICMP: + fl4->fl4_icmp_type = flkeys->icmp.type; + fl4->fl4_icmp_code = flkeys->icmp.code; + break; + } + fl4->flowi4_proto = flkeys->basic.ip_proto; fl4->flowi4_tos = flkeys->ip.tos; - fl4->fl4_icmp_type = flkeys->icmp.type; - fl4->fl4_icmp_type = flkeys->icmp.code; - fl4->fl4_gre_key = flkeys->gre.keyid; } #if IS_ENABLED(CONFIG_IPV6) @@ -3427,10 +3434,17 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve fl6->fl6_dport = flkeys->ports.dst; } + switch (flkeys->basic.ip_proto) { + case IPPROTO_GRE: + fl6->fl6_gre_key = flkeys->gre.keyid; + break; + case IPPROTO_ICMPV6: + fl6->fl6_icmp_type = flkeys->icmp.type; + fl6->fl6_icmp_code = flkeys->icmp.code; + break; + } + fl6->flowi6_proto = flkeys->basic.ip_proto; - fl6->fl6_icmp_type = flkeys->icmp.type; - fl6->fl6_icmp_type = flkeys->icmp.code; - fl6->fl6_gre_key = flkeys->gre.keyid; } #endif -- 2.41.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH ipsec-next v2] xfrm: policy: fix layer 4 flowi decoding 2023-10-26 14:45 ` [PATCH ipsec-next v2] " Florian Westphal @ 2023-10-28 8:31 ` Steffen Klassert 0 siblings, 0 replies; 13+ messages in thread From: Steffen Klassert @ 2023-10-28 8:31 UTC (permalink / raw) To: Florian Westphal; +Cc: netdev, herbert, Antony Antony On Thu, Oct 26, 2023 at 04:45:42PM +0200, Florian Westphal wrote: > The commit shipped with two bugs: > fl4->fl4_icmp_type = flkeys->icmp.type; > fl4->fl4_icmp_type = flkeys->icmp.code; > ~~~~ should have been "code". > > But the more severe bug is that I got fooled by flowi member defines: > fl4_icmp_type, fl4_gre_key and fl4_dport share the same union/address. > > Fix typo and make gre/icmp key setting depend on the l4 protocol. > > Fixes: 7a0207094f1b ("xfrm: policy: replace session decode with flow dissector") > Reported-and-tested-by: Antony Antony <antony@phenome.org> > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > v2: decode_session6 must use IPPROTO_ICMPV6, not IPPROTO_ICMP. > > I normally do not resend immediately but in this case it seems like the > lesser evil. This was indeed the better way to do it. Applied, thanks a lot! ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-10-28 8:31 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-04 16:09 [PATCH ipsec-next v3 0/3] xfrm: policy: replace session decode with flow dissector Florian Westphal 2023-10-04 16:09 ` [PATCH ipsec-next v3 1/3] xfrm: pass struct net to xfrm_decode_session wrappers Florian Westphal 2023-10-05 12:05 ` Simon Horman 2023-10-04 16:09 ` [PATCH ipsec-next v3 2/3] xfrm: move mark and oif flowi decode into common code Florian Westphal 2023-10-04 16:09 ` [PATCH ipsec-next v3 3/3] xfrm: policy: replace session decode with flow dissector Florian Westphal 2023-10-10 7:51 ` [PATCH ipsec-next v3 0/3] " Steffen Klassert 2023-10-26 12:12 ` Antony Antony 2023-10-26 12:57 ` Florian Westphal 2023-10-26 14:33 ` Antony Antony 2023-10-26 14:36 ` [PATCH ipsec-next] xfrm: policy: fix layer 4 flowi decoding Florian Westphal 2023-10-26 14:38 ` Florian Westphal 2023-10-26 14:45 ` [PATCH ipsec-next v2] " Florian Westphal 2023-10-28 8:31 ` Steffen Klassert
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).