Message ID | 1279548947-10470-2-git-send-email-luciano.coelho@nokia.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Monday 2010-07-19 16:15, Luciano Coelho wrote: >From: Luciano Coelho <coelho@testbed> > >This patch isolates and exports the condition list management code, in >preparation for the CONDITION target to use it. No functional change, >just reorganization of the code. Well, I guess it would make more sense if the two extensions be in a single file. That would alleviate the need for export reorganizations, and also works because the module metadata overhead is large already. >@@ -3,12 +3,27 @@ > > #include <linux/types.h> > >+#define XT_CONDITION_MAX_NAME_SIZE 30 >+ > struct xt_condition_mtinfo { >- char name[31]; >+ char name[XT_CONDITION_MAX_NAME_SIZE + 1]; > __u8 invert; Oh noes. Please please avoid any math operations inside []. It has already driven XT_FUNCTION_MAXNAMELEN into nuts ("was it now +1 or -1, or even -2 that we needed to pass for various functions?"). Just let MAX be 31 and have name[MAX]. > MODULE_ALIAS("ip6t_condition"); > >-struct condition_variable { >- struct list_head list; >- struct proc_dir_entry *status_proc; >- unsigned int refcount; >- bool enabled; >-}; Given your excellent usage example of a CONDITION target, I think it even makes sense to enlarge the "enabled" variable to a full-fledged 32-bit value that can be &, | and ^'d, similar to nfmark. -- 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 Mon, 2010-07-19 at 18:13 +0200, ext Jan Engelhardt wrote: > On Monday 2010-07-19 16:15, Luciano Coelho wrote: > > >From: Luciano Coelho <coelho@testbed> > > > >This patch isolates and exports the condition list management code, in > >preparation for the CONDITION target to use it. No functional change, > >just reorganization of the code. > > Well, I guess it would make more sense if the two extensions be in a > single file. That would alleviate the need for export reorganizations, > and also works because the module metadata overhead is large already. Right. I'll change the code so that the two extensions are in the same file/module. You're the second person to mention this already. ;) > >@@ -3,12 +3,27 @@ > > > > #include <linux/types.h> > > > >+#define XT_CONDITION_MAX_NAME_SIZE 30 > >+ > > struct xt_condition_mtinfo { > >- char name[31]; > >+ char name[XT_CONDITION_MAX_NAME_SIZE + 1]; > > __u8 invert; > > Oh noes. Please please avoid any math operations inside []. It has > already driven XT_FUNCTION_MAXNAMELEN into nuts ("was it now +1 or -1, > or even -2 that we needed to pass for various functions?"). Just let MAX > be 31 and have name[MAX]. Yeah, I had already done as you suggested in my previous module (IDLETIMER), I don't know what I had in my head today when I did it differently. Even the name of the macro is totally wrong (_SIZE), it would make a tiny little bit more sense if it was _LEN. I'll change it. > > MODULE_ALIAS("ip6t_condition"); > > > >-struct condition_variable { > >- struct list_head list; > >- struct proc_dir_entry *status_proc; > >- unsigned int refcount; > >- bool enabled; > >-}; > > Given your excellent usage example of a CONDITION target, I think it > even makes sense to enlarge the "enabled" variable to a full-fledged > 32-bit value that can be &, | and ^'d, similar to nfmark. Ok, that's a good idea, I'll do that. Thanks for your comments!
On Mon, 2010-07-19 at 21:14 +0200, Coelho Luciano (Nokia-MS/Helsinki) wrote: > On Mon, 2010-07-19 at 18:13 +0200, ext Jan Engelhardt wrote: > > On Monday 2010-07-19 16:15, Luciano Coelho wrote: > > >@@ -3,12 +3,27 @@ > > > > > > #include <linux/types.h> > > > > > >+#define XT_CONDITION_MAX_NAME_SIZE 30 > > >+ > > > struct xt_condition_mtinfo { > > >- char name[31]; > > >+ char name[XT_CONDITION_MAX_NAME_SIZE + 1]; > > > __u8 invert; > > > > Oh noes. Please please avoid any math operations inside []. It has > > already driven XT_FUNCTION_MAXNAMELEN into nuts ("was it now +1 or -1, > > or even -2 that we needed to pass for various functions?"). Just let MAX > > be 31 and have name[MAX]. > > Yeah, I had already done as you suggested in my previous module > (IDLETIMER), I don't know what I had in my head today when I did it > differently. Even the name of the macro is totally wrong (_SIZE), it > would make a tiny little bit more sense if it was _LEN. I'll change it. I was not very clear here, I meant I'll change to what you proposed, ie. keep it _SIZE and use 31, of course.
diff --git a/include/linux/netfilter/xt_condition.h b/include/linux/netfilter/xt_condition.h index 4faf3ca..eebf41a 100644 --- a/include/linux/netfilter/xt_condition.h +++ b/include/linux/netfilter/xt_condition.h @@ -3,12 +3,27 @@ #include <linux/types.h> +#define XT_CONDITION_MAX_NAME_SIZE 30 + struct xt_condition_mtinfo { - char name[31]; + char name[XT_CONDITION_MAX_NAME_SIZE + 1]; __u8 invert; /* Used internally by the kernel */ void *condvar __attribute__((aligned(8))); }; +#ifdef __KERNEL__ +struct condition_variable { + struct list_head list; + struct proc_dir_entry *status_proc; + unsigned int refcount; + bool enabled; +}; + +struct condition_variable *xt_condition_insert(const char *name); +void xt_condition_put(struct condition_variable *var); +void xt_condition_set(struct condition_variable *var, bool enabled); +#endif /* __KERNEL__ */ + #endif /* _XT_CONDITION_H */ diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c index a7ccea3..dec97fe 100644 --- a/net/netfilter/xt_condition.c +++ b/net/netfilter/xt_condition.c @@ -43,13 +43,6 @@ MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condi MODULE_ALIAS("ipt_condition"); MODULE_ALIAS("ip6t_condition"); -struct condition_variable { - struct list_head list; - struct proc_dir_entry *status_proc; - unsigned int refcount; - bool enabled; -}; - /* proc_lock is a user context only semaphore used for write access */ /* to the conditions' list. */ static DEFINE_MUTEX(proc_lock); @@ -100,47 +93,34 @@ condition_mt(const struct sk_buff *skb, struct xt_action_param *par) return var->enabled ^ info->invert; } -static int condition_mt_check(const struct xt_mtchk_param *par) +struct condition_variable *xt_condition_insert(const char *name) { - struct xt_condition_mtinfo *info = par->matchinfo; struct condition_variable *var; - /* Forbid certain names */ - if (*info->name == '\0' || *info->name == '.' || - info->name[sizeof(info->name)-1] != '\0' || - memchr(info->name, '/', sizeof(info->name)) != NULL) { - pr_info("name not allowed or too long: \"%.*s\"\n", - (unsigned int)sizeof(info->name), info->name); - return -EINVAL; - } /* * Let's acquire the lock, check for the condition and add it * or increase the reference counter. */ mutex_lock(&proc_lock); list_for_each_entry(var, &conditions_list, list) { - if (strcmp(info->name, var->status_proc->name) == 0) { + if (strcmp(name, var->status_proc->name) == 0) { ++var->refcount; - mutex_unlock(&proc_lock); - info->condvar = var; - return 0; + goto out; } } /* At this point, we need to allocate a new condition variable. */ var = kmalloc(sizeof(struct condition_variable), GFP_KERNEL); - if (var == NULL) { - mutex_unlock(&proc_lock); - return -ENOMEM; - } + if (var == NULL) + goto out; /* Create the condition variable's proc file entry. */ - var->status_proc = create_proc_entry(info->name, condition_list_perms, + var->status_proc = create_proc_entry(name, condition_list_perms, proc_net_condition); if (var->status_proc == NULL) { kfree(var); - mutex_unlock(&proc_lock); - return -ENOMEM; + var = NULL; + goto out; } var->refcount = 1; @@ -151,16 +131,14 @@ static int condition_mt_check(const struct xt_mtchk_param *par) var->status_proc->uid = condition_uid_perms; var->status_proc->gid = condition_gid_perms; list_add(&var->list, &conditions_list); +out: mutex_unlock(&proc_lock); - info->condvar = var; - return 0; + return var; } +EXPORT_SYMBOL_GPL(xt_condition_insert); -static void condition_mt_destroy(const struct xt_mtdtor_param *par) +void xt_condition_put(struct condition_variable *var) { - const struct xt_condition_mtinfo *info = par->matchinfo; - struct condition_variable *var = info->condvar; - mutex_lock(&proc_lock); if (--var->refcount == 0) { list_del(&var->list); @@ -171,6 +149,42 @@ static void condition_mt_destroy(const struct xt_mtdtor_param *par) } mutex_unlock(&proc_lock); } +EXPORT_SYMBOL_GPL(xt_condition_put); + +void xt_condition_set(struct condition_variable *var, bool enabled) +{ + var->enabled = enabled; +} +EXPORT_SYMBOL_GPL(xt_condition_set); + +static int condition_mt_check(const struct xt_mtchk_param *par) +{ + struct xt_condition_mtinfo *info = par->matchinfo; + struct condition_variable *var; + + /* Forbid certain names */ + if (*info->name == '\0' || *info->name == '.' || + info->name[sizeof(info->name)-1] != '\0' || + memchr(info->name, '/', sizeof(info->name)) != NULL) { + pr_info("name not allowed or too long: \"%.*s\"\n", + (unsigned int)sizeof(info->name), info->name); + return -EINVAL; + } + + var = xt_condition_insert(info->name); + if (var == NULL) + return -ENOMEM; + + info->condvar = var; + return 0; +} + +static void condition_mt_destroy(const struct xt_mtdtor_param *par) +{ + const struct xt_condition_mtinfo *info = par->matchinfo; + + xt_condition_put(info->condvar); +} static struct xt_match condition_mt_reg __read_mostly = { .name = "condition",