| Message ID | 1605769468-2078-1-git-send-email-kaixuxia@tencent.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Check the return value of dev_get_by_index_rcu() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 45 this patch: 45 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 45 this patch: 45 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
[ +David ] On 11/19/20 8:04 AM, xiakaixu1987@gmail.com wrote: > From: Kaixu Xia <kaixuxia@tencent.com> > > The return value of dev_get_by_index_rcu() can be NULL, so here it > is need to check the return value and return error code if it is NULL. > > Signed-off-by: Kaixu Xia <kaixuxia@tencent.com> > --- > net/core/filter.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 2ca5eecebacf..1263fe07170a 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -5573,6 +5573,8 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, > struct net_device *dev; > > dev = dev_get_by_index_rcu(net, params->ifindex); > + if (unlikely(!dev)) > + return -EINVAL; > if (!is_skb_forwardable(dev, skb)) > rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; The above logic is quite ugly anyway given we fetched the dev pointer already earlier in bpf_ipv{4,6}_fib_lookup() and now need to redo it again ... so yeah there could be a tiny race in here. We wanted do bring this logic closer to what XDP does anyway, something like below, for example. David, thoughts? Thx Subject: [PATCH] diff mtu check Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> --- net/core/filter.c | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/net/core/filter.c b/net/core/filter.c index 2ca5eecebacf..3bab0a97fa38 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5547,9 +5547,6 @@ static const struct bpf_func_proto bpf_xdp_fib_lookup_proto = { BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, struct bpf_fib_lookup *, params, int, plen, u32, flags) { - struct net *net = dev_net(skb->dev); - int rc = -EAFNOSUPPORT; - if (plen < sizeof(*params)) return -EINVAL; @@ -5559,25 +5556,16 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, switch (params->family) { #if IS_ENABLED(CONFIG_INET) case AF_INET: - rc = bpf_ipv4_fib_lookup(net, params, flags, false); - break; + return bpf_ipv4_fib_lookup(dev_net(skb->dev), params, flags, + !skb_is_gso(skb)); #endif #if IS_ENABLED(CONFIG_IPV6) case AF_INET6: - rc = bpf_ipv6_fib_lookup(net, params, flags, false); - break; + return bpf_ipv6_fib_lookup(dev_net(skb->dev), params, flags, + !skb_is_gso(skb)); #endif } - - if (!rc) { - struct net_device *dev; - - dev = dev_get_by_index_rcu(net, params->ifindex); - if (!is_skb_forwardable(dev, skb)) - rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; - } - - return rc; + return -EAFNOSUPPORT; } static const struct bpf_func_proto bpf_skb_fib_lookup_proto = {
On 11/20/20 8:13 AM, Daniel Borkmann wrote: > [ +David ] > > On 11/19/20 8:04 AM, xiakaixu1987@gmail.com wrote: >> From: Kaixu Xia <kaixuxia@tencent.com> >> >> The return value of dev_get_by_index_rcu() can be NULL, so here it >> is need to check the return value and return error code if it is NULL. >> >> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com> >> --- >> net/core/filter.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/core/filter.c b/net/core/filter.c >> index 2ca5eecebacf..1263fe07170a 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -5573,6 +5573,8 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, >> skb, >> struct net_device *dev; >> dev = dev_get_by_index_rcu(net, params->ifindex); >> + if (unlikely(!dev)) >> + return -EINVAL; >> if (!is_skb_forwardable(dev, skb)) >> rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; rcu lock is held right? It is impossible for dev to return NULL here. > > The above logic is quite ugly anyway given we fetched the dev pointer > already earlier > in bpf_ipv{4,6}_fib_lookup() and now need to redo it again ... so yeah evolved from the different needs of the xdp and tc paths. > there could be > a tiny race in here. We wanted do bring this logic closer to what XDP > does anyway, > something like below, for example. David, thoughts? Thx > > Subject: [PATCH] diff mtu check > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > --- > net/core/filter.c | 22 +++++----------------- > 1 file changed, 5 insertions(+), 17 deletions(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 2ca5eecebacf..3bab0a97fa38 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -5547,9 +5547,6 @@ static const struct bpf_func_proto > bpf_xdp_fib_lookup_proto = { > BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, > struct bpf_fib_lookup *, params, int, plen, u32, flags) > { > - struct net *net = dev_net(skb->dev); > - int rc = -EAFNOSUPPORT; > - > if (plen < sizeof(*params)) > return -EINVAL; > > @@ -5559,25 +5556,16 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, > skb, > switch (params->family) { > #if IS_ENABLED(CONFIG_INET) > case AF_INET: > - rc = bpf_ipv4_fib_lookup(net, params, flags, false); > - break; > + return bpf_ipv4_fib_lookup(dev_net(skb->dev), params, flags, > + !skb_is_gso(skb)); > #endif > #if IS_ENABLED(CONFIG_IPV6) > case AF_INET6: > - rc = bpf_ipv6_fib_lookup(net, params, flags, false); > - break; > + return bpf_ipv6_fib_lookup(dev_net(skb->dev), params, flags, > + !skb_is_gso(skb)); seems ok. > #endif > } > - > - if (!rc) { > - struct net_device *dev; > - > - dev = dev_get_by_index_rcu(net, params->ifindex); > - if (!is_skb_forwardable(dev, skb)) > - rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; > - } > - > - return rc; > + return -EAFNOSUPPORT; > } > > static const struct bpf_func_proto bpf_skb_fib_lookup_proto = {
On 11/20/20 4:19 PM, David Ahern wrote: > On 11/20/20 8:13 AM, Daniel Borkmann wrote: >> [ +David ] >> >> On 11/19/20 8:04 AM, xiakaixu1987@gmail.com wrote: >>> From: Kaixu Xia <kaixuxia@tencent.com> >>> >>> The return value of dev_get_by_index_rcu() can be NULL, so here it >>> is need to check the return value and return error code if it is NULL. >>> >>> Signed-off-by: Kaixu Xia <kaixuxia@tencent.com> >>> --- >>> net/core/filter.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/net/core/filter.c b/net/core/filter.c >>> index 2ca5eecebacf..1263fe07170a 100644 >>> --- a/net/core/filter.c >>> +++ b/net/core/filter.c >>> @@ -5573,6 +5573,8 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, >>> skb, >>> struct net_device *dev; >>> dev = dev_get_by_index_rcu(net, params->ifindex); >>> + if (unlikely(!dev)) >>> + return -EINVAL; >>> if (!is_skb_forwardable(dev, skb)) >>> rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; > > rcu lock is held right? It is impossible for dev to return NULL here. Yes, we're under RCU read side. Was wondering whether we could unlink it from the list but not yet free it, but also that seems not possible since we'd first need to close it which already has a synchronize_net(). So not an issue what Kaixu describes in the commit msg, agree.
diff --git a/net/core/filter.c b/net/core/filter.c index 2ca5eecebacf..1263fe07170a 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -5573,6 +5573,8 @@ BPF_CALL_4(bpf_skb_fib_lookup, struct sk_buff *, skb, struct net_device *dev; dev = dev_get_by_index_rcu(net, params->ifindex); + if (unlikely(!dev)) + return -EINVAL; if (!is_skb_forwardable(dev, skb)) rc = BPF_FIB_LKUP_RET_FRAG_NEEDED; }