diff mbox series

[nf,1/1] netfilter: xt_IDLETIMER: scope timer reuse to the owning netns

Message ID 9c5661fad291777d8e998e23f3cb27cac37aa607.1775353240.git.royenheart@gmail.com
State Rejected, archived
Headers show
Series [nf,1/1] netfilter: xt_IDLETIMER: scope timer reuse to the owning netns | expand

Commit Message

Ren Wei May 14, 2026, 4:05 a.m. UTC 
From: Haoze Xie <royenheart@gmail.com>

IDLETIMER keeps timers in a module-global list and reuses them
solely by label text.

The existing rev0 ALARM guard avoids the panic when rev0 reuses
a rev1 ALARM timer from another netns, but it still lets same
labels in different netns share the same timer object and the
same sysfs entry.

Track the owning netns in struct idletimer_tg and only reuse
timers when both the label and netns match. For non-init_net
timers, derive a namespace-scoped sysfs name from the netns
inode so non-init namespaces no longer collide in the global
xt_idletimer sysfs directory.

This keeps init_net sysfs paths unchanged for ABI compatibility
and preserves same-netns label reuse, while preventing the
cross-netns timer-object aliasing that caused refcount, expiry,
and teardown interference.

Fixes: 0902b469bd25 ("netfilter: xtables: idletimer target implementation")
Cc: stable@kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Tested-by: Haoze Xie <royenheart@gmail.com>
Signed-off-by: Haoze Xie <royenheart@gmail.com>
Signed-off-by: Ren Wei <n05ec@lzu.edu.cn>
---
 net/netfilter/xt_IDLETIMER.c | 74 ++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 25 deletions(-)

Comments

Florian Westphal May 14, 2026, 10:17 a.m. UTC | #1 
Ren Wei <n05ec@lzu.edu.cn> wrote:
> From: Haoze Xie <royenheart@gmail.com>
> 
> IDLETIMER keeps timers in a module-global list and reuses them
> solely by label text.
> 
> The existing rev0 ALARM guard avoids the panic when rev0 reuses
> a rev1 ALARM timer from another netns, but it still lets same
> labels in different netns share the same timer object and the
> same sysfs entry.

Isn't that by design?

> Track the owning netns in struct idletimer_tg and only reuse
> timers when both the label and netns match. For non-init_net
> timers, derive a namespace-scoped sysfs name from the netns
> inode so non-init namespaces no longer collide in the global
> xt_idletimer sysfs directory.

How can that work?  How would userspace daemon relize that the
name has changed?

> This keeps init_net sysfs paths unchanged for ABI compatibility
> and preserves same-netns label reuse, while preventing the
> cross-netns timer-object aliasing that caused refcount, expiry,
> and teardown interference.

I don't think there is a bug here.  Two netns using same
files having same sysfs mount should naturally "conflict".

Maybe one could make a patch to force-detach an idletime
in a non-init userns if init userns asks for "foo" that
is already claimed by different userns (to avoid the "Dos"
angle).

But I'm not sure its worth it.
Haoze Xie May 15, 2026, 1:54 a.m. UTC | #2 
On 5/14/2026 6:17 PM, Florian Westphal wrote:
> Ren Wei <n05ec@lzu.edu.cn> wrote:
>> From: Haoze Xie <royenheart@gmail.com>
>>
>> IDLETIMER keeps timers in a module-global list and reuses them
>> solely by label text.
>>
>> The existing rev0 ALARM guard avoids the panic when rev0 reuses
>> a rev1 ALARM timer from another netns, but it still lets same
>> labels in different netns share the same timer object and the
>> same sysfs entry.
> 
> Isn't that by design?

My patch was based on the premise here: I treated this as a
namespace-isolation bug and tried to enforce per-netns label ownership,
but that is not how xt_IDLETIMER is defined today.

> 
>> Track the owning netns in struct idletimer_tg and only reuse
>> timers when both the label and netns match. For non-init_net
>> timers, derive a namespace-scoped sysfs name from the netns
>> inode so non-init namespaces no longer collide in the global
>> xt_idletimer sysfs directory.
> 
> How can that work?  How would userspace daemon relize that the
> name has changed?

My proposed sysfs renaming for non-init_net users would introduce
userspace-visible semantic changes, and I did not justify how existing
userspace would discover or adapt to the renamed entries.

> 
>> This keeps init_net sysfs paths unchanged for ABI compatibility
>> and preserves same-netns label reuse, while preventing the
>> cross-netns timer-object aliasing that caused refcount, expiry,
>> and teardown interference.
> 
> I don't think there is a bug here.  Two netns using same
> files having same sysfs mount should naturally "conflict".
> 
> Maybe one could make a patch to force-detach an idletime
> in a non-init userns if init userns asks for "foo" that
> is already claimed by different userns (to avoid the "Dos"
> angle).
> 
> But I'm not sure its worth it.

