| Message ID | 1333448001-2507-4-git-send-email-pablo@netfilter.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
pablo@netfilter.org: > From: Pablo Neira Ayuso <pablo@netfilter.org> > > If CONFIG_NF_CONNTRACK_TIMEOUT=n we have following warning : > > CC [M] net/netfilter/xt_CT.o > net/netfilter/xt_CT.c: In function ‘xt_ct_tg_check_v1’: > net/netfilter/xt_CT.c:284: warning: label ‘err4’ defined but not used > > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > net/netfilter/xt_CT.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c > index 0c8e438..138b75e 100644 > --- a/net/netfilter/xt_CT.c > +++ b/net/netfilter/xt_CT.c > @@ -281,8 +281,10 @@ out: > info->ct = ct; > return 0; > > +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT > err4: > rcu_read_unlock(); > +#endif > err3: > nf_conntrack_free(ct); > err2: > -- > 1.7.2.5 Looking at that function: 216 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT 217 if (info->timeout) { 218 typeof(nf_ct_timeout_find_get_hook) timeout_find_get; 219 struct ctnl_timeout *timeout; 220 struct nf_conn_timeout *timeout_ext; 221 222 rcu_read_lock(); 223 timeout_find_get = 224 rcu_dereference(nf_ct_timeout_find_get_hook); 225 226 if (timeout_find_get) { 227 const struct ipt_entry *e = par->entryinfo; 228 struct nf_conntrack_l4proto *l4proto; 229 230 if (e->ip.invflags & IPT_INV_PROTO) { 231 ret = -EINVAL; 232 pr_info("You cannot use inversion on " 233 "L4 protocol\n"); 234 goto err4; 235 } 236 timeout = timeout_find_get(info->timeout); 237 if (timeout == NULL) { 238 ret = -ENOENT; 239 pr_info("No such timeout policy \"%s\"\n", 240 info->timeout); 241 goto err4; 242 } 243 if (timeout->l3num != par->family) { 244 ret = -EINVAL; 245 pr_info("Timeout policy `%s' can only be " 246 "used by L3 protocol number %d\n", 247 info->timeout, timeout->l3num); 248 goto err4; 249 } 250 /* Make sure the timeout policy matches any existing 251 * protocol tracker, otherwise default to generic. 252 */ 253 l4proto = __nf_ct_l4proto_find(par->family, 254 e->ip.proto); 255 if (timeout->l4proto->l4proto != l4proto->l4proto) { 256 ret = -EINVAL; 257 pr_info("Timeout policy `%s' can only be " 258 "used by L4 protocol number %d\n", 259 info->timeout, 260 timeout->l4proto->l4proto); 261 goto err4; 262 } 263 timeout_ext = nf_ct_timeout_ext_add(ct, timeout, 264 GFP_KERNEL); We are under rcu_read_lock() here. 265 if (timeout_ext == NULL) { 266 ret = -ENOMEM; 267 goto err4; 268 } 269 } else { 270 ret = -ENOENT; 271 pr_info("Timeout policy base is empty\n"); 272 goto err4; 273 } 274 rcu_read_unlock(); 275 } 276 #endif -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 03, 2012 at 07:27:50PM +0900, Tetsuo Handa wrote: > pablo@netfilter.org: > > From: Pablo Neira Ayuso <pablo@netfilter.org> > > > > If CONFIG_NF_CONNTRACK_TIMEOUT=n we have following warning : > > > > CC [M] net/netfilter/xt_CT.o > > net/netfilter/xt_CT.c: In function ‘xt_ct_tg_check_v1’: > > net/netfilter/xt_CT.c:284: warning: label ‘err4’ defined but not used > > > > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > net/netfilter/xt_CT.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c > > index 0c8e438..138b75e 100644 > > --- a/net/netfilter/xt_CT.c > > +++ b/net/netfilter/xt_CT.c > > @@ -281,8 +281,10 @@ out: > > info->ct = ct; > > return 0; > > > > +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT > > err4: > > rcu_read_unlock(); > > +#endif > > err3: > > nf_conntrack_free(ct); > > err2: > > -- > > 1.7.2.5 > > Looking at that function: > > 216 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT > 217 if (info->timeout) { > 218 typeof(nf_ct_timeout_find_get_hook) timeout_find_get; > 219 struct ctnl_timeout *timeout; > 220 struct nf_conn_timeout *timeout_ext; > 221 > 222 rcu_read_lock(); > 223 timeout_find_get = > 224 rcu_dereference(nf_ct_timeout_find_get_hook); > 225 > 226 if (timeout_find_get) { > 227 const struct ipt_entry *e = par->entryinfo; > 228 struct nf_conntrack_l4proto *l4proto; > 229 > 230 if (e->ip.invflags & IPT_INV_PROTO) { > 231 ret = -EINVAL; > 232 pr_info("You cannot use inversion on " > 233 "L4 protocol\n"); > 234 goto err4; > 235 } > 236 timeout = timeout_find_get(info->timeout); > 237 if (timeout == NULL) { > 238 ret = -ENOENT; > 239 pr_info("No such timeout policy \"%s\"\n", > 240 info->timeout); > 241 goto err4; > 242 } > 243 if (timeout->l3num != par->family) { > 244 ret = -EINVAL; > 245 pr_info("Timeout policy `%s' can only be " > 246 "used by L3 protocol number %d\n", > 247 info->timeout, timeout->l3num); > 248 goto err4; > 249 } > 250 /* Make sure the timeout policy matches any existing > 251 * protocol tracker, otherwise default to generic. > 252 */ > 253 l4proto = __nf_ct_l4proto_find(par->family, > 254 e->ip.proto); > 255 if (timeout->l4proto->l4proto != l4proto->l4proto) { > 256 ret = -EINVAL; > 257 pr_info("Timeout policy `%s' can only be " > 258 "used by L4 protocol number %d\n", > 259 info->timeout, > 260 timeout->l4proto->l4proto); > 261 goto err4; > 262 } > 263 timeout_ext = nf_ct_timeout_ext_add(ct, timeout, > 264 GFP_KERNEL); > We are under rcu_read_lock() here. Good catch, that needs to be GFP_ATOMIC. I'll send a follow-up patch for this. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
One more question. Tetsuo Handa wrote: > 216 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT > 217 if (info->timeout) { > 218 typeof(nf_ct_timeout_find_get_hook) timeout_find_get; > 219 struct ctnl_timeout *timeout; > 220 struct nf_conn_timeout *timeout_ext; > 221 > 222 rcu_read_lock(); > 223 timeout_find_get = > 224 rcu_dereference(nf_ct_timeout_find_get_hook); > 225 > 226 if (timeout_find_get) { I assume timeout_find_get points to e.g. ctnl_timeout_find_get in net/netfilter/nfnetlink_cttimeout.c . If yes, > 227 const struct ipt_entry *e = par->entryinfo; > 228 struct nf_conntrack_l4proto *l4proto; > 229 > 230 if (e->ip.invflags & IPT_INV_PROTO) { > 231 ret = -EINVAL; > 232 pr_info("You cannot use inversion on " > 233 "L4 protocol\n"); > 234 goto err4; > 235 } > 236 timeout = timeout_find_get(info->timeout); > 237 if (timeout == NULL) { > 238 ret = -ENOENT; > 239 pr_info("No such timeout policy \"%s\"\n", > 240 info->timeout); > 241 goto err4; > 242 } I think "goto err4;" after successful timeout_find_get() wants e.g. nf_ct_timeout_put_hook call (e.g. ctnl_timeout_put()). > 243 if (timeout->l3num != par->family) { > 244 ret = -EINVAL; > 245 pr_info("Timeout policy `%s' can only be " > 246 "used by L3 protocol number %d\n", > 247 info->timeout, timeout->l3num); > 248 goto err4; > 249 } -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/netfilter/xt_CT.c b/net/netfilter/xt_CT.c index 0c8e438..138b75e 100644 --- a/net/netfilter/xt_CT.c +++ b/net/netfilter/xt_CT.c @@ -281,8 +281,10 @@ out: info->ct = ct; return 0; +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT err4: rcu_read_unlock(); +#endif err3: nf_conntrack_free(ct); err2: