[PATCH xtables-addons 0/8] xt_condition: per-net improvements

[PATCH xtables-addons 0/8] xt_condition: per-net improvements

[Jeremy Sowden]

The first patch bumps the minimum version to 4.16 in order to allow us
to use a useful macro and function in patches 2 & 3.  4 makes the
proc_lock mutex a per-net variable.  5 removes an obsolete write
memory-barrier.  6-8 tidy up the clean-up of matches when a namespace is
deleted.

Jeremy Sowden (8):
  build: bump minimum supported kernel version from 4.15 to 4.16.
  xt_condition: use sizeof_field macro to size variable name.
  xt_condition: use `xt_check_proc_name` to validate /proc file-name.
  xt_condition: make mutex per-net.
  xt_condition: remove `wmb` when adding new variable.
  xt_condition: use `proc_net_condition` member of `struct
    condition_net`to signal that `condition_net_exit` has been called.
  xt_condition: don't delete variables in `condition_net_exit`.
  xt_condition: simplify clean-up of variables.

 configure.ac              |  2 +-
 extensions/xt_condition.c | 54 +++++++++++++--------------------------
 2 files changed, 19 insertions(+), 37 deletions(-)

-- 
2.32.0

[PATCH xtables-addons 1/8] build: bump minimum supported kernel version from 4.15 to 4.16.

[Jeremy Sowden]

The next two commits make use of a function and macro that were
introduced in 4.16.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 76129245f5c1..9705d750750e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -61,7 +61,7 @@ if test -n "$kbuilddir"; then
 			echo "WARNING: That kernel version is not officially supported yet. Continue at own luck.";
 		elif test "$kmajor" -eq 5 -a "$kminor" -ge 0; then
 			:
-		elif test "$kmajor" -eq 4 -a "$kminor" -ge 15; then
+		elif test "$kmajor" -eq 4 -a "$kminor" -ge 16; then
 			:
 		else
 			echo "WARNING: That kernel version is not officially supported.";
-- 
2.32.0

[PATCH xtables-addons 2/8] xt_condition: use sizeof_field macro to size variable name.

[Jeremy Sowden]

4.16 introduced a macro for getting the size of a struct member, so
let's use it.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/xt_condition.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/xt_condition.c b/extensions/xt_condition.c
index 8227c5d13f1a..c2c48670c788 100644
--- a/extensions/xt_condition.c
+++ b/extensions/xt_condition.c
@@ -55,7 +55,7 @@ struct condition_variable {
 	struct proc_dir_entry *status_proc;
 	unsigned int refcount;
 	bool enabled;
-	char name[sizeof(((struct xt_condition_mtinfo *)NULL)->name)];
+	char name[sizeof_field(struct xt_condition_mtinfo, name)];
 };
 
 /* proc_lock is a user context only semaphore used for write access */
-- 
2.32.0

[PATCH xtables-addons 3/8] xt_condition: use `xt_check_proc_name` to validate /proc file-name.

[Jeremy Sowden]

4.16 introduced a standard function to do the job, so let's use it.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/xt_condition.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/extensions/xt_condition.c b/extensions/xt_condition.c
index c2c48670c788..1d9d7352f069 100644
--- a/extensions/xt_condition.c
+++ b/extensions/xt_condition.c
@@ -135,9 +135,7 @@ static int condition_mt_check(const struct xt_mtchk_param *par)
 	struct condition_net *condition_net = condition_pernet(par->net);
 
 	/* Forbid certain names */