Thanks for the suggestion. We may experiment it later, but for now we
decided to scratch this patch since it didn't reproduces the more
severe behavior.
diff mbox series

Patch

diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
index 517106165ad2..c45af0cecb52 100644
--- a/net/netfilter/xt_IDLETIMER.c
+++ b/net/netfilter/xt_IDLETIMER.c
@@ -28,6 +28,7 @@ 
 #include <linux/kobject.h>
 #include <linux/workqueue.h>
 #include <linux/sysfs.h>
+#include <net/net_namespace.h>
 
 struct idletimer_tg {
 	struct list_head entry;
@@ -37,6 +38,8 @@  struct idletimer_tg {
 
 	struct kobject *kobj;
 	struct device_attribute attr;
+	struct net *net;
+	char label[MAX_IDLETIMER_LABEL_SIZE];
 
 	unsigned int refcnt;
 	u8 timer_type;
@@ -48,38 +51,46 @@  static DEFINE_MUTEX(list_mutex);
 static struct kobject *idletimer_tg_kobj;
 
 static
-struct idletimer_tg *__idletimer_tg_find_by_label(const char *label)
+struct idletimer_tg *__idletimer_tg_find_by_label(const struct net *net,
+						  const char *label)
 {
 	struct idletimer_tg *entry;
 
 	list_for_each_entry(entry, &idletimer_tg_list, entry) {
-		if (!strcmp(label, entry->attr.attr.name))
+		if (net_eq(entry->net, net) && !strcmp(label, entry->label))
 			return entry;
 	}
 
 	return NULL;
 }
 
+static char *idletimer_tg_sysfs_name(struct net *net, const char *label)
+{
+	if (net_eq(net, &init_net))
+		return kstrdup(label, GFP_KERNEL);
+
+	return kasprintf(GFP_KERNEL, "%u_%s", net->ns.inum, label);
+}
+
 static ssize_t idletimer_tg_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
-	struct idletimer_tg *timer;
+	struct idletimer_tg *timer = container_of(attr, struct idletimer_tg,
+						  attr);
 	unsigned long expires = 0;
 	struct timespec64 ktimespec = {};
 	long time_diff = 0;
 
 	mutex_lock(&list_mutex);
 
-	timer =	__idletimer_tg_find_by_label(attr->attr.name);
-	if (timer) {
-		if (timer->timer_type & XT_IDLETIMER_ALARM) {
-			ktime_t expires_alarm = alarm_expires_remaining(&timer->alarm);
-			ktimespec = ktime_to_timespec64(expires_alarm);
-			time_diff = ktimespec.tv_sec;
-		} else {
-			expires = timer->timer.expires;
-			time_diff = jiffies_to_msecs(expires - jiffies) / 1000;
-		}
+	if (timer->timer_type & XT_IDLETIMER_ALARM) {
+		ktime_t expires_alarm = alarm_expires_remaining(&timer->alarm);
+
+		ktimespec = ktime_to_timespec64(expires_alarm);
+		time_diff = ktimespec.tv_sec;
+	} else {
+		expires = timer->timer.expires;
+		time_diff = jiffies_to_msecs(expires - jiffies) / 1000;
 	}
 
 	mutex_unlock(&list_mutex);
@@ -102,7 +113,7 @@  static void idletimer_tg_expired(struct timer_list *t)
 {
 	struct idletimer_tg *timer = timer_container_of(timer, t, timer);
 
-	pr_debug("timer %s expired\n", timer->attr.attr.name);
+	pr_debug("timer %s expired\n", timer->label);
 
 	schedule_work(&timer->work);
 }
@@ -111,7 +122,7 @@  static void idletimer_tg_alarmproc(struct alarm *alarm, ktime_t now)
 {
 	struct idletimer_tg *timer = alarm->data;
 
-	pr_debug("alarm %s expired\n", timer->attr.attr.name);
+	pr_debug("alarm %s expired\n", timer->label);
 	schedule_work(&timer->work);
 }
 
@@ -131,7 +142,7 @@  static int idletimer_check_sysfs_name(const char *name, unsigned int size)
 	return 0;
 }
 
-static int idletimer_tg_create(struct idletimer_tg_info *info)
+static int idletimer_tg_create(struct idletimer_tg_info *info, struct net *net)
 {
 	int ret;
 
@@ -145,11 +156,14 @@  static int idletimer_tg_create(struct idletimer_tg_info *info)
 	if (ret < 0)
 		goto out_free_timer;
 
+	info->timer->net = get_net(net);
+	strscpy(info->timer->label, info->label, sizeof(info->timer->label));
+
 	sysfs_attr_init(&info->timer->attr.attr);
-	info->timer->attr.attr.name = kstrdup(info->label, GFP_KERNEL);
+	info->timer->attr.attr.name = idletimer_tg_sysfs_name(net, info->label);
 	if (!info->timer->attr.attr.name) {
 		ret = -ENOMEM;
-		goto out_free_timer;
+		goto out_put_net;
 	}
 	info->timer->attr.attr.mode = 0444;
 	info->timer->attr.show = idletimer_tg_show;
@@ -174,13 +188,16 @@  static int idletimer_tg_create(struct idletimer_tg_info *info)
 
 out_free_attr:
 	kfree(info->timer->attr.attr.name);
+out_put_net:
+	put_net(info->timer->net);
 out_free_timer:
 	kfree(info->timer);
 out:
 	return ret;
 }
 
-static int idletimer_tg_create_v1(struct idletimer_tg_info_v1 *info)
+static int idletimer_tg_create_v1(struct idletimer_tg_info_v1 *info,
+				  struct net *net)
 {
 	int ret;
 
@@ -194,11 +211,14 @@  static int idletimer_tg_create_v1(struct idletimer_tg_info_v1 *info)
 	if (ret < 0)
 		goto out_free_timer;
 
+	info->timer->net = get_net(net);
+	strscpy(info->timer->label, info->label, sizeof(info->timer->label));
+
 	sysfs_attr_init(&info->timer->attr.attr);
-	info->timer->attr.attr.name = kstrdup(info->label, GFP_KERNEL);
+	info->timer->attr.attr.name = idletimer_tg_sysfs_name(net, info->label);
 	if (!info->timer->attr.attr.name) {
 		ret = -ENOMEM;
-		goto out_free_timer;
+		goto out_put_net;
 	}
 	info->timer->attr.attr.mode = 0444;
 	info->timer->attr.show = idletimer_tg_show;
@@ -236,6 +256,8 @@  static int idletimer_tg_create_v1(struct idletimer_tg_info_v1 *info)
 
 out_free_attr:
 	kfree(info->timer->attr.attr.name);
+out_put_net:
+	put_net(info->timer->net);
 out_free_timer:
 	kfree(info->timer);
 out:
@@ -316,7 +338,7 @@  static int idletimer_tg_checkentry(const struct xt_tgchk_param *par)
 	}
 	mutex_lock(&list_mutex);
 
-	info->timer = __idletimer_tg_find_by_label(info->label);
+	info->timer = __idletimer_tg_find_by_label(par->net, info->label);
 	if (info->timer) {
 		if (info->timer->timer_type & XT_IDLETIMER_ALARM) {
 			pr_debug("Adding/Replacing rule with same label and different timer type is not allowed\n");
@@ -331,7 +353,7 @@  static int idletimer_tg_checkentry(const struct xt_tgchk_param *par)
 		pr_debug("increased refcnt of timer %s to %u\n",
 			 info->label, info->timer->refcnt);
 	} else {
-		ret = idletimer_tg_create(info);
+		ret = idletimer_tg_create(info, par->net);
 		if (ret < 0) {
 			pr_debug("failed to create timer\n");
 			mutex_unlock(&list_mutex);
@@ -367,7 +389,7 @@  static int idletimer_tg_checkentry_v1(const struct xt_tgchk_param *par)
 
 	mutex_lock(&list_mutex);
 
-	info->timer = __idletimer_tg_find_by_label(info->label);
+	info->timer = __idletimer_tg_find_by_label(par->net, info->label);
 	if (info->timer) {
 		if (info->timer->timer_type != info->timer_type) {
 			pr_debug("Adding/Replacing rule with same label and different timer type is not allowed\n");
@@ -393,7 +415,7 @@  static int idletimer_tg_checkentry_v1(const struct xt_tgchk_param *par)
 		pr_debug("increased refcnt of timer %s to %u\n",
 			 info->label, info->timer->refcnt);
 	} else {
-		ret = idletimer_tg_create_v1(info);
+		ret = idletimer_tg_create_v1(info, par->net);
 		if (ret < 0) {
 			pr_debug("failed to create timer\n");
 			mutex_unlock(&list_mutex);
@@ -429,6 +451,7 @@  static void idletimer_tg_destroy(const struct xt_tgdtor_param *par)
 	cancel_work_sync(&info->timer->work);
 	sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);
 	kfree(info->timer->attr.attr.name);
+	put_net(info->timer->net);
 	kfree(info->timer);
 }
 
@@ -460,6 +483,7 @@  static void idletimer_tg_destroy_v1(const struct xt_tgdtor_param *par)
 	cancel_work_sync(&info->timer->work);
 	sysfs_remove_file(idletimer_tg_kobj, &info->timer->attr.attr);
 	kfree(info->timer->attr.attr.name);
+	put_net(info->timer->net);
 	kfree(info->timer);
 }