diff mbox

[2/2] netfilter: xt_condition: change the value from boolean to u32

Message ID 1281019269-30985-3-git-send-email-luciano.coelho@nokia.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Luciano Coelho Aug. 5, 2010, 2:41 p.m. UTC 
From: Luciano Coelho <luciano.coelho@nokia.com>

Previously the condition match was using boolean values for the condition,
which is a bit limited, so this patch changes the value to a u32.

This is not a problem at the moment, since only userspace can set the
condition and it can do more advanced checks before setting the value.  But
when the condition target gets implemented, having a u32 value will make it
much more flexible.

Signed-off-by: Luciano Coelho <luciano.coelho@nokia.com>
---
 include/linux/netfilter/xt_condition.h |    3 +-
 net/netfilter/xt_condition.c           |   51 +++++++++++++++++---------------
 2 files changed, 29 insertions(+), 25 deletions(-)

Comments

Jan Engelhardt Aug. 5, 2010, 3:12 p.m. UTC | #1 
On Thursday 2010-08-05 16:41, luciano.coelho@nokia.com wrote:

> struct xt_condition_mtinfo {
>-	char name[31];
>+	char name[27];
> 	__u8 invert;
>+	__u32 value;

Please also bump the .revision field to 2 with this patch so that
testing can always proceed without an ABI clash.
(rev 2 would then remain over the course of the remaining patches 
you submit.)
(rev 0 = ipt_condition/pom-ng; rev 1 = xt_condition from Xt-a)

>+	char buf[14];

	char buf[sizeof("4294967296")];

seems more intuitive :-)

>+	unsigned long long value;
>+
>+	if (length == 0)
>+		return 0;
>+
>+	if (length > sizeof(buf))
>+		return -EINVAL;
>+
>+	if (copy_from_user(buf, input, length) != 0)
>+		return -EFAULT;
>+
>+	buf[length - 1] = '\0';
>+
>+	if (strict_strtoull(buf, 0, &value) != 0)
>+		return -EINVAL;
>+
>+	if (value > (u32) value)
>+		return -EINVAL;

Is it possible to use just strict_strtoul?

>-	return var->enabled ^ info->invert;
>+	return (var->value == info->value) ^ info->invert;

Since the condition value (cdmark) was thought of an nfmark-style thing, 
would it perhaps make sense to model it after it

	return (var->value & ~info->mask) ^ info->value;

Other opinions?

--
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
Luciano Coelho Aug. 6, 2010, 8 a.m. UTC | #2 
On Thu, 2010-08-05 at 17:12 +0200, ext Jan Engelhardt wrote:
> On Thursday 2010-08-05 16:41, luciano.coelho@nokia.com wrote:
> 
> > struct xt_condition_mtinfo {
> >-	char name[31];
> >+	char name[27];
> > 	__u8 invert;
> >+	__u32 value;
> 
> Please also bump the .revision field to 2 with this patch so that
> testing can always proceed without an ABI clash.
> (rev 2 would then remain over the course of the remaining patches 
> you submit.)
> (rev 0 = ipt_condition/pom-ng; rev 1 = xt_condition from Xt-a)

Oh, did I forget that? I thought I had bumped it up in my previous patch
after your comment, but apparently I didn't.  I'll fix it.


> >+	char buf[14];
> 
> 	char buf[sizeof("4294967296")];
> 
> seems more intuitive :-)

I'm using base 0 in the strtoull call, so I need at least 14 chars to
accommodate octals.  In octal MAX_UINT takes 11 digits, plus the leading
0 that marks it as an octal, plus the optional + sign (not sure if this
one makes sense, though), plus the null-terminator (or actually the \n
when writing to procfs) leads to 14.  But I'm not sure whether:

char buf[sizeof("+037777777777")]; 

Would be very intuitive? Surely I don't like the magic number, so I
should probably change it anyway.

> >+	unsigned long long value;
> >+
> >+	if (length == 0)
> >+		return 0;
> >+
> >+	if (length > sizeof(buf))
> >+		return -EINVAL;
> >+
> >+	if (copy_from_user(buf, input, length) != 0)
> >+		return -EFAULT;
> >+
> >+	buf[length - 1] = '\0';
> >+
> >+	if (strict_strtoull(buf, 0, &value) != 0)
> >+		return -EINVAL;
> >+
> >+	if (value > (u32) value)
> >+		return -EINVAL;
> 
> Is it possible to use just strict_strtoul?

