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

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

[Mathieu Xhonneux]

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://tools.ietf.org/html/draft-filsfils-spring-srv6-network-programming-05

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.

v2: - changed true/false -> 1/0
    - preempt_enable no longer called in first conditional block

Signed-off-by: Mathieu Xhonneux <m.xhonneux@gmail.com>
---
 include/net/seg6_local.h |  4 ++-
 net/core/filter.c        | 83 +++++++++++++++++++++++++++++++++---------------
 net/ipv6/seg6_local.c    | 48 ++++++++++++++++++----------
 3 files changed, 91 insertions(+), 44 deletions(-)

diff --git a/include/net/seg6_local.h b/include/net/seg6_local.h
index 661fd5b4d3e0..08359e2d8b35 100644
--- a/include/net/seg6_local.h
+++ b/include/net/seg6_local.h
@@ -21,10 +21,12 @@
 
 extern int seg6_lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
 			       u32 tbl_id);
+extern bool seg6_bpf_has_valid_srh(struct sk_buff *skb);
 
 struct seg6_bpf_srh_state {
-	bool valid;
+	struct ipv6_sr_hdr *srh;
 	u16 hdrlen;
+	bool valid;
 };
 
 DECLARE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states);
diff --git a/net/core/filter.c b/net/core/filter.c
index 104d560946da..2cdea7d05063 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4542,14 +4542,13 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
 {
 	struct seg6_bpf_srh_state *srh_state =
 		this_cpu_ptr(&seg6_bpf_srh_states);
+	struct ipv6_sr_hdr *srh = srh_state->srh;
 	void *srh_tlvs, *srh_end, *ptr;
-	struct ipv6_sr_hdr *srh;
 	int srhoff = 0;
 
-	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
+	if (srh == NULL)
 		return -EINVAL;
 
-	srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
 	srh_tlvs = (void *)((char *)srh + ((srh->first_segment + 1) << 4));
 	srh_end = (void *)((char *)srh + sizeof(*srh) + srh_state->hdrlen);
 
@@ -4562,6 +4561,9 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
 
 	if (unlikely(bpf_try_make_writable(skb, offset + len)))
 		return -EFAULT;
+	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
+		return -EINVAL;
+	srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
 
 	memcpy(skb->data + offset, from, len);
 	return 0;
@@ -4577,52 +4579,79 @@ static const struct bpf_func_proto bpf_lwt_seg6_store_bytes_proto = {
 	.arg4_type	= ARG_CONST_SIZE
 };
 
-BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
-	   u32, action, void *, param, u32, param_len)
+static void bpf_update_srh_state(struct sk_buff *skb)
 {
 	struct seg6_bpf_srh_state *srh_state =
 		this_cpu_ptr(&seg6_bpf_srh_states);
-	struct ipv6_sr_hdr *srh;
 	int srhoff = 0;
-	int err;
-
-	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
-		return -EINVAL;
-	srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
-
-	if (!srh_state->valid) {
-		if (unlikely((srh_state->hdrlen & 7) != 0))
-			return -EBADMSG;
-
-		srh->hdrlen = (u8)(srh_state->hdrlen >> 3);
-		if (unlikely(!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3)))
-			return -EBADMSG;
 
+	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0) {
+		srh_state->srh = NULL;
+	} else {
+		srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
+		srh_state->hdrlen = srh_state->srh->hdrlen << 3;
 		srh_state->valid = 1;
 	}
+}
+
+BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
+	   u32, action, void *, param, u32, param_len)
+{
+	struct seg6_bpf_srh_state *srh_state =
+		this_cpu_ptr(&seg6_bpf_srh_states);
+	int hdroff = 0;
+	int err;
 
 	switch (action) {
 	case SEG6_LOCAL_ACTION_END_X:
+		if (!seg6_bpf_has_valid_srh(skb))
+			return -EBADMSG;
 		if (param_len != sizeof(struct in6_addr))
 			return -EINVAL;
 		return seg6_lookup_nexthop(skb, (struct in6_addr *)param, 0);
 	case SEG6_LOCAL_ACTION_END_T:
+		if (!seg6_bpf_has_valid_srh(skb))
+			return -EBADMSG;
+		if (param_len != sizeof(int))
+			return -EINVAL;
+		return seg6_lookup_nexthop(skb, NULL, *(int *)param);
+	case SEG6_LOCAL_ACTION_END_DT6:
+		if (!seg6_bpf_has_valid_srh(skb))
+			return -EBADMSG;
 		if (param_len != sizeof(int))
 			return -EINVAL;
+
+		// find inner IPv6 header, pull outer IPv6 header
+		if (ipv6_find_hdr(skb, &hdroff, IPPROTO_IPV6, NULL, NULL) < 0)
+			return -EBADMSG;
+		if (!pskb_pull(skb, hdroff))
+			return -EBADMSG;
+
+		skb_postpull_rcsum(skb, skb_network_header(skb), hdroff);
+		skb_reset_network_header(skb);
+		skb_reset_transport_header(skb);
+		skb->encapsulation = 0;
+
+		bpf_compute_data_pointers(skb);
+		bpf_update_srh_state(skb);
 		return seg6_lookup_nexthop(skb, NULL, *(int *)param);
 	case SEG6_LOCAL_ACTION_END_B6:
+		if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
+			return -EBADMSG;
 		err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6_INLINE,
 					  param, param_len);
 		if (!err)
-			srh_state->hdrlen =
-				((struct ipv6_sr_hdr *)param)->hdrlen << 3;
+			bpf_update_srh_state(skb);
+
 		return err;
 	case SEG6_LOCAL_ACTION_END_B6_ENCAP:
+		if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
+			return -EBADMSG;
 		err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6,
 					  param, param_len);
 		if (!err)
