Re: [PATCH] xtables-addons 3.18 condition - Improved network namespace support

Re: [PATCH] xtables-addons 3.18 condition - Improved network namespace support

[Grzegorz Kuczyński]

My intention was removed special var 'after_clear'. The main idea was use a list as a flag.

But Jeremy has right, I used list_empty_careful() in wrong place.
I can change this for that:

        static void condition_mt_destroy(const struct xt_mtdtor_param *par)
        {
                const struct xt_condition_mtinfo *info = par->matchinfo;
                struct condition_variable *var = info->condvar;
                struct condition_net *cnet = get_condition_pernet(par->net);
        
                mutex_lock(&proc_lock);
                // is called after net namespace deleted?
                if (list_empty_careful(&cnet->conditions_list)) {
                        mutex_unlock(&proc_lock);
                        return;
                }
                if (--var->refcount == 0) {
                        list_del(&var->list);
                        remove_proc_entry(var->name, cnet->proc_net_condition);
                        mutex_unlock(&proc_lock);
                        kfree(var);
                        return;
                }
                mutex_unlock(&proc_lock);
        }

But now I realized that one var bool is less expensive than call a function.
So my improved is turned out the fail :) ... sorry for taking the time.

-- 
Grzegorz Kuczyński

Re: [PATCH] xtables-addons 3.18 condition - Improved network namespace support

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

On 2021-08-22, at 18:14:13 +0200, Grzegorz Kuczyński wrote:
> My intention was removed special var 'after_clear'. The main idea was
> use a list as a flag.
>
> But Jeremy has right, I used list_empty_careful() in wrong place.
> I can change this for that:
>
>         static void condition_mt_destroy(const struct xt_mtdtor_param *par)
>         {
>                 const struct xt_condition_mtinfo *info = par->matchinfo;
>                 struct condition_variable *var = info->condvar;
>                 struct condition_net *cnet = get_condition_pernet(par->net);
>         
>                 mutex_lock(&proc_lock);
>                 // is called after net namespace deleted?
>                 if (list_empty_careful(&cnet->conditions_list)) {
>                         mutex_unlock(&proc_lock);
>                         return;
>                 }
>                 if (--var->refcount == 0) {
>                         list_del(&var->list);
>                         remove_proc_entry(var->name, cnet->proc_net_condition);
>                         mutex_unlock(&proc_lock);
>                         kfree(var);
>                         return;
>                 }
>                 mutex_unlock(&proc_lock);
>         }
>
> But now I realized that one var bool is less expensive than call a
> function.  So my improved is turned out the fail :) ... sorry for
> taking the time.

I think there are some improvements that can be made.  Will send
patches.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] xtables-addons 3.18 condition - Improved network namespace support

[Grzegorz Kuczyński]

Hello
A few years ago I add network namespace to extension condition.
I review this changes again and make changes again.
This is better version.


diff --git a/extensions/xt_condition.c b/extensions/xt_condition.c
index 8227c5d..c5b0df3 100644
--- a/extensions/xt_condition.c
+++ b/extensions/xt_condition.c
@@ -65,12 +65,11 @@ static DEFINE_MUTEX(proc_lock);
 struct condition_net {
        struct list_head conditions_list;
        struct proc_dir_entry *proc_net_condition;
-       bool after_clear;
 };
 
 static int condition_net_id;
 
-static inline struct condition_net *condition_pernet(struct net *net)
+static inline struct condition_net *get_condition_pernet(struct net *net)
 {
        return net_generic(net, condition_net_id);
 }
