Re: [PATCH bpf-next] bpf: add End.DT6 action to bpf_lwt_seg6_action helper

[Daniel Borkmann <daniel@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.xhonneux@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.