* [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void
@ 2020-01-03 6:02 Li RongQing
2020-01-03 8:27 ` Simon Horman
0 siblings, 1 reply; 5+ messages in thread
From: Li RongQing @ 2020-01-03 6:02 UTC (permalink / raw)
To: netdev, bpf
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void
2020-01-03 6:02 [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void Li RongQing
@ 2020-01-03 8:27 ` Simon Horman
2020-01-03 19:18 ` Song Liu
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2020-01-03 8:27 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev, bpf
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void
2020-01-03 8:27 ` Simon Horman
@ 2020-01-03 19:18 ` Song Liu
2020-01-06 22:32 ` Alexei Starovoitov
0 siblings, 1 reply; 5+ messages in thread
From: Song Liu @ 2020-01-03 19:18 UTC (permalink / raw)
To: Simon Horman; +Cc: Li RongQing, Networking, bpf
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void
2020-01-03 19:18 ` Song Liu
@ 2020-01-06 22:32 ` Alexei Starovoitov
2020-01-06 22:35 ` Daniel Borkmann
0 siblings, 1 reply; 5+ messages in thread
From: Alexei Starovoitov @ 2020-01-06 22:32 UTC (permalink / raw)
To: Song Liu; +Cc: Simon Horman, Li RongQing, Networking, bpf
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void
2020-01-06 22:32 ` Alexei Starovoitov
@ 2020-01-06 22:35 ` Daniel Borkmann
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Borkmann @ 2020-01-06 22:35 UTC (permalink / raw)
To: Alexei Starovoitov, Song Liu; +Cc: Simon Horman, Li RongQing, Networking, bpf
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-01-06 22:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-03 6:02 [PATCH][bpf-next] bpf: change bpf_skb_generic_push type as void Li RongQing
2020-01-03 8:27 ` Simon Horman
2020-01-03 19:18 ` Song Liu
2020-01-06 22:32 ` Alexei Starovoitov
2020-01-06 22:35 ` Daniel Borkmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.