[PATCH net 0/3] netdevsim: fib: Fix reference count leak on route deletion failure

[PATCH net 0/3] netdevsim: fib: Fix reference count leak on route deletion failure

[Ido Schimmel]

Fix a recently reported netdevsim bug found using syzkaller.

Patch #1 fixes the bug.

Patch #2 adds a debugfs knob to allow us to test the fix.

Patch #3 adds test cases.

Ido Schimmel (3):
  netdevsim: fib: Fix reference count leak on route deletion failure
  netdevsim: fib: Add debugfs knob to simulate route deletion failure
  selftests: netdevsim: Add test cases for route deletion failure

 drivers/net/netdevsim/fib.c                   | 41 ++++++++++++++++-
 .../selftests/drivers/net/netdevsim/fib.sh    | 45 +++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

-- 
2.36.1

[PATCH net 1/3] netdevsim: fib: Fix reference count leak on route deletion failure

[Ido Schimmel]

As part of FIB offload simulation, netdevsim stores IPv4 and IPv6 routes
and holds a reference on FIB info structures that in turn hold a
reference on the associated nexthop device(s).

In the unlikely case where we are unable to allocate memory to process a
route deletion request, netdevsim will not release the reference from
the associated FIB info structure, thereby preventing the associated
nexthop device(s) from ever being removed [1].

Fix this by scheduling a work item that will flush netdevsim's FIB table
upon route deletion failure. This will cause netdevsim to release its
reference from all the FIB info structures in its table.

Reported by Lucas Leong of Trend Micro Zero Day Initiative.

Fixes: 0ae3eb7b4611 ("netdevsim: fib: Perform the route programming in a non-atomic context")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Amit Cohen <amcohen@nvidia.com>
---
No "Reported-by" tag since I do not have the mail address of the
reporter.
---
 drivers/net/netdevsim/fib.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