Not easily.  I found that there is a bug in strtoul (and strtoull for
that matter) that causes the long to overflow if there are valid digits
after the maximum possible digits for the base.  For example if you try
to strtoul 0xfffffffff (with 9 f's) the strtoul will overflow and come
up with a bogus result.  I can't easily truncate the string to avoid
this problem, because with decimal or octal, the same valid value would
take more spaces.  I could do some magic here, checking whether it's a
hex, dec or oct and truncate appropriately, but that would be very ugly.

So the simplest way I came up with was to use strtoull and return
-EINVAL if the value exceeds 32 bits. ;)


> >-	return var->enabled ^ info->invert;
> >+	return (var->value == info->value) ^ info->invert;
> 
> Since the condition value (cdmark) was thought of an nfmark-style thing, 
> would it perhaps make sense to model it after it
> 
> 	return (var->value & ~info->mask) ^ info->value;
> 
> Other opinions?

I think it's nicer to have it as a normal equals here for now and then
extend the match with more operations.  We can later add, for example,
an --and option to the condition match in order to do other kinds of
binary operations.  It would be more flexible this way because we could
use several different types of comparisons, wouldn't it? And in the
target we could have several different types of operations.
Jan Engelhardt Aug. 6, 2010, 8:52 a.m. UTC | #3 
On Friday 2010-08-06 10:00, Luciano Coelho wrote:
>> >+	buf[length - 1] = '\0';
>> >+
>> >+	if (strict_strtoull(buf, 0, &value) != 0)
>> >+		return -EINVAL;
>> >+
>> >+	if (value > (u32) value)
>> >+		return -EINVAL;
>> 
>> Is it possible to use just strict_strtoul?
>
>Not easily.  I found that there is a bug in strtoul (and strtoull for
>that matter) that causes the long to overflow if there are valid digits
>after the maximum possible digits for the base.  For example if you try
>to strtoul 0xfffffffff (with 9 f's) the strtoul will overflow and come
>up with a bogus result.

I see. Strange that no one has adressed this yet - I mean, writing
a just-too-large value into a procfs/sysfs file and thus effectively
causing a bogus value to be actually written isn't quite so thrilling
as things go haywire.

>I can't easily truncate the string to avoid
>this problem, because with decimal or octal, the same valid value would
>take more spaces.  I could do some magic here, checking whether it's a
>hex, dec or oct and truncate appropriately, but that would be very ugly.
>
>So the simplest way I came up with was to use strtoull and return
>-EINVAL if the value exceeds 32 bits. ;)

If I read strtoul(3) right, ERANGE is used for "out of range".

>> Since the condition value (cdmark) was thought of an nfmark-style thing, 
>> would it perhaps make sense to model it after it
>> 
>> 	return (var->value & ~info->mask) ^ info->value;
>> 
>> Other opinions?
>
>I think it's nicer to have it as a normal equals here for now and then
>extend the match with more operations.  We can later add, for example,
>an --and option to the condition match in order to do other kinds of
>binary operations.  It would be more flexible this way because we could
>use several different types of comparisons, wouldn't it? And in the
>target we could have several different types of operations.

Indeed.
--
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
Luciano Coelho Aug. 6, 2010, 10:34 a.m. UTC | #4 
On Fri, 2010-08-06 at 10:52 +0200, ext Jan Engelhardt wrote:
> On Friday 2010-08-06 10:00, Luciano Coelho wrote:
> >> >+	buf[length - 1] = '\0';
> >> >+
> >> >+	if (strict_strtoull(buf, 0, &value) != 0)
> >> >+		return -EINVAL;
> >> >+
> >> >+	if (value > (u32) value)
> >> >+		return -EINVAL;
> >> 
> >> Is it possible to use just strict_strtoul?
> >
> >Not easily.  I found that there is a bug in strtoul (and strtoull for
> >that matter) that causes the long to overflow if there are valid digits
> >after the maximum possible digits for the base.  For example if you try
> >to strtoul 0xfffffffff (with 9 f's) the strtoul will overflow and come
> >up with a bogus result.
> 
> I see. Strange that no one has adressed this yet - I mean, writing
> a just-too-large value into a procfs/sysfs file and thus effectively
> causing a bogus value to be actually written isn't quite so thrilling
> as things go haywire.

Yes, I was really surprised to see this happening when I was testing the
limits.  And I was even more surprised when I checked the strtoull code
and saw that it is broken.


> >I can't easily truncate the string to avoid
> >this problem, because with decimal or octal, the same valid value would
> >take more spaces.  I could do some magic here, checking whether it's a
> >hex, dec or oct and truncate appropriately, but that would be very ugly.
> >
> >So the simplest way I came up with was to use strtoull and return
> >-EINVAL if the value exceeds 32 bits. ;)
> 
> If I read strtoul(3) right, ERANGE is used for "out of range".

