* [PATCH] bpf: Check the return value of dev_get_by_index_rcu() @ 2020-11-19 7:04 xiakaixu1987 2020-11-20 15:13 ` Daniel Borkmann 0 siblings, 1 reply; 4+ messages in thread From: xiakaixu1987 @ 2020-11-19 7:04 UTC (permalink / raw) To: ast, daniel, kafai, songliubraving, yhs, andrii, john.fastabend, kpsingh Cc: netdev, bpf, linux-kernel, Kaixu Xia 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; } -- 2.20.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] bpf: Check the return value of dev_get_by_index_rcu() 2020-11-19 7:04 [PATCH] bpf: Check the return value of dev_get_by_index_rcu() xiakaixu1987 @ 2020-11-20 15:13 ` Daniel Borkmann 2020-11-20 15:19 ` David Ahern 0 siblings, 1 reply; 4+ messages in thread From: Daniel Borkmann @ 2020-11-20 15:13 UTC (permalink / raw) To: xiakaixu1987, ast, kafai, songliubraving, yhs, andrii, john.fastabend, kpsingh Cc: netdev, bpf, linux-kernel, Kaixu Xia, dsahern [ +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 = { -- 2.21.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] bpf: Check the return value of dev_get_by_index_rcu() 2020-11-20 15:13 ` Daniel Borkmann @ 2020-11-20 15:19 ` David Ahern 2020-11-20 16:01 ` Daniel Borkmann 0 siblings, 1 reply; 4+ messages in thread From: David Ahern @ 2020-11-20 15:19 UTC (permalink / raw) To: Daniel Borkmann, xiakaixu1987, ast, kafai, songliubraving, yhs, andrii, john.fastabend, kpsingh Cc: netdev, bpf, linux-kernel, Kaixu Xia 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 = { ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] bpf: Check the return value of dev_get_by_index_rcu() 2020-11-20 15:19 ` David Ahern @ 2020-11-20 16:01 ` Daniel Borkmann 0 siblings, 0 replies; 4+ messages in thread From: Daniel Borkmann @ 2020-11-20 16:01 UTC (permalink / raw) To: David Ahern, xiakaixu1987, ast, kafai, songliubraving, yhs, andrii, john.fastabend, kpsingh Cc: netdev, bpf, linux-kernel, Kaixu Xia 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-20 16:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-19 7:04 [PATCH] bpf: Check the return value of dev_get_by_index_rcu() xiakaixu1987 2020-11-20 15:13 ` Daniel Borkmann 2020-11-20 15:19 ` David Ahern 2020-11-20 16:01 ` Daniel Borkmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).