From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luciano Coelho Subject: Re: [PATCH RESEND 1/3] netfilter: xtables: inclusion of xt_condition Date: Tue, 21 Sep 2010 22:59:45 +0300 Message-ID: <1285099185.10579.17.camel@powerslave> References: <1282034213-1829-1-git-send-email-luciano.coelho@nokia.com> <1282034213-1829-2-git-send-email-luciano.coelho@nokia.com> <4C912EFC.9070406@trash.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: "netfilter-devel@vger.kernel.org" , "netdev@vger.kernel.org" , "jengelh@medozas.de" , "sameo@linux.intel.com" , "Ylalehto Janne (Nokia-MS/Tampere)" To: ext Patrick McHardy Return-path: Received: from smtp.nokia.com ([192.100.122.233]:61385 "EHLO mgw-mx06.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758157Ab0IUUAJ (ORCPT ); Tue, 21 Sep 2010 16:00:09 -0400 In-Reply-To: <4C912EFC.9070406@trash.net> Sender: netfilter-devel-owner@vger.kernel.org List-ID: On Wed, 2010-09-15 at 22:39 +0200, ext Patrick McHardy wrote: > Am 17.08.2010 10:36, schrieb Luciano Coelho: > > diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c > > new file mode 100644 > > index 0000000..a78d832 > > --- /dev/null > > +++ b/net/netfilter/xt_condition.c > > @@ -0,0 +1,265 @@ > > +/* > > + * "condition" match extension for Xtables > > + * > > + * Description: This module allows firewall rules to match using > > + * condition variables available through procfs. > > + * > > + * Authors: > > + * Stephane Ouellette , 2002-10-22 > > + * Massimiliano Hofer , 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +/* Defaults, these can be overridden on the module command-line. */ > > +static unsigned int condition_list_perms = S_IRUGO | S_IWUSR; > > +static unsigned int condition_uid_perms = 0; > > +static unsigned int condition_gid_perms = 0; > > I'm not sure whether we already discussed this, but this isn't > useful if namespaces are used since the IDs aren't global. As Jan suggested, I think it would be better to include it like this, since other modules already do similar things, and then I'll later send a new patch with support for multiple namespaces. > > + > > +MODULE_AUTHOR("Stephane Ouellette "); > > +MODULE_AUTHOR("Massimiliano Hofer "); > > +MODULE_AUTHOR("Jan Engelhardt "); > > +MODULE_DESCRIPTION("Allows rules to match against condition variables"); > > +MODULE_LICENSE("GPL"); > > +module_param(condition_list_perms, uint, S_IRUSR | S_IWUSR); > > +MODULE_PARM_DESC(condition_list_perms, "default permissions on /proc/net/nf_condition/* files"); > > +module_param(condition_uid_perms, uint, S_IRUSR | S_IWUSR); > > +MODULE_PARM_DESC(condition_uid_perms, "default user owner of /proc/net/nf_condition/* files"); > > +module_param(condition_gid_perms, uint, S_IRUSR | S_IWUSR); > > +MODULE_PARM_DESC(condition_gid_perms, "default group owner of /proc/net/nf_condition/* files"); > > +MODULE_ALIAS("ipt_condition"); > > +MODULE_ALIAS("ip6t_condition"); > > + > > > +/* proc_lock is a user context only semaphore used for write access */ > > +/* to the conditions' list. */ > > +static DEFINE_MUTEX(proc_lock); > > Why is this called proc_lock, as the comment states it protects > the condition variable list? The comment is also misleading since > this is a mutex and not a semaphore. I'd suggest list_mutex or > condition_mutex. I'll fix this. > > +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'; > > This is a user buffer, you need copy_to_user(). Also please run > sparse against your code to check for similar errors. Oooff, I actually also noticed this when integrating into our internal tree. I'll fix this and run sparse properly before resending. -- Cheers, Luca.