| Message ID | 20231004161002.10843-1-fw@strlen.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | xfrm: policy: replace session decode with flow dissector | expand |
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!
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
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
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