-	if (*info->name == '\0' || *info->name == '.' ||
-	    info->name[sizeof(info->name)-1] != '\0' ||
-	    memchr(info->name, '/', sizeof(info->name)) != NULL) {
+	if (xt_check_proc_name(info->name, sizeof(info->name))) {
 		printk(KERN_INFO KBUILD_MODNAME ": name not allowed or too "
 		       "long: \"%.*s\"\n", (unsigned int)sizeof(info->name),
 		       info->name);
-- 
2.32.0

[PATCH xtables-addons 4/8] xt_condition: make mutex per-net.

[Jeremy Sowden]

The mutex protects per-net resources, so make it per-net too.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/xt_condition.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/extensions/xt_condition.c b/extensions/xt_condition.c
index 1d9d7352f069..e1672985e59b 100644
--- a/extensions/xt_condition.c
+++ b/extensions/xt_condition.c
@@ -58,11 +58,10 @@ struct condition_variable {
 	char name[sizeof_field(struct xt_condition_mtinfo, name)];
 };
 
-/* proc_lock is a user context only semaphore used for write access */
-/*           to the conditions' list.                               */
-static DEFINE_MUTEX(proc_lock);
-
 struct condition_net {
+	/* proc_lock is a user context only semaphore used for write access */
+	/*           to the conditions' list.                               */
+	struct mutex proc_lock;
 	struct list_head conditions_list;
 	struct proc_dir_entry *proc_net_condition;
 	bool after_clear;
@@ -145,11 +144,11 @@ static int condition_mt_check(const struct xt_mtchk_param *par)
 	 * Let's acquire the lock, check for the condition and add it
 	 * or increase the reference counter.
 	 */
-	mutex_lock(&proc_lock);
+	mutex_lock(&condition_net->proc_lock);
 	list_for_each_entry(var, &condition_net->conditions_list, list) {
 		if (strcmp(info->name, var->name) == 0) {
 			var->refcount++;
-			mutex_unlock(&proc_lock);
+			mutex_unlock(&condition_net->proc_lock);
 			info->condvar = var;
 			return 0;
 		}
@@ -158,7 +157,7 @@ static int condition_mt_check(const struct xt_mtchk_param *par)
 	/* At this point, we need to allocate a new condition variable. */
 	var = kmalloc(sizeof(struct condition_variable), GFP_KERNEL);
 	if (var == NULL) {
-		mutex_unlock(&proc_lock);
+		mutex_unlock(&condition_net->proc_lock);
 		return -ENOMEM;
 	}
 
@@ -168,7 +167,7 @@ static int condition_mt_check(const struct xt_mtchk_param *par)
 	                   condition_net->proc_net_condition, &condition_proc_fops, var);
 	if (var->status_proc == NULL) {
 		kfree(var);
-		mutex_unlock(&proc_lock);
+		mutex_unlock(&condition_net->proc_lock);
 		return -ENOMEM;
 	}
 
@@ -179,7 +178,7 @@ static int condition_mt_check(const struct xt_mtchk_param *par)
 	var->enabled  = false;
 	wmb();
 	list_add(&var->list, &condition_net->conditions_list);
-	mutex_unlock(&proc_lock);
+	mutex_unlock(&condition_net->proc_lock);
 	info->condvar = var;
 	return 0;
 }
@@ -193,15 +192,15 @@ static void condition_mt_destroy(const struct xt_mtdtor_param *par)
 	if (cnet->after_clear)
 		return;
 
-	mutex_lock(&proc_lock);
+	mutex_lock(&cnet->proc_lock);
 	if (--var->refcount == 0) {
 		list_del(&var->list);
 		remove_proc_entry(var->name, cnet->proc_net_condition);
-		mutex_unlock(&proc_lock);
+		mutex_unlock(&cnet->proc_lock);
 		kfree(var);
 		return;
 	}
-	mutex_unlock(&proc_lock);
+	mutex_unlock(&cnet->proc_lock);
 }
 
 static struct xt_match condition_mt_reg[] __read_mostly = {
@@ -232,6 +231,8 @@ static const char *const dir_name = "nf_condition";
 static int __net_init condition_net_init(struct net *net)
 {
 	struct condition_net *condition_net = condition_pernet(net);
+
+	mutex_init(&condition_net->proc_lock);
 	INIT_LIST_HEAD(&condition_net->conditions_list);
 	condition_net->proc_net_condition = proc_mkdir(dir_name, net->proc_net);
 	if (condition_net->proc_net_condition == NULL)
@@ -247,13 +248,13 @@ static void __net_exit condition_net_exit(struct net *net)
 	struct condition_variable *var = NULL;
 
 	remove_proc_subtree(dir_name, net->proc_net);
-	mutex_lock(&proc_lock);
+	mutex_lock(&condition_net->proc_lock);
 	list_for_each_safe(pos, q, &condition_net->conditions_list) {
 		var = list_entry(pos, struct condition_variable, list);
 		list_del(pos);
 		kfree(var);
 	}
-	mutex_unlock(&proc_lock);
+	mutex_unlock(&condition_net->proc_lock);
 	condition_net->after_clear = true;
 }
 
@@ -269,7 +270,6 @@ static int __init condition_mt_init(void)
 {
 	int ret;
 
-	mutex_init(&proc_lock);
 	ret = register_pernet_subsys(&condition_net_ops);
 	if (ret != 0)
 		return ret;
-- 
2.32.0

[PATCH xtables-addons 5/8] xt_condition: remove `wmb` when adding new variable.

[Jeremy Sowden]

Originally, some accesses to `conditions_list` were protected by RCU and
the memory-barrier was needed to ensure that the new variable was fully
initialized before being added to the list.  These days, however, all
accesses are protected by the `proc_lock` mutex, so the barrier is no
longer required.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/xt_condition.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extensions/xt_condition.c b/extensions/xt_condition.c
index e1672985e59b..d390faeac1b0 100644
--- a/extensions/xt_condition.c
+++ b/extensions/xt_condition.c
@@ -176,7 +176,7 @@ static int condition_mt_check(const struct xt_mtchk_param *par)
 	              make_kgid(&init_user_ns, condition_gid_perms));
 	var->refcount = 1;
 	var->enabled  = false;
-	wmb();
+
 	list_add(&var->list, &condition_net->conditions_list);
 	mutex_unlock(&condition_net->proc_lock);
 	info->condvar = var;
-- 
2.32.0

[PATCH xtables-addons 6/8] xt_condition: use `proc_net_condition` member of `struct condition_net`to signal that `condition_net_exit` has been called.

[Jeremy Sowden]

There's no need for a separate boolean flag when we can just set
`proc_net_condition` to `NULL` after the directory has been removed.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/xt_condition.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/extensions/xt_condition.c b/extensions/xt_condition.c
index d390faeac1b0..cec232e30f1f 100644
--- a/extensions/xt_condition.c
+++ b/extensions/xt_condition.c
@@ -64,7 +64,6 @@ struct condition_net {
 	struct mutex proc_lock;
 	struct list_head conditions_list;
 	struct proc_dir_entry *proc_net_condition;
-	bool after_clear;
 };
 
 static int condition_net_id;
@@ -189,7 +188,7 @@ static void condition_mt_destroy(const struct xt_mtdtor_param *par)
 	struct condition_variable *var = info->condvar;
 	struct condition_net *cnet = condition_pernet(par->net);
 
-	if (cnet->after_clear)
+	if (!cnet->proc_net_condition)
 		return;
 
 	mutex_lock(&cnet->proc_lock);
@@ -237,7 +236,6 @@ static int __net_init condition_net_init(struct net *net)
 	condition_net->proc_net_condition = proc_mkdir(dir_name, net->proc_net);
 	if (condition_net->proc_net_condition == NULL)
 		return -EACCES;
-	condition_net->after_clear = 0;
 	return 0;
 }
 
@@ -255,7 +253,7 @@ static void __net_exit condition_net_exit(struct net *net)
 		kfree(var);
 	}
 	mutex_unlock(&condition_net->proc_lock);