Yes, libc's strtoul returns ERANGE in that case.  strict_strtoul() in
the kernel code doesn't.  I'll change my code to return -ERANGE here
too, for consistency.


> >> Since the condition value (cdmark) was thought of an nfmark-style thing, 
> >> would it perhaps make sense to model it after it
> >> 
> >> 	return (var->value & ~info->mask) ^ info->value;
> >> 
> >> Other opinions?
> >
> >I think it's nicer to have it as a normal equals here for now and then
> >extend the match with more operations.  We can later add, for example,
> >an --and option to the condition match in order to do other kinds of
> >binary operations.  It would be more flexible this way because we could
> >use several different types of comparisons, wouldn't it? And in the
> >target we could have several different types of operations.
> 
> Indeed.
diff mbox

Patch

diff --git a/include/linux/netfilter/xt_condition.h b/include/linux/netfilter/xt_condition.h
index 4faf3ca..c4fe899 100644
--- a/include/linux/netfilter/xt_condition.h
+++ b/include/linux/netfilter/xt_condition.h
@@ -4,8 +4,9 @@ 
 #include <linux/types.h>
 
 struct xt_condition_mtinfo {
-	char name[31];
+	char name[27];
 	__u8 invert;
+	__u32 value;
 
 	/* Used internally by the kernel */
 	void *condvar __attribute__((aligned(8)));
diff --git a/net/netfilter/xt_condition.c b/net/netfilter/xt_condition.c
index a78d832..08dfb67 100644
--- a/net/netfilter/xt_condition.c
+++ b/net/netfilter/xt_condition.c
@@ -48,7 +48,7 @@  struct condition_variable {
 	struct list_head list;
 	struct proc_dir_entry *status_proc;
 	unsigned int refcount;
-	bool enabled;
+	u32 value;
 };
 
 struct condition_net {
@@ -71,32 +71,35 @@  static int condition_proc_read(char __user *buffer, char **start, off_t offset,
 {
 	const struct condition_variable *var = data;
 
-	buffer[0] = var->enabled ? '1' : '0';
-	buffer[1] = '\n';
-	if (length >= 2)
-		*eof = true;
-	return 2;
+	return snprintf(buffer, length, "%u\n", var->value);
 }
 
-static int condition_proc_write(struct file *file, const char __user *buffer,
+static int condition_proc_write(struct file *file, const char __user *input,
 				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;
-		}
-	}
+	char buf[14];
+	unsigned long long value;
+
+	if (length == 0)
+		return 0;
+
+	if (length > sizeof(buf))
+		return -EINVAL;
+
+	if (copy_from_user(buf, input, length) != 0)
+		return -EFAULT;
+
+	buf[length - 1] = '\0';
+
+	if (strict_strtoull(buf, 0, &value) != 0)
+		return -EINVAL;
+
+	if (value > (u32) value)
+		return -EINVAL;
+
+	var->value = value;
+
 	return length;
 }
 
@@ -106,7 +109,7 @@  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;
+	return (var->value == info->value) ^ info->invert;
 }
 
 static int condition_mt_check(const struct xt_mtchk_param *par)
@@ -156,7 +159,7 @@  static int condition_mt_check(const struct xt_mtchk_param *par)
 	}
 
 	var->refcount = 1;
-	var->enabled  = false;
+	var->value = 0;
 	var->status_proc->data       = var;
 	var->status_proc->read_proc  = condition_proc_read;
 	var->status_proc->write_proc = condition_proc_write;