* [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).