Indeed, I missed this one. Thanks, sending a v2.
2018-07-25 5:40 GMT+00:00 Daniel Borkmann <dan...@iogearbox.net>: > On 07/24/2018 07:14 PM, Martin KaFai Lau wrote: >> On Tue, Jul 24, 2018 at 04:59:54PM +0000, Mathieu Xhonneux wrote: >>> The seg6local LWT provides the End.DT6 action, which allows to >>> decapsulate an outer IPv6 header containing a Segment Routing Header >>> (SRH), full specification is available here: >>> >>> https://urldefense.proofpoint.com/v2/url?u=https-3A__tools.ietf.org_html_draft-2Dfilsfils-2Dspring-2Dsrv6-2Dnetwork-2Dprogramming-2D05&d=DwIBAg&c=5VD0RTtNlTh3ycd41b3MUw&r=VQnoQ7LvghIj0gVEaiQSUw&m=c61PGnhPMmCUcL5lpyBsxOmsBU2mU5KFY0-Ioo-pBC4&s=mzShtRc5ofzfknAuqoehbGN1ifA17aKihiVLJVfkuZ8&e= >>> >>> This patch adds this action now to the seg6local BPF >>> interface. Since it is not mandatory that the inner IPv6 header also >>> contains a SRH, seg6_bpf_srh_state has been extended with a pointer to >>> a possible SRH of the outermost IPv6 header. This helps assessing if the >>> validation must be triggered or not, and avoids some calls to >>> ipv6_find_hdr. >>> >>> Signed-off-by: Mathieu Xhonneux <m.xhonn...@gmail.com> > [...] >>> + >>> static int input_action_end_bpf(struct sk_buff *skb, >>> struct seg6_local_lwt *slwt) >>> { >>> struct seg6_bpf_srh_state *srh_state = >>> this_cpu_ptr(&seg6_bpf_srh_states); >>> - struct seg6_bpf_srh_state local_srh_state; >>> struct ipv6_sr_hdr *srh; >>> - int srhoff = 0; >>> int ret; >>> >>> srh = get_and_validate_srh(skb); >>> @@ -478,6 +499,7 @@ static int input_action_end_bpf(struct sk_buff *skb, >>> * which is also accessed by the bpf_lwt_seg6_* helpers >>> */ >>> preempt_disable(); >>> + srh_state->srh = srh; >>> srh_state->hdrlen = srh->hdrlen << 3; >>> srh_state->valid = 1; >>> >>> @@ -486,9 +508,6 @@ static int input_action_end_bpf(struct sk_buff *skb, >>> ret = bpf_prog_run_save_cb(slwt->bpf.prog, skb); >>> rcu_read_unlock(); >>> >>> - local_srh_state = *srh_state; >>> - preempt_enable(); >>> - >>> switch (ret) { >>> case BPF_OK: >>> case BPF_REDIRECT: >>> @@ -500,24 +519,17 @@ static int input_action_end_bpf(struct sk_buff *skb, >>> goto drop; >>> } >>> >>> - if (unlikely((local_srh_state.hdrlen & 7) != 0)) >>> - goto drop; >>> - >>> - if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0) >>> - goto drop; >>> - srh = (struct ipv6_sr_hdr *)(skb->data + srhoff); >>> - srh->hdrlen = (u8)(local_srh_state.hdrlen >> 3); >>> - >>> - if (!local_srh_state.valid && >>> - unlikely(!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3))) >>> + if (srh_state->srh && !seg6_bpf_has_valid_srh(skb)) >>> goto drop; >>> >>> + preempt_enable(); >>> if (ret != BPF_REDIRECT) >>> seg6_lookup_nexthop(skb, NULL, 0); >>> >>> return dst_input(skb); >>> >>> drop: >>> + preempt_enable(); >> For this drop case at the beginning of this function: >> >> srh = get_and_validate_srh(skb); >> if (!srh) >> goto drop; >> >> preempt_disable() was not called yet? > > Agree, this is buggy.
