* [PATCH net 1/2] net: ipv4: Set skb->dev for output route resolution
2018-12-20 17:03 [PATCH net 0/2] net: ipv4: Prevent user triggerable warning Ido Schimmel
@ 2018-12-20 17:03 ` Ido Schimmel
2018-12-21 1:51 ` Willem de Bruijn
2018-12-20 17:03 ` [PATCH net 2/2] selftests: rtnetlink: Add a test case for multipath route get Ido Schimmel
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Ido Schimmel @ 2018-12-20 17:03 UTC (permalink / raw)
To: netdev; +Cc: davem, willemb, dsahern, Alexander Petrovskiy, Ido Schimmel
When user requests to resolve an output route, the kernel synthesizes
an skb where the relevant parameters (e.g., source address) are set. The
skb is then passed to ip_route_output_key_hash_rcu() which might call
into the flow dissector in case a multipath route was hit and a nexthop
needs to be selected based on the multipath hash.
Since both 'skb->dev' and 'skb->sk' are not set, a warning is triggered
in the flow dissector [1]. The warning is there to prevent codepaths
from silently falling back to the standard flow dissector instead of the
BPF one.
Therefore, instead of removing the warning, set 'skb->dev' to the
loopback device, as its not used for anything but resolving the correct
namespace.
[1]
WARNING: CPU: 1 PID: 24819 at net/core/flow_dissector.c:764 __skb_flow_dissect+0x314/0x16b0
...
RSP: 0018:ffffa0df41fdf650 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff8bcded232000 RCX: 0000000000000000
RDX: ffffa0df41fdf7e0 RSI: ffffffff98e415a0 RDI: ffff8bcded232000
RBP: ffffa0df41fdf760 R08: 0000000000000000 R09: 0000000000000000
R10: ffffa0df41fdf7e8 R11: ffff8bcdf27a3000 R12: ffffffff98e415a0
R13: ffffa0df41fdf7e0 R14: ffffffff98dd2980 R15: ffffa0df41fdf7e0
FS: 00007f46f6897680(0000) GS:ffff8bcdf7a80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055933e95f9a0 CR3: 000000021e636000 CR4: 00000000001006e0
Call Trace:
fib_multipath_hash+0x28c/0x2d0
? fib_multipath_hash+0x28c/0x2d0
fib_select_path+0x241/0x32f
? __fib_lookup+0x6a/0xb0
ip_route_output_key_hash_rcu+0x650/0xa30
? __alloc_skb+0x9b/0x1d0
inet_rtm_getroute+0x3f7/0xb80
? __alloc_pages_nodemask+0x11c/0x2c0
rtnetlink_rcv_msg+0x1d9/0x2f0
? rtnl_calcit.isra.24+0x120/0x120
netlink_rcv_skb+0x54/0x130
rtnetlink_rcv+0x15/0x20
netlink_unicast+0x20a/0x2c0
netlink_sendmsg+0x2d1/0x3d0
sock_sendmsg+0x39/0x50
___sys_sendmsg+0x2a0/0x2f0
? filemap_map_pages+0x16b/0x360
? __handle_mm_fault+0x108e/0x13d0
__sys_sendmsg+0x63/0xa0
? __sys_sendmsg+0x63/0xa0
__x64_sys_sendmsg+0x1f/0x30
do_syscall_64+0x5a/0x120
entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: d0e13a1488ad ("flow_dissector: lookup netns by skb->sk if skb->dev is NULL")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
net/ipv4/route.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index c0a9d26c06ce..4f0c1a5942d0 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2849,6 +2849,7 @@ static int inet_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
err = -rt->dst.error;
} else {
fl4.flowi4_iif = LOOPBACK_IFINDEX;
+ skb->dev = net->loopback_dev;
rt = ip_route_output_key_hash_rcu(net, &fl4, &res, skb);
err = 0;
if (IS_ERR(rt))
--
2.20.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 1/2] net: ipv4: Set skb->dev for output route resolution
2018-12-20 17:03 ` [PATCH net 1/2] net: ipv4: Set skb->dev for output route resolution Ido Schimmel
@ 2018-12-21 1:51 ` Willem de Bruijn
0 siblings, 0 replies; 6+ messages in thread
From: Willem de Bruijn @ 2018-12-21 1:51 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, davem, willemb, dsahern, Alexander Petrovskiy
On Thu, Dec 20, 2018 at 6:39 PM Ido Schimmel <idosch@mellanox.com> wrote:
>
> When user requests to resolve an output route, the kernel synthesizes
> an skb where the relevant parameters (e.g., source address) are set. The
> skb is then passed to ip_route_output_key_hash_rcu() which might call
> into the flow dissector in case a multipath route was hit and a nexthop
> needs to be selected based on the multipath hash.
>
> Since both 'skb->dev' and 'skb->sk' are not set, a warning is triggered
> in the flow dissector [1]. The warning is there to prevent codepaths
> from silently falling back to the standard flow dissector instead of the
> BPF one.
>
> Therefore, instead of removing the warning, set 'skb->dev' to the
> loopback device, as its not used for anything but resolving the correct
> namespace.
>
> [1]
> WARNING: CPU: 1 PID: 24819 at net/core/flow_dissector.c:764 __skb_flow_dissect+0x314/0x16b0
> ...
> RSP: 0018:ffffa0df41fdf650 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff8bcded232000 RCX: 0000000000000000
> RDX: ffffa0df41fdf7e0 RSI: ffffffff98e415a0 RDI: ffff8bcded232000
> RBP: ffffa0df41fdf760 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffa0df41fdf7e8 R11: ffff8bcdf27a3000 R12: ffffffff98e415a0
> R13: ffffa0df41fdf7e0 R14: ffffffff98dd2980 R15: ffffa0df41fdf7e0
> FS: 00007f46f6897680(0000) GS:ffff8bcdf7a80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000055933e95f9a0 CR3: 000000021e636000 CR4: 00000000001006e0
> Call Trace:
> fib_multipath_hash+0x28c/0x2d0
> ? fib_multipath_hash+0x28c/0x2d0
> fib_select_path+0x241/0x32f
> ? __fib_lookup+0x6a/0xb0
> ip_route_output_key_hash_rcu+0x650/0xa30
> ? __alloc_skb+0x9b/0x1d0
> inet_rtm_getroute+0x3f7/0xb80
> ? __alloc_pages_nodemask+0x11c/0x2c0
> rtnetlink_rcv_msg+0x1d9/0x2f0
> ? rtnl_calcit.isra.24+0x120/0x120
> netlink_rcv_skb+0x54/0x130
> rtnetlink_rcv+0x15/0x20
> netlink_unicast+0x20a/0x2c0
> netlink_sendmsg+0x2d1/0x3d0
> sock_sendmsg+0x39/0x50
> ___sys_sendmsg+0x2a0/0x2f0
> ? filemap_map_pages+0x16b/0x360
> ? __handle_mm_fault+0x108e/0x13d0
> __sys_sendmsg+0x63/0xa0
> ? __sys_sendmsg+0x63/0xa0
> __x64_sys_sendmsg+0x1f/0x30
> do_syscall_64+0x5a/0x120
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Fixes: d0e13a1488ad ("flow_dissector: lookup netns by skb->sk if skb->dev is NULL")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Thanks a lot for addressing this warning, Ido!
David even already applied it, but still a belated
Acked-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 2/2] selftests: rtnetlink: Add a test case for multipath route get
2018-12-20 17:03 [PATCH net 0/2] net: ipv4: Prevent user triggerable warning Ido Schimmel
2018-12-20 17:03 ` [PATCH net 1/2] net: ipv4: Set skb->dev for output route resolution Ido Schimmel
@ 2018-12-20 17:03 ` Ido Schimmel
2018-12-21 0:42 ` [PATCH net 0/2] net: ipv4: Prevent user triggerable warning David Miller
2018-12-21 1:27 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: Ido Schimmel @ 2018-12-20 17:03 UTC (permalink / raw)
To: netdev; +Cc: davem, willemb, dsahern, Alexander Petrovskiy, Ido Schimmel
Without previous patch a warning would be generated upon multipath route
get when FIB multipath hash policy is to use a 5-tuple for multipath
hash calculation.
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
tools/testing/selftests/net/rtnetlink.sh | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/tools/testing/selftests/net/rtnetlink.sh b/tools/testing/selftests/net/rtnetlink.sh
index e101af52d1d6..035406e5fb38 100755
--- a/tools/testing/selftests/net/rtnetlink.sh
+++ b/tools/testing/selftests/net/rtnetlink.sh
@@ -205,6 +205,8 @@ kci_test_polrouting()
kci_test_route_get()
{
+ local hash_policy=$(sysctl -n net.ipv4.fib_multipath_hash_policy)
+
ret=0
ip route get 127.0.0.1 > /dev/null
@@ -223,6 +225,19 @@ kci_test_route_get()
check_err $?
ip route get 10.23.7.11 from 10.23.7.12 iif "$devdummy" > /dev/null
check_err $?
+ ip route add 10.23.8.0/24 \
+ nexthop via 10.23.7.13 dev "$devdummy" \
+ nexthop via 10.23.7.14 dev "$devdummy"
+ check_err $?
+ sysctl -wq net.ipv4.fib_multipath_hash_policy=0
+ ip route get 10.23.8.11 > /dev/null
+ check_err $?
+ sysctl -wq net.ipv4.fib_multipath_hash_policy=1
+ ip route get 10.23.8.11 > /dev/null
+ check_err $?
+ sysctl -wq net.ipv4.fib_multipath_hash_policy="$hash_policy"
+ ip route del 10.23.8.0/24
+ check_err $?
ip addr del dev "$devdummy" 10.23.7.11/24
check_err $?
--
2.20.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net 0/2] net: ipv4: Prevent user triggerable warning
2018-12-20 17:03 [PATCH net 0/2] net: ipv4: Prevent user triggerable warning Ido Schimmel
2018-12-20 17:03 ` [PATCH net 1/2] net: ipv4: Set skb->dev for output route resolution Ido Schimmel
2018-12-20 17:03 ` [PATCH net 2/2] selftests: rtnetlink: Add a test case for multipath route get Ido Schimmel
@ 2018-12-21 0:42 ` David Miller
2018-12-21 1:27 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-12-21 0:42 UTC (permalink / raw)
To: idosch; +Cc: netdev, willemb, dsahern, alexpe
From: Ido Schimmel <idosch@mellanox.com>
Date: Thu, 20 Dec 2018 17:03:26 +0000
> Patch #1 prevents a user triaggerable warning in the flow dissector by
> setting 'skb->dev' in skbs used for IPv4 output route get requests.
>
> Patch #2 adds a test case that triggers the warning without the first
> patch.
>
> I have audited all the RTM_GETROUTE handlers and could not find any
> other callpath where an skb is passed to the flow dissector with both
> 'skb->dev' and 'skb->sk' cleared.
Series applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net 0/2] net: ipv4: Prevent user triggerable warning
2018-12-20 17:03 [PATCH net 0/2] net: ipv4: Prevent user triggerable warning Ido Schimmel
` (2 preceding siblings ...)
2018-12-21 0:42 ` [PATCH net 0/2] net: ipv4: Prevent user triggerable warning David Miller
@ 2018-12-21 1:27 ` David Miller
3 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2018-12-21 1:27 UTC (permalink / raw)
To: idosch; +Cc: netdev, willemb, dsahern, alexpe
From: Ido Schimmel <idosch@mellanox.com>
Date: Thu, 20 Dec 2018 17:03:26 +0000
> Patch #1 prevents a user triaggerable warning in the flow dissector by
> setting 'skb->dev' in skbs used for IPv4 output route get requests.
>
> Patch #2 adds a test case that triggers the warning without the first
> patch.
>
> I have audited all the RTM_GETROUTE handlers and could not find any
> other callpath where an skb is passed to the flow dissector with both
> 'skb->dev' and 'skb->sk' cleared.
I'll apply this, thanks Ido.
^ permalink raw reply [flat|nested] 6+ messages in thread