Message ID | 1279807758-6876-2-git-send-email-luciano.coelho@nokia.com |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
On Thursday 2010-07-22 16:09, Luciano Coelho wrote: >+static int condition_mt_check(const struct xt_mtchk_param *par) >+{ >+ struct xt_condition_mtinfo *info = par->matchinfo; >+ struct condition_variable *var; >+ struct condition_net *cond_net = >+ condition_pernet(current->nsproxy->net_ns); Cc'ing Alexey who has done the netns support. Alexey, you added par->net, but given Luciano just did it with current->nsproxy->net_ns, do we really need par->net? >+int xt_condition_set_module_perms(const char *val, struct kernel_param *kp) >+{ >+ unsigned long l; >+ int ret; >+ struct condition_net *cond_net = >+ condition_pernet(current->nsproxy->net_ns); >+ >+ if (!val) return -EINVAL; newline before return. >+ ret = strict_strtoul(val, 0, &l); >+ if (ret == -EINVAL || ((uint)l != l)) >+ return -EINVAL; >+ *((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l; I don't think we need this level of granularity; let the options be global, similar to what xt_hashlimit does. (I am not even sure if kp->arg can be non-multiples-of-4, in which case this would be an alignment violation even.) >+ >+ return 0; >+} >+EXPORT_SYMBOL_GPL(xt_condition_set_module_perms); >+ >+int xt_condition_get_module_perms(char *buffer, struct kernel_param *kp) >+{ >+ struct condition_net *cond_net = >+ condition_pernet(current->nsproxy->net_ns); >+ >+ return sprintf(buffer, "%u", >+ *((u32 *) ((u8 *) cond_net + (size_t) kp->arg))); >+} >+EXPORT_SYMBOL_GPL(xt_condition_get_module_perms); >+ >+module_param_call(list_perms, >+ xt_condition_set_module_perms, >+ xt_condition_get_module_perms, >+ (void *) offsetof(struct condition_net, list_perms), >+ 0600); >+module_param_call(uid_perms, >+ xt_condition_set_module_perms, >+ xt_condition_get_module_perms, >+ (void *) offsetof(struct condition_net, uid_perms), >+ 0600); >+module_param_call(gid_perms, >+ xt_condition_set_module_perms, >+ xt_condition_get_module_perms, >+ (void *) offsetof(struct condition_net, gid_perms), >+ 0600); >+ >+module_init(condition_mt_init); >+module_exit(condition_mt_exit); >-- >1.7.0.4 > -- 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 Thu, 2010-07-22 at 16:44 +0200, ext Jan Engelhardt wrote: > On Thursday 2010-07-22 16:09, Luciano Coelho wrote: > >+static int condition_mt_check(const struct xt_mtchk_param *par) > >+{ > >+ struct xt_condition_mtinfo *info = par->matchinfo; > >+ struct condition_variable *var; > >+ struct condition_net *cond_net = > >+ condition_pernet(current->nsproxy->net_ns); > > Cc'ing Alexey who has done the netns support. > > Alexey, you added par->net, but given Luciano just did it with > current->nsproxy->net_ns, do we really need par->net? > > > >+int xt_condition_set_module_perms(const char *val, struct kernel_param *kp) > >+{ > >+ unsigned long l; > >+ int ret; > >+ struct condition_net *cond_net = > >+ condition_pernet(current->nsproxy->net_ns); > >+ > >+ if (!val) return -EINVAL; > > newline before return. Sure! I copied this from params.c. I'll fix it. > >+ ret = strict_strtoul(val, 0, &l); > >+ if (ret == -EINVAL || ((uint)l != l)) > >+ return -EINVAL; > > >+ *((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l; > > I don't think we need this level of granularity; let the options be > global, similar to what xt_hashlimit does. I did this according to Patrick's comment: > > proc_net_condition is a global variable, so this won't work for > > namespaces. What the code does is reinitialize it when instantiating > > a new namespace, so it will always point to the last instantiated > > namespace. > > > > The same problem exists for the condition_list, each namespace > > should only be able to access its own conditions. > > This also applies to the permission variables. Basically, we shouldn't > be having any globals except perhaps the mutex. You probably need a > module_param_call function to set them for the correct namespace (you > can access that through current->nsproxy->net_ns). I found it a bit strange to be able to change the module params in a per-netns basis, but it is actually possible if you're changing the parameters via sysfs. I tried it and it even seems to work. ;) I can't see any module parameters in the xt_hashlimit.c file. Am I looking in the wrong place? I would be fine with making the module params global (as they were before), if that's fine with Patrick too. > (I am not even sure if kp->arg can be non-multiples-of-4, in which case > this would be an alignment violation even.) I'm passing size_t in kp->arg. It looks quite ugly, because usually kp->arg is a pointer to some data. But at least this way, using offsetof(), I could avoid lots of repeated code for the options...
On Thursday 2010-07-22 17:16, Luciano Coelho wrote: >> >+ ret = strict_strtoul(val, 0, &l); >> >+ if (ret == -EINVAL || ((uint)l != l)) >> >+ return -EINVAL; >> >> >+ *((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l; >> >> I don't think we need this level of granularity; let the options be >> global, similar to what xt_hashlimit does. > >I did this according to Patrick's comment: >> > proc_net_condition is a global variable, so this won't work for >> > namespaces. What the code does is reinitialize it when instantiating >> > a new namespace, so it will always point to the last instantiated >> > namespace. >> > >> > The same problem exists for the condition_list, each namespace >> > should only be able to access its own conditions. >> >> This also applies to the permission variables. Basically, we shouldn't >> be having any globals except perhaps the mutex. You probably need a >> module_param_call function to set them for the correct namespace (you >> can access that through current->nsproxy->net_ns). > >I found it a bit strange to be able to change the module params in a >per-netns basis, but it is actually possible if you're changing the >parameters via sysfs. I tried it and it even seems to work. ;) > >I can't see any module parameters in the xt_hashlimit.c file. Am I >looking in the wrong place? Oops, xt_recent.c. >I would be fine with making the module params global (as they were >before), if that's fine with Patrick too. "When was the last time you needed to change the default ownership when you _also_ have the possibility to chown each procfs file individually?" >> (I am not even sure if kp->arg can be non-multiples-of-4, in which case >> this would be an alignment violation even.) > >I'm passing size_t in kp->arg. It looks quite ugly, because usually >kp->arg is a pointer to some data. But at least this way, using >offsetof(), I could avoid lots of repeated code for the options... if kp->arg is 1, ((u8*)cond_net + kp->arg) yields a pointer that's usually not aligned for u32. (And C pedants would probably argue that is should be char* not u8*, even if the one is a typedef of another.) -- 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 Thu, Jul 22, 2010 at 04:44:35PM +0200, Jan Engelhardt wrote: > > On Thursday 2010-07-22 16:09, Luciano Coelho wrote: > >+static int condition_mt_check(const struct xt_mtchk_param *par) > >+{ > >+ struct xt_condition_mtinfo *info = par->matchinfo; > >+ struct condition_variable *var; > >+ struct condition_net *cond_net = > >+ condition_pernet(current->nsproxy->net_ns); > > Cc'ing Alexey who has done the netns support. > > Alexey, you added par->net, but given Luciano just did it with > current->nsproxy->net_ns, do we really need par->net? In ->check, maybe, we can get away with current->nsproxy->net_ns. But definitely not in ->destroy(), because destruction can happen when _no_ task is in netns, so current->nsproxy->net_ns is 100% bogus. Steps to reproduce: iptables -A ... exit ->destroy hook gets netns from par->net, ->checkentry does the same for symmetry and less confusion. -- 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 Thu, 2010-07-22 at 17:36 +0200, ext Jan Engelhardt wrote: > On Thursday 2010-07-22 17:16, Luciano Coelho wrote: > >> >+ ret = strict_strtoul(val, 0, &l); > >> >+ if (ret == -EINVAL || ((uint)l != l)) > >> >+ return -EINVAL; > >> > >> >+ *((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l; > >> > >> I don't think we need this level of granularity; let the options be > >> global, similar to what xt_hashlimit does. > > > >I did this according to Patrick's comment: > >> > proc_net_condition is a global variable, so this won't work for > >> > namespaces. What the code does is reinitialize it when instantiating > >> > a new namespace, so it will always point to the last instantiated > >> > namespace. > >> > > >> > The same problem exists for the condition_list, each namespace > >> > should only be able to access its own conditions. > >> > >> This also applies to the permission variables. Basically, we shouldn't > >> be having any globals except perhaps the mutex. You probably need a > >> module_param_call function to set them for the correct namespace (you > >> can access that through current->nsproxy->net_ns). > > > >I found it a bit strange to be able to change the module params in a > >per-netns basis, but it is actually possible if you're changing the > >parameters via sysfs. I tried it and it even seems to work. ;) > > > >I can't see any module parameters in the xt_hashlimit.c file. Am I > >looking in the wrong place? > > Oops, xt_recent.c. > > >I would be fine with making the module params global (as they were > >before), if that's fine with Patrick too. > > "When was the last time you needed to change the default ownership > when you _also_ have the possibility to chown each procfs file > individually?" > > >> (I am not even sure if kp->arg can be non-multiples-of-4, in which case > >> this would be an alignment violation even.) > > > >I'm passing size_t in kp->arg. It looks quite ugly, because usually > >kp->arg is a pointer to some data. But at least this way, using > >offsetof(), I could avoid lots of repeated code for the options... > > if kp->arg is 1, ((u8*)cond_net + kp->arg) yields a pointer that's > usually not aligned for u32. (And C pedants would probably argue > that is should be char* not u8*, even if the one is a typedef > of another.) Oh, I see. In the current case it works because all the parameters are 32-bit. In any case, I'll just remove the per-net module parameters code. It is indeed over-fine-grained as you said earlier.
On Thu, 2010-07-22 at 21:19 +0200, ext Alexey Dobriyan wrote: > On Thu, Jul 22, 2010 at 04:44:35PM +0200, Jan Engelhardt wrote: > > > > On Thursday 2010-07-22 16:09, Luciano Coelho wrote: > > >+static int condition_mt_check(const struct xt_mtchk_param *par) > > >+{ > > >+ struct xt_condition_mtinfo *info = par->matchinfo; > > >+ struct condition_variable *var; > > >+ struct condition_net *cond_net = > > >+ condition_pernet(current->nsproxy->net_ns); > > > > Cc'ing Alexey who has done the netns support. > > > > Alexey, you added par->net, but given Luciano just did it with > > current->nsproxy->net_ns, do we really need par->net? > > In ->check, maybe, we can get away with current->nsproxy->net_ns. > > But definitely not in ->destroy(), because destruction can happen > when _no_ task is in netns, so current->nsproxy->net_ns is 100% bogus. > > Steps to reproduce: > iptables -A ... > exit > > ->destroy hook gets netns from par->net, ->checkentry does the same > for symmetry and less confusion. Very good point. I guess that when Patrick suggested using current->nsproxy->net_ns, he meant only for the module_params part. I'll be removing that anyway. And I'll change the code to use par->net instead of current->nsproxy->net_ns to avoid the problem in _destroy. Thanks for your comments! I must admit that I was a bit insecure about this code. That's why I sent a RFC early enough. ;)
diff --git a/include/linux/netfilter/Kbuild b/include/linux/netfilter/Kbuild index bb103f4..d873f67 100644 --- a/include/linux/netfilter/Kbuild +++ b/include/linux/netfilter/Kbuild @@ -20,6 +20,7 @@ header-y += xt_TCPOPTSTRIP.h header-y += xt_TEE.h header-y += xt_TPROXY.h header-y += xt_comment.h +header-y += xt_condition.h header-y += xt_connbytes.h header-y += xt_connlimit.h header-y += xt_connmark.h diff --git a/include/linux/netfilter/xt_condition.h b/include/linux/netfilter/xt_condition.h new file mode 100644 index 0000000..4faf3ca --- /dev/null +++ b/include/linux/netfilter/xt_condition.h @@ -0,0 +1,14 @@ +#ifndef _XT_CONDITION_H +#define _XT_CONDITION_H + +#include <linux/types.h> + +struct xt_condition_mtinfo { + char name[31]; + __u8 invert; + + /* Used internally by the kernel */ + void *condvar __attribute__((aligned(8))); +}; + +#endif /* _XT_CONDITION_H */ diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig index aa2f106..8c114b8 100644 --- a/net/netfilter/Kconfig +++ b/net/netfilter/Kconfig @@ -605,6 +605,14 @@ config NETFILTER_XT_MATCH_COMMENT If you want to compile it as a module, say M here and read <file:Documentation/kbuild/modules.txt>. If unsure, say `N'. +config NETFILTER_XT_MATCH_CONDITION + tristate '"condition" match support' + depends on NETFILTER_ADVANCED + depends on PROC_FS + ---help--- + This option allows you to match firewall rules against condition + variables stored in the /proc/net/nf_condition directory. + config NETFILTER_XT_MATCH_CONNBYTES tristate '"connbytes" per-connection counter match support' depends on NF_CONNTRACK diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile index e28420a..474dd06 100644 --- a/net/netfilter/Makefile +++ b/net/netfilter/Makefile @@ -66,6 +66,7 @@ obj-$(CONFIG_NETFILTER_XT_TARGET_IDLETIMER) += xt_IDLETIMER.o # matches obj-$(CONFIG_NETFILTER_XT_MATCH_CLUSTER) += xt_cluster.o obj-$(CONFIG_NETFILTER_XT_MATCH_COMMENT) += xt_comment.o +obj-$(CONFIG_NETFILTER_XT_MATCH_CONDITION) += xt_condition.o obj-$(CONFIG_NETFILTER_XT_MATCH_CONNBYTES) += xt_connbytes.o obj-$(CONFIG_NETFILTER_XT_MATCH_CONNLIMIT) += xt_connlimit.o obj-$(CONFIG_NETFILTER_XT_MATCH_CONNTRACK) += xt_conntrack.o diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c new file mode 100644 index 0000000..162aa60 --- /dev/null +++ b/net/netfilter/xt_condition.c @@ -0,0 +1,294 @@ +/* + * "condition" match extension for Xtables + * + * Description: This module allows firewall rules to match using + * condition variables available through procfs. + * + * Authors: + * Stephane Ouellette <ouellettes@videotron.ca>, 2002-10-22 + * Massimiliano Hofer <max@nucleus.it>, 2006-05-15 + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License; either version 2 + * or 3 of the License, as published by the Free Software Foundation. + */ +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#include <linux/kernel.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/proc_fs.h> +#include <linux/spinlock.h> +#include <linux/string.h> +#include <linux/version.h> +#include <linux/nsproxy.h> +#include <linux/netfilter/x_tables.h> +#include <linux/netfilter/xt_condition.h> +#include <net/netns/generic.h> +#include <asm/uaccess.h> + +MODULE_AUTHOR("Stephane Ouellette <ouellettes@videotron.ca>"); +MODULE_AUTHOR("Massimiliano Hofer <max@nucleus.it>"); +MODULE_AUTHOR("Jan Engelhardt <jengelh@medozas.de>"); +MODULE_DESCRIPTION("Allows rules to match against condition variables"); +MODULE_LICENSE("GPL"); +MODULE_PARM_DESC(condition_list_perms, "default permissions on /proc/net/nf_condition/* files"); +MODULE_PARM_DESC(condition_uid_perms, "default user owner of /proc/net/nf_condition/* files"); +MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condition/* files"); +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; +}; + +struct condition_net { + struct list_head list; + struct proc_dir_entry *proc_dir; + unsigned int list_perms; + unsigned int uid_perms; + unsigned int gid_perms; +}; + +static int condition_net_id; +static inline struct condition_net *condition_pernet(struct net *net) +{ + return net_generic(net, condition_net_id); +} + +/* proc_lock is a user context only semaphore used for write access */ +/* to the conditions' list. */ +static DEFINE_MUTEX(proc_lock); + +static int condition_proc_read(char __user *buffer, char **start, off_t offset, + int length, int *eof, void *data) +{ + const struct condition_variable *var = data; + + buffer[0] = var->enabled ? '1' : '0'; + buffer[1] = '\n'; + if (length >= 2) + *eof = true; + return 2; +} + +static int condition_proc_write(struct file *file, const char __user *buffer, + unsigned long length, void *data) +{ + struct condition_variable *var = data; + char newval; + + if (length > 0) { + if (get_user(newval, buffer) != 0) + return -EFAULT; + /* Match only on the first character */ + switch (newval) { + case '0': + var->enabled = false; + break; + case '1': + var->enabled = true; + break; + } + } + return length; +} + +static bool +condition_mt(const struct sk_buff *skb, struct xt_action_param *par) +{ + const struct xt_condition_mtinfo *info = par->matchinfo; + const struct condition_variable *var = info->condvar; + + return var->enabled ^ info->invert; +} + +static int condition_mt_check(const struct xt_mtchk_param *par) +{ + struct xt_condition_mtinfo *info = par->matchinfo; + struct condition_variable *var; + struct condition_net *cond_net = + condition_pernet(current->nsproxy->net_ns); + + /* 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, &cond_net->list, list) { + if (strcmp(info->name, var->status_proc->name) == 0) { + ++var->refcount; + mutex_unlock(&proc_lock); + info->condvar = var; + return 0; + } + } + + /* 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; + } + + /* Create the condition variable's proc file entry. */ + var->status_proc = create_proc_entry(info->name, + cond_net->list_perms, + cond_net->proc_dir); + if (var->status_proc == NULL) { + kfree(var); + mutex_unlock(&proc_lock); + return -ENOMEM; + } + + var->refcount = 1; + var->enabled = false; + var->status_proc->data = var; + var->status_proc->read_proc = condition_proc_read; + var->status_proc->write_proc = condition_proc_write; + var->status_proc->uid = cond_net->uid_perms; + var->status_proc->gid = cond_net->gid_perms; + list_add(&var->list, &cond_net->list); + mutex_unlock(&proc_lock); + info->condvar = var; + return 0; +} + +static void condition_mt_destroy(const struct xt_mtdtor_param *par) +{ + const struct xt_condition_mtinfo *info = par->matchinfo; + struct condition_variable *var = info->condvar; + struct condition_net *cond_net = + condition_pernet(current->nsproxy->net_ns); + + mutex_lock(&proc_lock); + if (--var->refcount == 0) { + list_del(&var->list); + remove_proc_entry(var->status_proc->name, + cond_net->proc_dir); + mutex_unlock(&proc_lock); + kfree(var); + return; + } + mutex_unlock(&proc_lock); +} + +static struct xt_match condition_mt_reg __read_mostly = { + .name = "condition", + .revision = 1, + .family = NFPROTO_UNSPEC, + .matchsize = sizeof(struct xt_condition_mtinfo), + .match = condition_mt, + .checkentry = condition_mt_check, + .destroy = condition_mt_destroy, + .me = THIS_MODULE, +}; + +static const char *const dir_name = "nf_condition"; + +static int __net_init condnet_mt_init(struct net *net) +{ + struct condition_net *cond_net = condition_pernet(net); + + INIT_LIST_HEAD(&cond_net->list); + cond_net->list_perms = S_IRUSR | S_IWUSR; + cond_net->uid_perms = S_IRUSR | S_IWUSR; + cond_net->gid_perms = S_IRUSR | S_IWUSR; + + cond_net->proc_dir = proc_mkdir(dir_name, net->proc_net); + + return (cond_net->proc_dir == NULL) ? -EACCES : 0; +} + +static void __net_exit condnet_mt_exit(struct net *net) +{ + remove_proc_entry(dir_name, net->proc_net); +} + +static struct pernet_operations condition_mt_netops = { + .init = condnet_mt_init, + .exit = condnet_mt_exit, + .id = &condition_net_id, + .size = sizeof(struct condition_net), +}; + +static int __init condition_mt_init(void) +{ + int ret; + + mutex_init(&proc_lock); + ret = xt_register_match(&condition_mt_reg); + if (ret < 0) + return ret; + + ret = register_pernet_subsys(&condition_mt_netops); + if (ret < 0) { + xt_unregister_match(&condition_mt_reg); + return ret; + } + + return 0; +} + +static void __exit condition_mt_exit(void) +{ + unregister_pernet_subsys(&condition_mt_netops); + xt_unregister_match(&condition_mt_reg); +} + +int xt_condition_set_module_perms(const char *val, struct kernel_param *kp) +{ + unsigned long l; + int ret; + struct condition_net *cond_net = + condition_pernet(current->nsproxy->net_ns); + + if (!val) return -EINVAL; + ret = strict_strtoul(val, 0, &l); + if (ret == -EINVAL || ((uint)l != l)) + return -EINVAL; + *((u32 *) ((u8 *) cond_net + (size_t) kp->arg)) = l; + + return 0; +} +EXPORT_SYMBOL_GPL(xt_condition_set_module_perms); + +int xt_condition_get_module_perms(char *buffer, struct kernel_param *kp) +{ + struct condition_net *cond_net = + condition_pernet(current->nsproxy->net_ns); + + return sprintf(buffer, "%u", + *((u32 *) ((u8 *) cond_net + (size_t) kp->arg))); +} +EXPORT_SYMBOL_GPL(xt_condition_get_module_perms); + +module_param_call(list_perms, + xt_condition_set_module_perms, + xt_condition_get_module_perms, + (void *) offsetof(struct condition_net, list_perms), + 0600); +module_param_call(uid_perms, + xt_condition_set_module_perms, + xt_condition_get_module_perms, + (void *) offsetof(struct condition_net, uid_perms), + 0600); +module_param_call(gid_perms, + xt_condition_set_module_perms, + xt_condition_get_module_perms, + (void *) offsetof(struct condition_net, gid_perms), + 0600); + +module_init(condition_mt_init); +module_exit(condition_mt_exit);
xt_condition can be used by userspace to influence decisions in rules by means of togglable variables without having to reload the entire ruleset. This is a respin of the module in Xtables-addons, with support for multiple namespaces and other small improvements. Cc: Jan Engelhardt <jengelh@medozas.de> Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com> --- include/linux/netfilter/Kbuild | 1 + include/linux/netfilter/xt_condition.h | 14 ++ net/netfilter/Kconfig | 8 + net/netfilter/Makefile | 1 + net/netfilter/xt_condition.c | 294 ++++++++++++++++++++++++++++++++ 5 files changed, 318 insertions(+), 0 deletions(-) create mode 100644 include/linux/netfilter/xt_condition.h create mode 100644 net/netfilter/xt_condition.c