-	condition_net->after_clear = true;
+	condition_net->proc_net_condition = NULL;
 }
 
 static struct pernet_operations condition_net_ops = {
-- 
2.32.0

[PATCH xtables-addons 7/8] xt_condition: don't delete variables in `condition_net_exit`.

[Jeremy Sowden]

`condition_mt_destroy` will be called for every match anyway, so we may
as well do the clean-up then, rather than duplicating it.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/xt_condition.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/extensions/xt_condition.c b/extensions/xt_condition.c
index cec232e30f1f..0b0508b7723c 100644
--- a/extensions/xt_condition.c
+++ b/extensions/xt_condition.c
@@ -188,13 +188,11 @@ static void condition_mt_destroy(const struct xt_mtdtor_param *par)
 	struct condition_variable *var = info->condvar;
 	struct condition_net *cnet = condition_pernet(par->net);
 
-	if (!cnet->proc_net_condition)
-		return;
-
 	mutex_lock(&cnet->proc_lock);
 	if (--var->refcount == 0) {
 		list_del(&var->list);
-		remove_proc_entry(var->name, cnet->proc_net_condition);
+		if (cnet->proc_net_condition)
+			remove_proc_entry(var->name, cnet->proc_net_condition);
 		mutex_unlock(&cnet->proc_lock);
 		kfree(var);
 		return;
@@ -242,17 +240,8 @@ static int __net_init condition_net_init(struct net *net)
 static void __net_exit condition_net_exit(struct net *net)
 {
 	struct condition_net *condition_net = condition_pernet(net);
-	struct list_head *pos, *q;
-	struct condition_variable *var = NULL;
 
 	remove_proc_subtree(dir_name, net->proc_net);
-	mutex_lock(&condition_net->proc_lock);
-	list_for_each_safe(pos, q, &condition_net->conditions_list) {
-		var = list_entry(pos, struct condition_variable, list);
-		list_del(pos);
-		kfree(var);
-	}
-	mutex_unlock(&condition_net->proc_lock);
 	condition_net->proc_net_condition = NULL;
 }
 
@@ -263,7 +252,6 @@ static struct pernet_operations condition_net_ops = {
 	.size   = sizeof(struct condition_net),
 };
 
-
 static int __init condition_mt_init(void)
 {
 	int ret;
-- 
2.32.0

[PATCH xtables-addons 8/8] xt_condition: simplify clean-up of variables.

[Jeremy Sowden]

Unlocking early and returning in the if-block just complicate the code
to no material benefit.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/xt_condition.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/extensions/xt_condition.c b/extensions/xt_condition.c
index 0b0508b7723c..cf07966e71b7 100644
--- a/extensions/xt_condition.c
+++ b/extensions/xt_condition.c
@@ -193,9 +193,7 @@ static void condition_mt_destroy(const struct xt_mtdtor_param *par)
 		list_del(&var->list);
 		if (cnet->proc_net_condition)
 			remove_proc_entry(var->name, cnet->proc_net_condition);
-		mutex_unlock(&cnet->proc_lock);
 		kfree(var);
-		return;
 	}
 	mutex_unlock(&cnet->proc_lock);
 }
-- 
2.32.0

Re: [PATCH xtables-addons 1/8] build: bump minimum supported kernel version from 4.15 to 4.16.

[Jan Engelhardt]

On Sunday 2021-08-22 18:35, Jeremy Sowden wrote:

>The next two commits make use of a function and macro that were
>introduced in 4.16.
>
>Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
>---
> configure.ac | 2 +-

I needed to touch some more files for 4.16.

Re: [PATCH xtables-addons 0/8] xt_condition: per-net improvements

[Jan Engelhardt]

On Sunday 2021-08-22 18:35, Jeremy Sowden wrote:

>The first patch bumps the minimum version to 4.16 in order to allow us
>to use a useful macro and function in patches 2 & 3.  4 makes the
>proc_lock mutex a per-net variable.  5 removes an obsolete write
>memory-barrier.  6-8 tidy up the clean-up of matches when a namespace is
>deleted.

Processed.