index c8f398f5bc5b..57371c697d5c 100644
--- a/drivers/net/netdevsim/fib.c
+++ b/drivers/net/netdevsim/fib.c
@@ -54,6 +54,7 @@ struct nsim_fib_data {
 	struct rhashtable nexthop_ht;
 	struct devlink *devlink;
 	struct work_struct fib_event_work;
+	struct work_struct fib_flush_work;
 	struct list_head fib_event_queue;
 	spinlock_t fib_event_queue_lock; /* Protects fib event queue list */
 	struct mutex nh_lock; /* Protects NH HT */
@@ -978,7 +979,7 @@ static int nsim_fib_event_schedule_work(struct nsim_fib_data *data,
 
 	fib_event = kzalloc(sizeof(*fib_event), GFP_ATOMIC);
 	if (!fib_event)
-		return NOTIFY_BAD;
+		goto err_fib_event_alloc;
 
 	fib_event->data = data;
 	fib_event->event = event;
@@ -1006,6 +1007,9 @@ static int nsim_fib_event_schedule_work(struct nsim_fib_data *data,
 
 err_fib_prepare_event:
 	kfree(fib_event);
+err_fib_event_alloc:
+	if (event == FIB_EVENT_ENTRY_DEL)
+		schedule_work(&data->fib_flush_work);
 	return NOTIFY_BAD;
 }
 
@@ -1483,6 +1487,24 @@ static void nsim_fib_event_work(struct work_struct *work)
 	mutex_unlock(&data->fib_lock);
 }
 
+static void nsim_fib_flush_work(struct work_struct *work)
+{
+	struct nsim_fib_data *data = container_of(work, struct nsim_fib_data,
+						  fib_flush_work);
+	struct nsim_fib_rt *fib_rt, *fib_rt_tmp;
+
+	/* Process pending work. */
+	flush_work(&data->fib_event_work);
+
+	mutex_lock(&data->fib_lock);
+	list_for_each_entry_safe(fib_rt, fib_rt_tmp, &data->fib_rt_list, list) {
+		rhashtable_remove_fast(&data->fib_rt_ht, &fib_rt->ht_node,
+				       nsim_fib_rt_ht_params);
+		nsim_fib_rt_free(fib_rt, data);
+	}
+	mutex_unlock(&data->fib_lock);
+}
+
 static int
 nsim_fib_debugfs_init(struct nsim_fib_data *data, struct nsim_dev *nsim_dev)
 {
@@ -1541,6 +1563,7 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
 		goto err_rhashtable_nexthop_destroy;
 
 	INIT_WORK(&data->fib_event_work, nsim_fib_event_work);
+	INIT_WORK(&data->fib_flush_work, nsim_fib_flush_work);
 	INIT_LIST_HEAD(&data->fib_event_queue);
 	spin_lock_init(&data->fib_event_queue_lock);
 
@@ -1587,6 +1610,7 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
 err_nexthop_nb_unregister:
 	unregister_nexthop_notifier(devlink_net(devlink), &data->nexthop_nb);
 err_rhashtable_fib_destroy:
+	cancel_work_sync(&data->fib_flush_work);
 	flush_work(&data->fib_event_work);
 	rhashtable_free_and_destroy(&data->fib_rt_ht, nsim_fib_rt_free,
 				    data);
@@ -1616,6 +1640,7 @@ void nsim_fib_destroy(struct devlink *devlink, struct nsim_fib_data *data)
 					    NSIM_RESOURCE_IPV4_FIB);
 	unregister_fib_notifier(devlink_net(devlink), &data->fib_nb);
 	unregister_nexthop_notifier(devlink_net(devlink), &data->nexthop_nb);
+	cancel_work_sync(&data->fib_flush_work);
 	flush_work(&data->fib_event_work);
 	rhashtable_free_and_destroy(&data->fib_rt_ht, nsim_fib_rt_free,
 				    data);
-- 
2.36.1

[PATCH net 2/3] netdevsim: fib: Add debugfs knob to simulate route deletion failure

[Ido Schimmel]

The previous patch ("netdevsim: fib: Fix reference count leak on route
deletion failure") fixed a reference count leak that happens on route
deletion failure.

Such failures can only be simulated by injecting slab allocation
failures, which cannot be surgically injected.

In order to be able to specifically test this scenario, add a debugfs
knob that allows user space to fail route deletion requests when
enabled.

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Amit Cohen <amcohen@nvidia.com>
---
 drivers/net/netdevsim/fib.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
index 57371c697d5c..38a1fde8d886 100644
--- a/drivers/net/netdevsim/fib.c
+++ b/drivers/net/netdevsim/fib.c
@@ -62,6 +62,7 @@ struct nsim_fib_data {
 	bool fail_route_offload;
 	bool fail_res_nexthop_group_replace;
 	bool fail_nexthop_bucket_replace;
+	bool fail_route_delete;
 };
 
 struct nsim_fib_rt_key {
@@ -915,6 +916,10 @@ static int nsim_fib4_prepare_event(struct fib_notifier_info *info,
 		}
 		break;
 	case FIB_EVENT_ENTRY_DEL:
+		if (data->fail_route_delete) {
+			NL_SET_ERR_MSG_MOD(extack, "Failed to process route deletion");
+			return -EINVAL;
+		}
 		nsim_fib_account(&data->ipv4.fib, false);
 		break;
 	}
@@ -953,6 +958,11 @@ static int nsim_fib6_prepare_event(struct fib_notifier_info *info,
 		}
 		break;
 	case FIB_EVENT_ENTRY_DEL:
+		if (data->fail_route_delete) {
+			err = -EINVAL;
+			NL_SET_ERR_MSG_MOD(extack, "Failed to process route deletion");
+			goto err_fib6_event_fini;
+		}
 		nsim_fib_account(&data->ipv6.fib, false);
 		break;
 	}
@@ -1526,6 +1536,10 @@ nsim_fib_debugfs_init(struct nsim_fib_data *data, struct nsim_dev *nsim_dev)
 
 	debugfs_create_file("nexthop_bucket_activity", 0200, data->ddir,
 			    data, &nsim_nexthop_bucket_activity_fops);
+
+	data->fail_route_delete = false;
+	debugfs_create_bool("fail_route_delete", 0600, data->ddir,
+			    &data->fail_route_delete);
 	return 0;
 }
 
-- 
2.36.1

[PATCH net 3/3] selftests: netdevsim: Add test cases for route deletion failure

[Ido Schimmel]

Add IPv4 and IPv6 test cases that ensure that we are not leaking a
reference on the nexthop device when we are unable to delete its
associated route.