-			srh_state->hdrlen =
-				((struct ipv6_sr_hdr *)param)->hdrlen << 3;
+			bpf_update_srh_state(skb);
+
 		return err;
 	default:
 		return -EINVAL;
@@ -4644,15 +4673,14 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
 {
 	struct seg6_bpf_srh_state *srh_state =
 		this_cpu_ptr(&seg6_bpf_srh_states);
+	struct ipv6_sr_hdr *srh = srh_state->srh;
 	void *srh_end, *srh_tlvs, *ptr;
-	struct ipv6_sr_hdr *srh;
 	struct ipv6hdr *hdr;
 	int srhoff = 0;
 	int ret;
 
-	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
+	if (unlikely(srh == NULL))
 		return -EINVAL;
-	srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
 
 	srh_tlvs = (void *)((unsigned char *)srh + sizeof(*srh) +
 			((srh->first_segment + 1) << 4));
@@ -4682,6 +4710,9 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
 	hdr = (struct ipv6hdr *)skb->data;
 	hdr->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
 
+	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
+		return -EINVAL;
+	srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
 	srh_state->hdrlen += len;
 	srh_state->valid = 0;
 	return 0;
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
index e1025b493a18..01f5d6c17f3f 100644
--- a/net/ipv6/seg6_local.c
+++ b/net/ipv6/seg6_local.c
@@ -459,25 +459,49 @@ static int input_action_end_b6_encap(struct sk_buff *skb,
 
 DEFINE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states);
 
+bool seg6_bpf_has_valid_srh(struct sk_buff *skb)
+{
+	struct seg6_bpf_srh_state *srh_state =
+		this_cpu_ptr(&seg6_bpf_srh_states);
+	struct ipv6_sr_hdr *srh = srh_state->srh;
+
+	if (unlikely(srh == NULL))
+		return 0;
+
+	if (unlikely(!srh_state->valid)) {
+		if ((srh_state->hdrlen & 7) != 0)
+			return 0;
+
+		srh->hdrlen = (u8)(srh_state->hdrlen >> 3);
+		if (!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3))
+			return 0;
+
+		srh_state->valid = 1;
+	}
+
+	return 1;
+}
+
 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);
