| Message ID | 1540656343-822-6-git-send-email-kadlec@blackhole.kfki.hu |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [1/5] netfilter: ipset: list:set: Decrease refcount synchronously on deletion and replace | expand |
Hi Jozsef, On Sat, Oct 27, 2018 at 06:05:43PM +0200, Jozsef Kadlecsik wrote: > The ip_set() macro is called when either ip_set_ref_lock held only > or no lock/nfnl mutex is held at dumping. Take this into account > properly. > > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> > --- > net/netfilter/ipset/ip_set_core.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c > index 68db946..292f7c7 100644 > --- a/net/netfilter/ipset/ip_set_core.c > +++ b/net/netfilter/ipset/ip_set_core.c > @@ -55,12 +55,27 @@ MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>"); > MODULE_DESCRIPTION("core IP set support"); > MODULE_ALIAS_NFNL_SUBSYS(NFNL_SUBSYS_IPSET); > > -/* When the nfnl mutex is held: */ > +/* When the nfnl mutex or ip_set_ref_lock is held: */ > #define ip_set_dereference(p) \ > - rcu_dereference_protected(p, lockdep_nfnl_is_held(NFNL_SUBSYS_IPSET)) > + rcu_dereference_protected(p, \ > + lockdep_nfnl_is_held(NFNL_SUBSYS_IPSET) || \ > + lockdep_is_held(&ip_set_ref_lock)) > #define ip_set(inst, id) \ > ip_set_dereference((inst)->ip_set_list)[id] > > +/* When the nfnl mutex is not held (dumping): */ > +static inline > +struct ip_set * ip_set_no_mutex(struct ip_set_net *inst, ip_set_id_t id) > +{ > + struct ip_set *set; > + > + rcu_read_lock(); > + set = rcu_dereference((inst)->ip_set_list)[id]; > + rcu_read_unlock(); If your goal is to calm down bogus warnings, you can use instead: set = rcu_dereference_raw(...); instead, in case ip_set_dump_done() is already protected under rcu_read_lock(). Otherwise, you have to rework your rcu approach in the dump_done path. rcu is not useful in the way you use it above, because future dereference of set are only valid insider the rcu read lock section. Would you send me a follow up patch to amend this? Thanks.
Hi Pablo, On Mon, 29 Oct 2018, Pablo Neira Ayuso wrote: > On Sat, Oct 27, 2018 at 06:05:43PM +0200, Jozsef Kadlecsik wrote: > > The ip_set() macro is called when either ip_set_ref_lock held only > > or no lock/nfnl mutex is held at dumping. Take this into account > > properly. > > > > Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> > > --- > > net/netfilter/ipset/ip_set_core.c | 23 +++++++++++++++++++---- > > 1 file changed, 19 insertions(+), 4 deletions(-) > > > > diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c > > index 68db946..292f7c7 100644 > > --- a/net/netfilter/ipset/ip_set_core.c > > +++ b/net/netfilter/ipset/ip_set_core.c > > @@ -55,12 +55,27 @@ MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>"); > > MODULE_DESCRIPTION("core IP set support"); > > MODULE_ALIAS_NFNL_SUBSYS(NFNL_SUBSYS_IPSET); > > > > -/* When the nfnl mutex is held: */ > > +/* When the nfnl mutex or ip_set_ref_lock is held: */ > > #define ip_set_dereference(p) \ > > - rcu_dereference_protected(p, lockdep_nfnl_is_held(NFNL_SUBSYS_IPSET)) > > + rcu_dereference_protected(p, \ > > + lockdep_nfnl_is_held(NFNL_SUBSYS_IPSET) || \ > > + lockdep_is_held(&ip_set_ref_lock)) > > #define ip_set(inst, id) \ > > ip_set_dereference((inst)->ip_set_list)[id] > > > > +/* When the nfnl mutex is not held (dumping): */ > > +static inline > > +struct ip_set * ip_set_no_mutex(struct ip_set_net *inst, ip_set_id_t id) > > +{ > > + struct ip_set *set; > > + > > + rcu_read_lock(); > > + set = rcu_dereference((inst)->ip_set_list)[id]; > > + rcu_read_unlock(); > > If your goal is to calm down bogus warnings, you can use instead: > > set = rcu_dereference_raw(...); > > instead, in case ip_set_dump_done() is already protected under > rcu_read_lock(). Otherwise, you have to rework your rcu approach in > the dump_done path. Yes, you are completely right: the goal of the patch was to silence bogus warnings but that part is useless, as you pointed out. > rcu is not useful in the way you use it above, because future > dereference of set are only valid insider the rcu read lock section. > > Would you send me a follow up patch to amend this? I'm going to send you a fixed patch. Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c index 68db946..292f7c7 100644 --- a/net/netfilter/ipset/ip_set_core.c +++ b/net/netfilter/ipset/ip_set_core.c @@ -55,12 +55,27 @@ MODULE_AUTHOR("Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>"); MODULE_DESCRIPTION("core IP set support"); MODULE_ALIAS_NFNL_SUBSYS(NFNL_SUBSYS_IPSET); -/* When the nfnl mutex is held: */ +/* When the nfnl mutex or ip_set_ref_lock is held: */ #define ip_set_dereference(p) \ - rcu_dereference_protected(p, lockdep_nfnl_is_held(NFNL_SUBSYS_IPSET)) + rcu_dereference_protected(p, \ + lockdep_nfnl_is_held(NFNL_SUBSYS_IPSET) || \ + lockdep_is_held(&ip_set_ref_lock)) #define ip_set(inst, id) \ ip_set_dereference((inst)->ip_set_list)[id] +/* When the nfnl mutex is not held (dumping): */ +static inline +struct ip_set * ip_set_no_mutex(struct ip_set_net *inst, ip_set_id_t id) +{ + struct ip_set *set; + + rcu_read_lock(); + set = rcu_dereference((inst)->ip_set_list)[id]; + rcu_read_unlock(); + + return set; +} + /* The set types are implemented in modules and registered set types * can be found in ip_set_type_list. Adding/deleting types is * serialized by ip_set_type_mutex. @@ -1251,7 +1266,7 @@ ip_set_dump_done(struct netlink_callback *cb) struct ip_set_net *inst = (struct ip_set_net *)cb->args[IPSET_CB_NET]; ip_set_id_t index = (ip_set_id_t)cb->args[IPSET_CB_INDEX]; - struct ip_set *set = ip_set(inst, index); + struct ip_set *set = ip_set_no_mutex(inst, index); if (set->variant->uref) set->variant->uref(set, cb, false); @@ -1440,7 +1455,7 @@ ip_set_dump_start(struct sk_buff *skb, struct netlink_callback *cb) release_refcount: /* If there was an error or set is done, release set */ if (ret || !cb->args[IPSET_CB_ARG0]) { - set = ip_set(inst, index); + set = ip_set_no_mutex(inst, index); if (set->variant->uref) set->variant->uref(set, cb, false); pr_debug("release set %s\n", set->name);
The ip_set() macro is called when either ip_set_ref_lock held only or no lock/nfnl mutex is held at dumping. Take this into account properly. Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> --- net/netfilter/ipset/ip_set_core.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)