Without the fix in a previous patch ("netdevsim: fib: Fix reference
count leak on route deletion failure") both test cases get stuck,
waiting for the reference to be released from the dummy device [1][2].

[1]
unregister_netdevice: waiting for dummy1 to become free. Usage count = 5
leaked reference.
 fib_check_nh+0x275/0x620
 fib_create_info+0x237c/0x4d30
 fib_table_insert+0x1dd/0x1d20
 inet_rtm_newroute+0x11b/0x200
 rtnetlink_rcv_msg+0x43b/0xd20
 netlink_rcv_skb+0x15e/0x430
 netlink_unicast+0x53b/0x800
 netlink_sendmsg+0x945/0xe40
 ____sys_sendmsg+0x747/0x960
 ___sys_sendmsg+0x11d/0x190
 __sys_sendmsg+0x118/0x1e0
 do_syscall_64+0x34/0x80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

[2]
unregister_netdevice: waiting for dummy1 to become free. Usage count = 5
leaked reference.
 fib6_nh_init+0xc46/0x1ca0
 ip6_route_info_create+0x1167/0x19a0
 ip6_route_add+0x27/0x150
 inet6_rtm_newroute+0x161/0x170
 rtnetlink_rcv_msg+0x43b/0xd20
 netlink_rcv_skb+0x15e/0x430
 netlink_unicast+0x53b/0x800
 netlink_sendmsg+0x945/0xe40
 ____sys_sendmsg+0x747/0x960
 ___sys_sendmsg+0x11d/0x190
 __sys_sendmsg+0x118/0x1e0
 do_syscall_64+0x34/0x80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Reviewed-by: Amit Cohen <amcohen@nvidia.com>
---
 .../selftests/drivers/net/netdevsim/fib.sh    | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/netdevsim/fib.sh b/tools/testing/selftests/drivers/net/netdevsim/fib.sh
index fc794cd30389..6800de816e8b 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/fib.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/fib.sh
@@ -16,6 +16,7 @@ ALL_TESTS="
 	ipv4_replay
 	ipv4_flush
 	ipv4_error_path
+	ipv4_delete_fail
 	ipv6_add
 	ipv6_metric
 	ipv6_append_single
@@ -29,11 +30,13 @@ ALL_TESTS="
 	ipv6_replay_single
 	ipv6_replay_multipath
 	ipv6_error_path
+	ipv6_delete_fail
 "
 NETDEVSIM_PATH=/sys/bus/netdevsim/
 DEV_ADDR=1337
 DEV=netdevsim${DEV_ADDR}
 SYSFS_NET_DIR=/sys/bus/netdevsim/devices/$DEV/net/
+DEBUGFS_DIR=/sys/kernel/debug/netdevsim/$DEV/
 NUM_NETIFS=0
 source $lib_dir/lib.sh
 source $lib_dir/fib_offload_lib.sh
@@ -157,6 +160,27 @@ ipv4_error_path()
 	ipv4_error_path_replay
 }
 
+ipv4_delete_fail()
+{
+	RET=0
+
+	echo "y" > $DEBUGFS_DIR/fib/fail_route_delete
+
+	ip -n testns1 link add name dummy1 type dummy
+	ip -n testns1 link set dev dummy1 up
+
+	ip -n testns1 route add 192.0.2.0/24 dev dummy1
+	ip -n testns1 route del 192.0.2.0/24 dev dummy1 &> /dev/null
+
+	# We should not be able to delete the netdev if we are leaking a
+	# reference.
+	ip -n testns1 link del dev dummy1
+
+	log_test "IPv4 route delete failure"
+
+	echo "n" > $DEBUGFS_DIR/fib/fail_route_delete
+}
+
 ipv6_add()
 {
 	fib_ipv6_add_test "testns1"
@@ -304,6 +328,27 @@ ipv6_error_path()
 	ipv6_error_path_replay
 }
 