-	if (!srh)
-		goto drop;
+	if (!srh) {
+		kfree_skb(skb);
+		return -EINVAL;
+	}
 	advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
 
 	/* preempt_disable is needed to protect the per-CPU buffer srh_state,
 	 * 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 +510,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 +521,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();
 	kfree_skb(skb);
 	return -EINVAL;
 }
-- 
2.16.1

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

[Martin KaFai Lau]

On Wed, Jul 25, 2018 at 12:36:45PM +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=xOQOjR3OUfKkBdRSeFH8x1QqbAb8VVRwECipEqCJyuw&s=L3YiDuRAH4hYSETfa5t_5q2BqaYKJR4d8Vqa8dqqHGo&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.
> 
> v2: - changed true/false -> 1/0
hmmm...I thought I was asking to replace 1/0 with true/false.  More
below.

>     - preempt_enable no longer called in first conditional block
> 
> Signed-off-by: Mathieu Xhonneux <m.xhonneux@gmail.com>
> ---
>  include/net/seg6_local.h |  4 ++-
>  net/core/filter.c        | 83 +++++++++++++++++++++++++++++++++---------------
>  net/ipv6/seg6_local.c    | 48 ++++++++++++++++++----------
>  3 files changed, 91 insertions(+), 44 deletions(-)
> 
> diff --git a/include/net/seg6_local.h b/include/net/seg6_local.h
> index 661fd5b4d3e0..08359e2d8b35 100644
> --- a/include/net/seg6_local.h
> +++ b/include/net/seg6_local.h
> @@ -21,10 +21,12 @@
>  
>  extern int seg6_lookup_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
>  			       u32 tbl_id);
> +extern bool seg6_bpf_has_valid_srh(struct sk_buff *skb);
>  
>  struct seg6_bpf_srh_state {
> -	bool valid;
> +	struct ipv6_sr_hdr *srh;
>  	u16 hdrlen;
> +	bool valid;
"valid" is a bool, so it is easier to read
if true/false is used in srh_state->valid = true/false;

>  };
>  
>  DECLARE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states);
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 104d560946da..2cdea7d05063 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4542,14 +4542,13 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
>  {
>  	struct seg6_bpf_srh_state *srh_state =
>  		this_cpu_ptr(&seg6_bpf_srh_states);
> +	struct ipv6_sr_hdr *srh = srh_state->srh;
>  	void *srh_tlvs, *srh_end, *ptr;
> -	struct ipv6_sr_hdr *srh;
>  	int srhoff = 0;
>  
> -	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> +	if (srh == NULL)
>  		return -EINVAL;
>  
> -	srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>  	srh_tlvs = (void *)((char *)srh + ((srh->first_segment + 1) << 4));
>  	srh_end = (void *)((char *)srh + sizeof(*srh) + srh_state->hdrlen);
>  
> @@ -4562,6 +4561,9 @@ BPF_CALL_4(bpf_lwt_seg6_store_bytes, struct sk_buff *, skb, u32, offset,
>  
>  	if (unlikely(bpf_try_make_writable(skb, offset + len)))
>  		return -EFAULT;
> +	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> +		return -EINVAL;
> +	srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>  
>  	memcpy(skb->data + offset, from, len);
>  	return 0;
> @@ -4577,52 +4579,79 @@ static const struct bpf_func_proto bpf_lwt_seg6_store_bytes_proto = {
>  	.arg4_type	= ARG_CONST_SIZE
>  };
>  
> -BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
> -	   u32, action, void *, param, u32, param_len)
> +static void bpf_update_srh_state(struct sk_buff *skb)
>  {
>  	struct seg6_bpf_srh_state *srh_state =
>  		this_cpu_ptr(&seg6_bpf_srh_states);
> -	struct ipv6_sr_hdr *srh;
>  	int srhoff = 0;
> -	int err;
> -
> -	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> -		return -EINVAL;
> -	srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> -
> -	if (!srh_state->valid) {
> -		if (unlikely((srh_state->hdrlen & 7) != 0))
> -			return -EBADMSG;
> -
> -		srh->hdrlen = (u8)(srh_state->hdrlen >> 3);
> -		if (unlikely(!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3)))
> -			return -EBADMSG;
>  
> +	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0) {
> +		srh_state->srh = NULL;
> +	} else {
> +		srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
> +		srh_state->hdrlen = srh_state->srh->hdrlen << 3;
>  		srh_state->valid = 1;
e.g. here

>  	}
> +}
> +
> +BPF_CALL_4(bpf_lwt_seg6_action, struct sk_buff *, skb,
> +	   u32, action, void *, param, u32, param_len)
> +{
> +	struct seg6_bpf_srh_state *srh_state =
> +		this_cpu_ptr(&seg6_bpf_srh_states);
> +	int hdroff = 0;
> +	int err;
>  
>  	switch (action) {
>  	case SEG6_LOCAL_ACTION_END_X:
> +		if (!seg6_bpf_has_valid_srh(skb))
> +			return -EBADMSG;
>  		if (param_len != sizeof(struct in6_addr))
>  			return -EINVAL;
>  		return seg6_lookup_nexthop(skb, (struct in6_addr *)param, 0);
>  	case SEG6_LOCAL_ACTION_END_T:
> +		if (!seg6_bpf_has_valid_srh(skb))
> +			return -EBADMSG;
> +		if (param_len != sizeof(int))
> +			return -EINVAL;
> +		return seg6_lookup_nexthop(skb, NULL, *(int *)param);
> +	case SEG6_LOCAL_ACTION_END_DT6:
> +		if (!seg6_bpf_has_valid_srh(skb))
> +			return -EBADMSG;
>  		if (param_len != sizeof(int))
>  			return -EINVAL;
> +
> +		// find inner IPv6 header, pull outer IPv6 header
> +		if (ipv6_find_hdr(skb, &hdroff, IPPROTO_IPV6, NULL, NULL) < 0)
> +			return -EBADMSG;
> +		if (!pskb_pull(skb, hdroff))
> +			return -EBADMSG;
> +
> +		skb_postpull_rcsum(skb, skb_network_header(skb), hdroff);
> +		skb_reset_network_header(skb);
> +		skb_reset_transport_header(skb);
> +		skb->encapsulation = 0;
> +
> +		bpf_compute_data_pointers(skb);
> +		bpf_update_srh_state(skb);
>  		return seg6_lookup_nexthop(skb, NULL, *(int *)param);
>  	case SEG6_LOCAL_ACTION_END_B6:
> +		if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
> +			return -EBADMSG;
>  		err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6_INLINE,
>  					  param, param_len);
>  		if (!err)
> -			srh_state->hdrlen =
> -				((struct ipv6_sr_hdr *)param)->hdrlen << 3;
> +			bpf_update_srh_state(skb);
> +
>  		return err;
>  	case SEG6_LOCAL_ACTION_END_B6_ENCAP:
> +		if (srh_state->srh && !seg6_bpf_has_valid_srh(skb))
> +			return -EBADMSG;
>  		err = bpf_push_seg6_encap(skb, BPF_LWT_ENCAP_SEG6,
>  					  param, param_len);
>  		if (!err)
> -			srh_state->hdrlen =
> -				((struct ipv6_sr_hdr *)param)->hdrlen << 3;
> +			bpf_update_srh_state(skb);
> +
>  		return err;
>  	default:
>  		return -EINVAL;
> @@ -4644,15 +4673,14 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
>  {
>  	struct seg6_bpf_srh_state *srh_state =
>  		this_cpu_ptr(&seg6_bpf_srh_states);
> +	struct ipv6_sr_hdr *srh = srh_state->srh;
>  	void *srh_end, *srh_tlvs, *ptr;
> -	struct ipv6_sr_hdr *srh;
>  	struct ipv6hdr *hdr;
>  	int srhoff = 0;
>  	int ret;
>  
> -	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> +	if (unlikely(srh == NULL))
>  		return -EINVAL;
> -	srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>  
>  	srh_tlvs = (void *)((unsigned char *)srh + sizeof(*srh) +
>  			((srh->first_segment + 1) << 4));
> @@ -4682,6 +4710,9 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
>  	hdr = (struct ipv6hdr *)skb->data;
>  	hdr->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
>  
> +	if (ipv6_find_hdr(skb, &srhoff, IPPROTO_ROUTING, NULL, NULL) < 0)
> +		return -EINVAL;
> +	srh_state->srh = (struct ipv6_sr_hdr *)(skb->data + srhoff);
>  	srh_state->hdrlen += len;
>  	srh_state->valid = 0;
>  	return 0;
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index e1025b493a18..01f5d6c17f3f 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -459,25 +459,49 @@ static int input_action_end_b6_encap(struct sk_buff *skb,
>  
>  DEFINE_PER_CPU(struct seg6_bpf_srh_state, seg6_bpf_srh_states);
>  
> +bool seg6_bpf_has_valid_srh(struct sk_buff *skb)
Same here. returning true/false is better.

> +{
> +	struct seg6_bpf_srh_state *srh_state =
> +		this_cpu_ptr(&seg6_bpf_srh_states);
> +	struct ipv6_sr_hdr *srh = srh_state->srh;
> +
> +	if (unlikely(srh == NULL))
> +		return 0;
> +
> +	if (unlikely(!srh_state->valid)) {
> +		if ((srh_state->hdrlen & 7) != 0)
> +			return 0;
> +
> +		srh->hdrlen = (u8)(srh_state->hdrlen >> 3);
> +		if (!seg6_validate_srh(srh, (srh->hdrlen + 1) << 3))
> +			return 0;
> +
> +		srh_state->valid = 1;
> +	}
> +
> +	return 1;
> +}
> +
>  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);
> -	if (!srh)
> -		goto drop;
> +	if (!srh) {
> +		kfree_skb(skb);
> +		return -EINVAL;
> +	}
>  	advance_nextseg(srh, &ipv6_hdr(skb)->daddr);
>  
>  	/* preempt_disable is needed to protect the per-CPU buffer srh_state,
>  	 * 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 +510,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 +521,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();
>  	kfree_skb(skb);
>  	return -EINVAL;
>  }
> -- 
> 2.16.1
> 

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

[Mathieu Xhonneux]

2018-07-25 23:13 GMT+02:00 Martin KaFai Lau <kafai@fb.com>:
>> v2: - changed true/false -> 1/0
> hmmm...I thought I was asking to replace 1/0 with true/false.  More
> below.

Silly me, I read your indication backwards. Agreed.

@Daniel: sorry for this one, sending a v3.