netfilter: IDLETIMER: Fix for possible ABBA deadlock

Deletion of the last rule referencing a given idletimer may happen at
the same time as a read of its file in sysfs:

| ======================================================
| WARNING: possible circular locking dependency detected
| 6.12.0-rc7-01692-g5e9a28f41134-dirty #594 Not tainted
| ------------------------------------------------------
| iptables/3303 is trying to acquire lock:
| ffff8881057e04b8 (kn->active#48){++++}-{0:0}, at: __kernfs_remove+0x20
|
| but task is already holding lock:
| ffffffffa0249068 (list_mutex){+.+.}-{3:3}, at: idletimer_tg_destroy_v]
|
| which lock already depends on the new lock.

A simple reproducer is:

| #!/bin/bash
|
| while true; do
|         iptables -A INPUT -i foo -j IDLETIMER --timeout 10 --label "testme"
|         iptables -D INPUT -i foo -j IDLETIMER --timeout 10 --label "testme"
| done &
| while true; do
|         cat /sys/class/xt_idletimer/timers/testme >/dev/null
| done

Avoid this by freeing list_mutex right after deleting the element from
the list, then continuing with the teardown.

Fixes: 0902b469bd25 ("netfilter: xtables: idletimer target implementation")
Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
index 85f017e37cfcc085bc05594125a3b2c2380e0363..9f54819eb52ca28a6ef6544249e864f17ca3de7a 100644 (file)
--- a/net/netfilter/xt_IDLETIMER.c
+++ b/net/netfilter/xt_IDLETIMER.c
@@ -407,21 +407,23 @@ static void idletimer_tg_destroy(const struct xt_tgdtor_param *par)
 
        mutex_lock(&list_mutex);
 
-       if (--info->timer->refcnt == 0) {
-               pr_debug("deleting timer %s\n", info->label);
-
-               list_del(&info->timer->entry);
-               timer_shutdown_sync(&info->timer->timer);
-               cancel_work_sync(&info->timer->work);
-               sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);
-               kfree(info->timer->attr.attr.name);
-               kfree(info->timer);
-       } else {
+       if (--info->timer->refcnt > 0) {
                pr_debug("decreased refcnt of timer %s to %u\n",
                         info->label, info->timer->refcnt);
+               mutex_unlock(&list_mutex);
+               return;
        }
 
+       pr_debug("deleting timer %s\n", info->label);
+
+       list_del(&info->timer->entry);
        mutex_unlock(&list_mutex);
+
+       timer_shutdown_sync(&info->timer->timer);
+       cancel_work_sync(&info->timer->work);
+       sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);
+       kfree(info->timer->attr.attr.name);
+       kfree(info->timer);
 }
 
 static void idletimer_tg_destroy_v1(const struct xt_tgdtor_param *par)
@@ -432,25 +434,27 @@ static void idletimer_tg_destroy_v1(const struct xt_tgdtor_param *par)
 
        mutex_lock(&list_mutex);
 
-       if (--info->timer->refcnt == 0) {
-               pr_debug("deleting timer %s\n", info->label);
-
-               list_del(&info->timer->entry);
-               if (info->timer->timer_type & XT_IDLETIMER_ALARM) {
-                       alarm_cancel(&info->timer->alarm);
-               } else {
-                       timer_shutdown_sync(&info->timer->timer);
-               }
-               cancel_work_sync(&info->timer->work);
-               sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);
-               kfree(info->timer->attr.attr.name);
-               kfree(info->timer);
-       } else {
+       if (--info->timer->refcnt > 0) {
                pr_debug("decreased refcnt of timer %s to %u\n",
                         info->label, info->timer->refcnt);
+               mutex_unlock(&list_mutex);
+               return;
        }
 
+       pr_debug("deleting timer %s\n", info->label);
+
+       list_del(&info->timer->entry);
        mutex_unlock(&list_mutex);
+
+       if (info->timer->timer_type & XT_IDLETIMER_ALARM) {
+               alarm_cancel(&info->timer->alarm);
+       } else {
+               timer_shutdown_sync(&info->timer->timer);
+       }
+       cancel_work_sync(&info->timer->work);
+       sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);
+       kfree(info->timer->attr.attr.name);
+       kfree(info->timer);
 }