[PATCH net] netfilter: unlock xt_table earlier in __do_replace

[PATCH net] netfilter: unlock xt_table earlier in __do_replace

[Xin Long]

Now it's doing cleanup_entry for oldinfo under the xt_table lock,
but it's not really necessary. After the replacement job is done
in xt_replace_table, oldinfo is not used elsewhere any more, and
it can be freed without xt_table lock safely.

The important thing is that rtnl_lock is called in some xt_target
destroy, which means rtnl_lock, a big lock is used in xt_table
lock, a smaller one. It usually could be the reason why a dead
lock may happen.

Besides, all xt_target/match checkentry is called out of xt_table
lock. It's better also to move all cleanup_entry calling out of
xt_table lock, just as do_replace_finish does for ebtables.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/ipv4/netfilter/arp_tables.c | 3 ++-
 net/ipv4/netfilter/ip_tables.c  | 3 ++-
 net/ipv6/netfilter/ip6_tables.c | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 4ffe302..f6c7404 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -921,6 +921,8 @@ static int __do_replace(struct net *net, const char *name,
 	    (newinfo->number <= oldinfo->initial_entries))
 		module_put(t->me);
 
+	xt_table_unlock(t);
+
 	get_old_counters(oldinfo, counters);
 
 	/* Decrease module usage counts and free resource */
@@ -935,7 +937,6 @@ static int __do_replace(struct net *net, const char *name,
 		net_warn_ratelimited("arptables: counters copy to user failed while replacing table\n");
 	}
 	vfree(counters);
-	xt_table_unlock(t);
 	return ret;
 
  put_module:
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 9a71f31..a6517c1 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1082,6 +1082,8 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 	    (newinfo->number <= oldinfo->initial_entries))
 		module_put(t->me);
 
+	xt_table_unlock(t);
+
 	get_old_counters(oldinfo, counters);
 
 	/* Decrease module usage counts and free resource */
@@ -1095,7 +1097,6 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 		net_warn_ratelimited("iptables: counters copy to user failed while replacing table\n");
 	}
 	vfree(counters);
-	xt_table_unlock(t);
 	return ret;
 
  put_module:
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index af4c917..96577c9 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1101,6 +1101,8 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 	    (newinfo->number <= oldinfo->initial_entries))
 		module_put(t->me);
 
+	xt_table_unlock(t);
+
 	get_old_counters(oldinfo, counters);
 
 	/* Decrease module usage counts and free resource */
@@ -1114,7 +1116,6 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 		net_warn_ratelimited("ip6tables: counters copy to user failed while replacing table\n");
 	}
 	vfree(counters);
-	xt_table_unlock(t);
 	return ret;
 
  put_module:
-- 
2.1.0

Re: [PATCH net] netfilter: unlock xt_table earlier in __do_replace

[Florian Westphal]

Xin Long <lucien.xin@gmail.com> wrote:
> Now it's doing cleanup_entry for oldinfo under the xt_table lock,
> but it's not really necessary. After the replacement job is done
> in xt_replace_table, oldinfo is not used elsewhere any more, and
> it can be freed without xt_table lock safely.

Right.

> The important thing is that rtnl_lock is called in some xt_target
> destroy, which means rtnl_lock, a big lock is used in xt_table
> lock, a smaller one. It usually could be the reason why a dead
> lock may happen.

In which cases do we aquire the xt table mutex from places that hold
rtnl mutex?

> Besides, all xt_target/match checkentry is called out of xt_table
> lock. It's better also to move all cleanup_entry calling out of
> xt_table lock, just as do_replace_finish does for ebtables.

Agree but I don't see how this patch fixes a bug so I would prefer if
this could simmer in nf-next first.

Re: [PATCH net] netfilter: unlock xt_table earlier in __do_replace

[Xin Long]

On Fri, Feb 16, 2018 at 12:02 PM, Florian Westphal <fw@strlen.de> wrote:
> Xin Long <lucien.xin@gmail.com> wrote:
>> Now it's doing cleanup_entry for oldinfo under the xt_table lock,
>> but it's not really necessary. After the replacement job is done
>> in xt_replace_table, oldinfo is not used elsewhere any more, and
>> it can be freed without xt_table lock safely.
>
> Right.
>
>> The important thing is that rtnl_lock is called in some xt_target
>> destroy, which means rtnl_lock, a big lock is used in xt_table
>> lock, a smaller one. It usually could be the reason why a dead
>> lock may happen.
>
> In which cases do we aquire the xt table mutex from places that hold
> rtnl mutex?
Not really now.
But there was one, which though had been fixed in another way in:
https://patchwork.ozlabs.org/patch/870797/

I meant this kind of case (big lock used under small lock) have the
risk that may cause a dead lock.
Sorry for confusing.

>
>> Besides, all xt_target/match checkentry is called out of xt_table
>> lock. It's better also to move all cleanup_entry calling out of
>> xt_table lock, just as do_replace_finish does for ebtables.
>
> Agree but I don't see how this patch fixes a bug so I would prefer if
> this could simmer in nf-next first.
Sure. No bug fix, it's an improvement.

Re: [PATCH net] netfilter: unlock xt_table earlier in __do_replace

[Pablo Neira Ayuso]

On Fri, Feb 16, 2018 at 12:25:56PM +0100, Xin Long wrote:
> On Fri, Feb 16, 2018 at 12:02 PM, Florian Westphal <fw@strlen.de> wrote:
> > Xin Long <lucien.xin@gmail.com> wrote:
[...]
> >> Besides, all xt_target/match checkentry is called out of xt_table
> >> lock. It's better also to move all cleanup_entry calling out of
> >> xt_table lock, just as do_replace_finish does for ebtables.
> >
> > Agree but I don't see how this patch fixes a bug so I would prefer if
> > this could simmer in nf-next first.
>
> Sure. No bug fix, it's an improvement.

Applied to nf-next, thanks.