From: Kaixu Xia <[email protected]>
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 <[email protected]>
---
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
[ +David ]
On 11/19/20 8:04 AM, [email protected] wrote:
> From: Kaixu Xia <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
---
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
On 11/20/20 8:13 AM, Daniel Borkmann wrote:
> [ +David ]
>
> On 11/19/20 8:04 AM, [email protected] wrote:
>> From: Kaixu Xia <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> ---
> 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, [email protected] wrote:
>>> From: Kaixu Xia <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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.