| Message ID | 1485262100-12366-1-git-send-email-dwindsor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Tue, 24 Jan 2017, David Windsor wrote: > Currently, the ip_vs_dest cache frees ip_vs_dest objects when their reference > count becomes < 0. Aside from not being semantically sound, this is problematic > for the new type refcount_t, which will be introduced shortly in a separate patch. > refcount_t is the new kernel type for holding reference counts, and provides > overflow protection and a constrained interface relative to atomic_t (the type > currently being used for kernel reference counts). > > Per Juilan Anastasov: "The problem is that dest_trash currently holds deleted Julian > dests (unlinked from RCU lists) with refcnt=0." Changing dest_trash to hold > dest with refcnt=1 will allow us to free ip_vs_dest structs when their refcnt=0, > in ip_vs_dest_put_and_free(). Too long lines, should be up to 75 columns as per Documentation/process/submitting-patches.rst Check with: scripts/checkpatch.pl --strict /tmp/file.patch > Signed-off-by: David Windsor <dwindsor@gmail.com> > --- > include/net/ip_vs.h | 2 +- > net/netfilter/ipvs/ip_vs_ctl.c | 4 +--- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c > index 55e0169..6b5492e 100644 > --- a/net/netfilter/ipvs/ip_vs_ctl.c > +++ b/net/netfilter/ipvs/ip_vs_ctl.c > @@ -711,7 +711,6 @@ ip_vs_trash_get_dest(struct ip_vs_service *svc, int dest_af, > dest->vport == svc->port))) { > /* HIT */ > list_del(&dest->t_list); > - ip_vs_dest_hold(dest); > goto out; > } > } The changes look ok to me but as this is a net-next material I also prefer some comments in the code to be updated: - "be 0, so we simply release them here" before ip_vs_trash_cleanup should become "be 1, so we simply release them here". - "dest lives in trash with[out] reference" in __ip_vs_del_dest When sending v2 for net-next please also CC: Simon Horman <horms@verge.net.au> Pablo Neira Ayuso <pablo@netfilter.org> lvs-devel@vger.kernel.org I'll ack the next version after some tests... Regards -- Julian Anastasov <ja@ssi.bg>
From: Julian Anastasov <ja@ssi.bg> Date: Tue, 24 Jan 2017 23:14:29 +0200 (EET) > The changes look ok to me but as this is a net-next > material I also prefer some comments in the code to be updated: It also needs to be posted to netfilter-devel with the IPVS maintainer CC:'d
diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h index cd6018a..a3e78ad 100644 --- a/include/net/ip_vs.h +++ b/include/net/ip_vs.h @@ -1421,7 +1421,7 @@ static inline void ip_vs_dest_put(struct ip_vs_dest *dest) static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest) { - if (atomic_dec_return(&dest->refcnt) < 0) + if (atomic_dec_and_test(&dest->refcnt)) kfree(dest); } diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index 55e0169..6b5492e 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -711,7 +711,6 @@ ip_vs_trash_get_dest(struct ip_vs_service *svc, int dest_af, dest->vport == svc->port))) { /* HIT */ list_del(&dest->t_list); - ip_vs_dest_hold(dest); goto out; } } @@ -1084,7 +1083,6 @@ static void __ip_vs_del_dest(struct netns_ipvs *ipvs, struct ip_vs_dest *dest, list_add(&dest->t_list, &ipvs->dest_trash); dest->idle_start = 0; spin_unlock_bh(&ipvs->dest_trash_lock); - ip_vs_dest_put(dest); } @@ -1160,7 +1158,7 @@ static void ip_vs_dest_trash_expire(unsigned long data) spin_lock(&ipvs->dest_trash_lock); list_for_each_entry_safe(dest, next, &ipvs->dest_trash, t_list) { - if (atomic_read(&dest->refcnt) > 0) + if (atomic_read(&dest->refcnt) > 1) continue; if (dest->idle_start) { if (time_before(now, dest->idle_start +
Currently, the ip_vs_dest cache frees ip_vs_dest objects when their reference count becomes < 0. Aside from not being semantically sound, this is problematic for the new type refcount_t, which will be introduced shortly in a separate patch. refcount_t is the new kernel type for holding reference counts, and provides overflow protection and a constrained interface relative to atomic_t (the type currently being used for kernel reference counts). Per Juilan Anastasov: "The problem is that dest_trash currently holds deleted dests (unlinked from RCU lists) with refcnt=0." Changing dest_trash to hold dest with refcnt=1 will allow us to free ip_vs_dest structs when their refcnt=0, in ip_vs_dest_put_and_free(). Signed-off-by: David Windsor <dwindsor@gmail.com> --- include/net/ip_vs.h | 2 +- net/netfilter/ipvs/ip_vs_ctl.c | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-)