| Message ID | 61f106376f836829d8c283fb4c0cf11567c953bf.1518775496.git.lucien.xin@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net] netfilter: unlock xt_table earlier in __do_replace | expand |
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:
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(-)