+ipv6_delete_fail()
+{
+	RET=0
+
+	echo "y" > $DEBUGFS_DIR/fib/fail_route_delete
+
+	ip -n testns1 link add name dummy1 type dummy
+	ip -n testns1 link set dev dummy1 up
+
+	ip -n testns1 route add 2001:db8:1::/64 dev dummy1
+	ip -n testns1 route del 2001:db8:1::/64 dev dummy1 &> /dev/null
+
+	# We should not be able to delete the netdev if we are leaking a
+	# reference.
+	ip -n testns1 link del dev dummy1
+
+	log_test "IPv6 route delete failure"
+
+	echo "n" > $DEBUGFS_DIR/fib/fail_route_delete
+}
+
 fib_notify_on_flag_change_set()
 {
 	local notify=$1; shift
-- 
2.36.1

Re: [PATCH net 2/3] netdevsim: fib: Add debugfs knob to simulate route deletion failure

[David Ahern]

On 7/28/22 5:45 AM, Ido Schimmel wrote:
> The previous patch ("netdevsim: fib: Fix reference count leak on route
> deletion failure") fixed a reference count leak that happens on route
> deletion failure.
> 
> Such failures can only be simulated by injecting slab allocation
> failures, which cannot be surgically injected.

One option is CONFIG_FAULT_INJECTION, labeling functions with
ALLOW_ERROR_INJECTION and writing tests to set the fail_function. Been
very convenient for testing cleanup paths.

That said, I am not against this option.
> 
> In order to be able to specifically test this scenario, add a debugfs
> knob that allows user space to fail route deletion requests when
> enabled.
> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Amit Cohen <amcohen@nvidia.com>
> ---
>  drivers/net/netdevsim/fib.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 3/3] selftests: netdevsim: Add test cases for route deletion failure

[David Ahern]

On 7/28/22 5:45 AM, Ido Schimmel wrote:
> Add IPv4 and IPv6 test cases that ensure that we are not leaking a
> reference on the nexthop device when we are unable to delete its
> associated route.
> 
> Without the fix in a previous patch ("netdevsim: fib: Fix reference
> count leak on route deletion failure") both test cases get stuck,
> waiting for the reference to be released from the dummy device [1][2].
> 
...

> 
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Amit Cohen <amcohen@nvidia.com>
> ---
>  .../selftests/drivers/net/netdevsim/fib.sh    | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 1/3] netdevsim: fib: Fix reference count leak on route deletion failure

[David Ahern]

On 7/28/22 5:45 AM, Ido Schimmel wrote:
> As part of FIB offload simulation, netdevsim stores IPv4 and IPv6 routes
> and holds a reference on FIB info structures that in turn hold a
> reference on the associated nexthop device(s).
> 
> In the unlikely case where we are unable to allocate memory to process a
> route deletion request, netdevsim will not release the reference from
> the associated FIB info structure, thereby preventing the associated
> nexthop device(s) from ever being removed [1].
> 
> Fix this by scheduling a work item that will flush netdevsim's FIB table
> upon route deletion failure. This will cause netdevsim to release its
> reference from all the FIB info structures in its table.
> 
> Reported by Lucas Leong of Trend Micro Zero Day Initiative.
> 
> Fixes: 0ae3eb7b4611 ("netdevsim: fib: Perform the route programming in a non-atomic context")
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> Reviewed-by: Amit Cohen <amcohen@nvidia.com>
> ---
> No "Reported-by" tag since I do not have the mail address of the
> reporter.
> ---
>  drivers/net/netdevsim/fib.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net 0/3] netdevsim: fib: Fix reference count leak on route deletion failure

[Jakub Kicinski]

On Thu, 28 Jul 2022 14:45:32 +0300 Ido Schimmel wrote:
> Fix a recently reported netdevsim bug found using syzkaller.
> 
> Patch #1 fixes the bug.
> 
> Patch #2 adds a debugfs knob to allow us to test the fix.
> 
> Patch #3 adds test cases.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net 0/3] netdevsim: fib: Fix reference count leak on route deletion failure

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 28 Jul 2022 14:45:32 +0300 you wrote:
> Fix a recently reported netdevsim bug found using syzkaller.
> 
> Patch #1 fixes the bug.
> 
> Patch #2 adds a debugfs knob to allow us to test the fix.
> 
> Patch #3 adds test cases.
> 
> [...]

Here is the summary with links:
  - [net,1/3] netdevsim: fib: Fix reference count leak on route deletion failure
    https://git.kernel.org/netdev/net/c/180a6a3ee60a
  - [net,2/3] netdevsim: fib: Add debugfs knob to simulate route deletion failure
    https://git.kernel.org/netdev/net/c/974be75f2503
  - [net,3/3] selftests: netdevsim: Add test cases for route deletion failure
    https://git.kernel.org/netdev/net/c/40823f3ee05f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html