@@ -132,7 +131,7 @@ static int condition_mt_check(const struct
xt_mtchk_param *par)
 {
        struct xt_condition_mtinfo *info = par->matchinfo;
        struct condition_variable *var;
-       struct condition_net *condition_net = condition_pernet(par->net);
+       struct condition_net *condition_net =
get_condition_pernet(par->net);
 
        /* Forbid certain names */
        if (*info->name == '\0' || *info->name == '.' ||
@@ -190,13 +189,10 @@ static void condition_mt_destroy(const struct
xt_mtdtor_param *par)
 {
        const struct xt_condition_mtinfo *info = par->matchinfo;
        struct condition_variable *var = info->condvar;
-       struct condition_net *cnet = condition_pernet(par->net);
-
-       if (cnet->after_clear)
-               return;
-
+       struct condition_net *cnet = get_condition_pernet(par->net);
+      
        mutex_lock(&proc_lock);
-       if (--var->refcount == 0) {
+       if (--var->refcount == 0 &&
!list_empty_careful(&cnet->conditions_list)) {
                list_del(&var->list);
                remove_proc_entry(var->name, cnet->proc_net_condition);
                mutex_unlock(&proc_lock);
@@ -233,18 +229,17 @@ 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);
+       struct condition_net *condition_net = get_condition_pernet(net);
        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)
                return -EACCES;
-       condition_net->after_clear = 0;
        return 0;
 }
 
 static void __net_exit condition_net_exit(struct net *net)
 {
-       struct condition_net *condition_net = condition_pernet(net);
+       struct condition_net *condition_net = get_condition_pernet(net);
        struct list_head *pos, *q;
        struct condition_variable *var = NULL;
 
@@ -256,7 +251,6 @@ static void __net_exit condition_net_exit(struct net
*net)
                kfree(var);
        }
        mutex_unlock(&proc_lock);
-       condition_net->after_clear = true;
 }
 
 static struct pernet_operations condition_net_ops = {


-- 
Grzegorz Kuczyński

Re: [PATCH] xtables-addons 3.18 condition - Improved network namespace support

[Jan Engelhardt]

On Friday 2021-08-20 20:24, Grzegorz Kuczyński wrote:

>Hello
>A few years ago I add network namespace to extension condition.
>I review this changes again and make changes again.
>This is better version.

It does not apply. Your mail software mangled the patch.

I would also wish for a more precise description what causes
these lines to need removal.

>-       if (cnet->after_clear)
>-               return;

>-       if (--var->refcount == 0) {
>+       if (--var->refcount == 0 &&
>!list_empty_careful(&cnet->conditions_list)) {

Re: [PATCH] xtables-addons 3.18 condition - Improved network namespace support

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 3431 bytes --]

On 2021-08-22, at 13:19:46 +0200, Jan Engelhardt wrote:
> On Friday 2021-08-20 20:24, Grzegorz Kuczyński wrote:
> > A few years ago I add network namespace to extension condition.
> > I review this changes again and make changes again.
> > This is better version.
>
> It does not apply. Your mail software mangled the patch.
>
> I would also wish for a more precise description what causes
> these lines to need removal.
>
> >-       if (cnet->after_clear)
> >-               return;
>
> >-       if (--var->refcount == 0) {
> >+       if (--var->refcount == 0 && !list_empty_careful(&cnet->conditions_list)) {

I've been looking at this, and I don't think it works.

The idea is to do away with a separate flag whose purpose is to indicate
that condition_mt_destroy is being called during the tear-down of a
name-space.

Currently we have:

  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(&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);
    condition_net->after_clear = true;
  }

and:

  static void condition_mt_destroy(const struct xt_mtdtor_param *par)
  {
    const struct xt_condition_mtinfo *info = par->matchinfo;
    struct condition_variable *var = info->condvar;
    struct condition_net *cnet = condition_pernet(par->net);

    if (cnet->after_clear)
      return;

    mutex_lock(&proc_lock);
    if (--var->refcount == 0) {
      list_del(&var->list);
      remove_proc_entry(var->name, cnet->proc_net_condition);
      mutex_unlock(&proc_lock);
      kfree(var);
      return;
    }
    mutex_unlock(&proc_lock);
  }

Since pernet_operations destructors are called in the reverse of the
order in which they are registered, the destructor for xt_condition will
be called before the destructor for the table which deletes all the
rules.  The `after_clear` flag serves to indicate during the call of
condition_mt_destroy that condition_net_exit has already been called and
freed all the variables.  Replacing it with a check whether list of
variables is empty could work, but not after we've checked the ref-count
of a variable that we have already freed.  Based on what I've seen in
other modules, I'd be more inclined to do something like this:

  static void __net_exit condition_net_exit(struct net *net)
  {
    struct condition_net *condition_net = condition_pernet(net);

    remove_proc_subtree(dir_name, net->proc_net);
    condition_net->proc_net_condition = NULL;
  }

and:

  static void condition_mt_destroy(const struct xt_mtdtor_param *par)
  {
    const struct xt_condition_mtinfo *info = par->matchinfo;
    struct condition_variable *var = info->condvar;
    struct condition_net *cnet = condition_pernet(par->net);

    mutex_lock(&proc_lock);
    if (--var->refcount == 0) {
      list_del(&var->list);
      if (condition_net->proc_net_condition)
        remove_proc_entry(var->name, cnet->proc_net_condition);
      kfree(var);
    }
    mutex_unlock(&proc_lock);
  }

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]