[PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void

[PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void

[Li RongQing]

bpf_skb_generic_push always returns 0, not need to check
its return, so change its type as void

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 net/core/filter.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 42fd17c48c5f..1cbac34a4e11 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2761,7 +2761,7 @@ static const struct bpf_func_proto bpf_skb_vlan_pop_proto = {
 	.arg1_type      = ARG_PTR_TO_CTX,
 };
 
-static int bpf_skb_generic_push(struct sk_buff *skb, u32 off, u32 len)
+static void bpf_skb_generic_push(struct sk_buff *skb, u32 off, u32 len)
 {
 	/* Caller already did skb_cow() with len as headroom,
 	 * so no need to do it here.
@@ -2775,7 +2775,6 @@ static int bpf_skb_generic_push(struct sk_buff *skb, u32 off, u32 len)
 	 * result for checksum complete when summing over
 	 * zeroed blocks.
 	 */
-	return 0;
 }
 
 static int bpf_skb_generic_pop(struct sk_buff *skb, u32 off, u32 len)
@@ -2793,24 +2792,19 @@ static int bpf_skb_generic_pop(struct sk_buff *skb, u32 off, u32 len)
 	return 0;
 }
 
-static int bpf_skb_net_hdr_push(struct sk_buff *skb, u32 off, u32 len)
+static void bpf_skb_net_hdr_push(struct sk_buff *skb, u32 off, u32 len)
 {
 	bool trans_same = skb->transport_header == skb->network_header;
-	int ret;
 
 	/* There's no need for __skb_push()/__skb_pull() pair to
 	 * get to the start of the mac header as we're guaranteed
 	 * to always start from here under eBPF.
 	 */
-	ret = bpf_skb_generic_push(skb, off, len);
-	if (likely(!ret)) {
-		skb->mac_header -= len;
-		skb->network_header -= len;
-		if (trans_same)
-			skb->transport_header = skb->network_header;
-	}
-
-	return ret;
+	bpf_skb_generic_push(skb, off, len);
+	skb->mac_header -= len;
+	skb->network_header -= len;
+	if (trans_same)
+		skb->transport_header = skb->network_header;
 }
 
 static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len)
@@ -2843,9 +2837,7 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb)
 	if (unlikely(ret < 0))
 		return ret;
 
-	ret = bpf_skb_net_hdr_push(skb, off, len_diff);
-	if (unlikely(ret < 0))
-		return ret;
+	bpf_skb_net_hdr_push(skb, off, len_diff);
 
 	if (skb_is_gso(skb)) {
 		struct skb_shared_info *shinfo = skb_shinfo(skb);
@@ -3050,9 +3042,7 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
 		inner_trans = skb->transport_header;
 	}
 
-	ret = bpf_skb_net_hdr_push(skb, off, len_diff);
-	if (unlikely(ret < 0))
-		return ret;
+	bpf_skb_net_hdr_push(skb, off, len_diff);
 
 	if (encap) {
 		skb->inner_mac_header = inner_net - inner_mac_len;
@@ -5144,7 +5134,7 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
 		if (unlikely(ret < 0))
 			return ret;
 
-		ret = bpf_skb_net_hdr_push(skb, offset, len);
+		bpf_skb_net_hdr_push(skb, offset, len);
 	} else {
 		ret = bpf_skb_net_hdr_pop(skb, offset, -1 * len);
 	}
-- 
2.16.2

Re: [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void

[Simon Horman]

On Fri, Jan 03, 2020 at 02:02:33PM +0800, Li RongQing wrote:
> bpf_skb_generic_push always returns 0, not need to check
> its return, so change its type as void
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

Reviewed-by: Simon Horman <simon.horman@netronome.com>

> ---
>  net/core/filter.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 42fd17c48c5f..1cbac34a4e11 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

...

> @@ -5144,7 +5134,7 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
>  		if (unlikely(ret < 0))
>  			return ret;
>  
> -		ret = bpf_skb_net_hdr_push(skb, offset, len);
> +		bpf_skb_net_hdr_push(skb, offset, len);

There is a check for (ret < 0) just below this if block.
That is ok becuase in order to get to here (ret < 0) must
be true as per the check a few lines above.

So I think this is ok although the asymmetry with the else arm
of this if statement is not ideal IMHO.

>  	} else {
>  		ret = bpf_skb_net_hdr_pop(skb, offset, -1 * len);
>  	}
> -- 
> 2.16.2
> 

