| Message ID | 20200728122044.1900-1-ahabdels@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] seg6: using DSCP of inner IPv4 packets | expand |
From: Ahmed Abdelsalam <ahabdels@gmail.com> Date: Tue, 28 Jul 2020 12:20:44 +0000 > This patch allows copying the DSCP from inner IPv4 header to the > outer IPv6 header, when doing SRv6 Encapsulation. > > This allows forwarding packet across the SRv6 fabric based on their > original traffic class. > > Signed-off-by: Ahmed Abdelsalam <ahabdels@gmail.com> The conditionals in this function are now a mess. > - inner_hdr = ipv6_hdr(skb); > + if (skb->protocol == htons(ETH_P_IPV6)) > + inner_hdr = ipv6_hdr(skb); > + else > + inner_ipv4_hdr = ip_hdr(skb); > + You assume that if skb->protocol is not ipv6 then it is ipv4. > @@ -138,6 +143,10 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto) > ip6_flow_hdr(hdr, ip6_tclass(ip6_flowinfo(inner_hdr)), > flowlabel); > hdr->hop_limit = inner_hdr->hop_limit; > + } else if (skb->protocol == htons(ETH_P_IP)) { > + ip6_flow_hdr(hdr, inner_ipv4_hdr->tos, flowlabel); > + hdr->hop_limit = inner_ipv4_hdr->ttl; > + memset(IP6CB(skb), 0, sizeof(*IP6CB(skb))); > } else { > ip6_flow_hdr(hdr, 0, flowlabel); > hdr->hop_limit = ip6_dst_hoplimit(skb_dst(skb)); But this code did not make that assumption at all. Only one of the two can be correct. The conditional assignment is also very ugly, you have two pointers conditionally initialized. The compiler is going to have a hard time figuring out that each pointer is only used in the code path where it is guaranteed to be initialiazed. And it can't do that, as far as the compiler knows, skb->protocol can change between those two locations. It MUST assume that can happen if there are any functions calls whatsoever between these two code points. This function has to be sanitized, with better handling of access to the inner protocol header values, before I am willing to apply this.
I will refactor the code of this function and submit a new patch. Ahmed On 31/07/2020 01:44, David Miller wrote: > From: Ahmed Abdelsalam <ahabdels@gmail.com> > Date: Tue, 28 Jul 2020 12:20:44 +0000 > >> This patch allows copying the DSCP from inner IPv4 header to the >> outer IPv6 header, when doing SRv6 Encapsulation. >> >> This allows forwarding packet across the SRv6 fabric based on their >> original traffic class. >> >> Signed-off-by: Ahmed Abdelsalam <ahabdels@gmail.com> > > The conditionals in this function are now a mess. > >> - inner_hdr = ipv6_hdr(skb); >> + if (skb->protocol == htons(ETH_P_IPV6)) >> + inner_hdr = ipv6_hdr(skb); >> + else >> + inner_ipv4_hdr = ip_hdr(skb); >> + > > You assume that if skb->protocol is not ipv6 then it is ipv4. > >> @@ -138,6 +143,10 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto) >> ip6_flow_hdr(hdr, ip6_tclass(ip6_flowinfo(inner_hdr)), >> flowlabel); >> hdr->hop_limit = inner_hdr->hop_limit; >> + } else if (skb->protocol == htons(ETH_P_IP)) { >> + ip6_flow_hdr(hdr, inner_ipv4_hdr->tos, flowlabel); >> + hdr->hop_limit = inner_ipv4_hdr->ttl; >> + memset(IP6CB(skb), 0, sizeof(*IP6CB(skb))); >> } else { >> ip6_flow_hdr(hdr, 0, flowlabel); >> hdr->hop_limit = ip6_dst_hoplimit(skb_dst(skb)); > > But this code did not make that assumption at all. > > Only one of the two can be correct. > > The conditional assignment is also very ugly, you have two pointers > conditionally initialized. The compiler is going to have a hard time > figuring out that each pointer is only used in the code path where it > is guaranteed to be initialiazed. > > And it can't do that, as far as the compiler knows, skb->protocol can > change between those two locations. It MUST assume that can happen if > there are any functions calls whatsoever between these two code points. > > This function has to be sanitized, with better handling of access to > the inner protocol header values, before I am willing to apply this. >
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c index e0e9f48ab14f..9753d10c4a51 100644 --- a/net/ipv6/seg6_iptunnel.c +++ b/net/ipv6/seg6_iptunnel.c @@ -110,6 +110,7 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto) struct dst_entry *dst = skb_dst(skb); struct net *net = dev_net(dst->dev); struct ipv6hdr *hdr, *inner_hdr; + struct iphdr *inner_ipv4_hdr; struct ipv6_sr_hdr *isrh; int hdrlen, tot_len, err; __be32 flowlabel; @@ -121,7 +122,11 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto) if (unlikely(err)) return err; - inner_hdr = ipv6_hdr(skb); + if (skb->protocol == htons(ETH_P_IPV6)) + inner_hdr = ipv6_hdr(skb); + else + inner_ipv4_hdr = ip_hdr(skb); + flowlabel = seg6_make_flowlabel(net, skb, inner_hdr); skb_push(skb, tot_len); @@ -138,6 +143,10 @@ int seg6_do_srh_encap(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto) ip6_flow_hdr(hdr, ip6_tclass(ip6_flowinfo(inner_hdr)), flowlabel); hdr->hop_limit = inner_hdr->hop_limit; + } else if (skb->protocol == htons(ETH_P_IP)) { + ip6_flow_hdr(hdr, inner_ipv4_hdr->tos, flowlabel); + hdr->hop_limit = inner_ipv4_hdr->ttl; + memset(IP6CB(skb), 0, sizeof(*IP6CB(skb))); } else { ip6_flow_hdr(hdr, 0, flowlabel); hdr->hop_limit = ip6_dst_hoplimit(skb_dst(skb));
This patch allows copying the DSCP from inner IPv4 header to the outer IPv6 header, when doing SRv6 Encapsulation. This allows forwarding packet across the SRv6 fabric based on their original traffic class. Signed-off-by: Ahmed Abdelsalam <ahabdels@gmail.com> --- net/ipv6/seg6_iptunnel.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)