Re: [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void

[Song Liu]

On Fri, Jan 3, 2020 at 12:27 AM Simon Horman <simon.horman@netronome.com> wrote:
>
> On Fri, Jan 03, 2020 at 02:02:33PM +0800, Li RongQing wrote:
> > bpf_skb_generic_push always returns 0, not need to check
> > its return, so change its type as void
> >
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
Acked-by: Song Liu <songliubraving@fb.com>

>
> > ---
> >  net/core/filter.c | 30 ++++++++++--------------------
> >  1 file changed, 10 insertions(+), 20 deletions(-)
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 42fd17c48c5f..1cbac34a4e11 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
>
> ...
>
> > @@ -5144,7 +5134,7 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
> >               if (unlikely(ret < 0))
> >                       return ret;
> >
> > -             ret = bpf_skb_net_hdr_push(skb, offset, len);
> > +             bpf_skb_net_hdr_push(skb, offset, len);
>
> There is a check for (ret < 0) just below this if block.
> That is ok becuase in order to get to here (ret < 0) must
> be true as per the check a few lines above.
>
> So I think this is ok although the asymmetry with the else arm
> of this if statement is not ideal IMHO.

Agreed with this concern. But I cannot think of any free solution. I guess we
will just live with assumption that skb_cow_head() never return >0.

Thanks,
Song

Re: [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void

[Alexei Starovoitov]

On Fri, Jan 03, 2020 at 11:18:28AM -0800, Song Liu wrote:
> On Fri, Jan 3, 2020 at 12:27 AM Simon Horman <simon.horman@netronome.com> wrote:
> >
> > On Fri, Jan 03, 2020 at 02:02:33PM +0800, Li RongQing wrote:
> > > bpf_skb_generic_push always returns 0, not need to check
> > > its return, so change its type as void
> > >
> > > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> >
> > Reviewed-by: Simon Horman <simon.horman@netronome.com>
> Acked-by: Song Liu <songliubraving@fb.com>
> 
> >
> > > ---
> > >  net/core/filter.c | 30 ++++++++++--------------------
> > >  1 file changed, 10 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/net/core/filter.c b/net/core/filter.c
> > > index 42fd17c48c5f..1cbac34a4e11 100644
> > > --- a/net/core/filter.c
> > > +++ b/net/core/filter.c
> >
> > ...
> >
> > > @@ -5144,7 +5134,7 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
> > >               if (unlikely(ret < 0))
> > >                       return ret;
> > >
> > > -             ret = bpf_skb_net_hdr_push(skb, offset, len);
> > > +             bpf_skb_net_hdr_push(skb, offset, len);
> >
> > There is a check for (ret < 0) just below this if block.
> > That is ok becuase in order to get to here (ret < 0) must
> > be true as per the check a few lines above.
> >
> > So I think this is ok although the asymmetry with the else arm
> > of this if statement is not ideal IMHO.
> 
> Agreed with this concern. But I cannot think of any free solution. I guess we
> will just live with assumption that skb_cow_head() never return >0.

I don't think this patch is worth doing.
I can imagine bpf_skb_generic_push() handling some errors in the future.
compiler can do this optimization job instead.

Re: [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void

[Daniel Borkmann]

On 1/6/20 11:32 PM, Alexei Starovoitov wrote:
> On Fri, Jan 03, 2020 at 11:18:28AM -0800, Song Liu wrote:
>> On Fri, Jan 3, 2020 at 12:27 AM Simon Horman <simon.horman@netronome.com> wrote:
>>> On Fri, Jan 03, 2020 at 02:02:33PM +0800, Li RongQing wrote:
>>>> bpf_skb_generic_push always returns 0, not need to check
>>>> its return, so change its type as void
>>>>
>>>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>>>
>>> Reviewed-by: Simon Horman <simon.horman@netronome.com>
>> Acked-by: Song Liu <songliubraving@fb.com>
>>
>>>
>>>> ---
>>>>   net/core/filter.c | 30 ++++++++++--------------------
>>>>   1 file changed, 10 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>>> index 42fd17c48c5f..1cbac34a4e11 100644
>>>> --- a/net/core/filter.c
>>>> +++ b/net/core/filter.c
>>>
>>> ...
>>>
>>>> @@ -5144,7 +5134,7 @@ BPF_CALL_3(bpf_lwt_seg6_adjust_srh, struct sk_buff *, skb, u32, offset,
>>>>                if (unlikely(ret < 0))
>>>>                        return ret;
>>>>
>>>> -             ret = bpf_skb_net_hdr_push(skb, offset, len);
>>>> +             bpf_skb_net_hdr_push(skb, offset, len);
>>>
>>> There is a check for (ret < 0) just below this if block.
>>> That is ok becuase in order to get to here (ret < 0) must
>>> be true as per the check a few lines above.
>>>
>>> So I think this is ok although the asymmetry with the else arm
>>> of this if statement is not ideal IMHO.
>>
>> Agreed with this concern. But I cannot think of any free solution. I guess we
>> will just live with assumption that skb_cow_head() never return >0.
> 
> I don't think this patch is worth doing.
> I can imagine bpf_skb_generic_push() handling some errors in the future.
> compiler can do this optimization job instead.

Yep, fully agree.