sparc64 match module - bug id 94

sparc64 match module - bug id 94

[dan radom]

Is there plans to ever fix this problem?  There hasn't been any updates
to the bug's bugzilla page since the bug was reported.

Re: sparc64 match module - bug id 94

[Antony Stone]

On Tuesday 04 May 2004 1:14 pm, dan radom wrote:

> Is there plans to ever fix this problem?  There hasn't been any updates
> to the bug's bugzilla page since the bug was reported.

I suspect you may get a better response asking the netfilter development list 
about this, rather than the users' list.

Regards,

Antony.

-- 
"Reports that say that something hasn't happened are always interesting to me, 
because as we know, there are known knowns; there are things we know we know. 
We also know there are known unknowns; that is to say we know there are some 
things we do not know. But there are also unknown unknowns - the ones we 
don't know we don't know."

 - Donald Rumsfeld, US Secretary of Defence

                                                     Please reply to the list;
                                                           please don't CC me.

sparc64 match module - bug id 94

[dan radom]

Hi,

This bug has been open for quite some time, and bugzilla doesn't really
shows much beyond the initial bug report.  Is this something that will
ever work?  I would love to use this feature on sparc64.

Re: sparc64 match module - bug id 94

[Henrik Nordstrom]

On Wed, 5 May 2004, dan radom wrote:

> This bug has been open for quite some time, and bugzilla doesn't really
> shows much beyond the initial bug report.  Is this something that will
> ever work?  I would love to use this feature on sparc64.

It requires some clever guy with a sparc64 to come up with a scheme how to 
make user and kernel data types compatible in a 32/64 split user/kernel 
environment.

Regards
Henrik

Re: sparc64 match module - bug id 94

[KOVACS Krisztian]

  Hi,

2004-05-06, cs keltezéssel 12:12-kor Henrik Nordstrom ezt írta:
> > This bug has been open for quite some time, and bugzilla doesn't really
> > shows much beyond the initial bug report.  Is this something that will
> > ever work?  I would love to use this feature on sparc64.
> 
> It requires some clever guy with a sparc64 to come up with a scheme how to 
> make user and kernel data types compatible in a 32/64 split user/kernel 
> environment.

  I think the main problem is that there is no easy way to store
per-match internal data, so it is stored in the matchinfo structure
describing the match itself. If the data describing the match itself
(which is copied from userspace) and data used for in-kernel status
could be separated, problems like these didn't exist at all...

  A possible hack: let's define two separate structures, one for
in-kernel use, and one for the userspace. The current ipt_rateinfo
structure looks like this:

struct ipt_rateinfo {
	u_int32_t avg;    /* Average secs between packets * scale */
	u_int32_t burst;  /* Period multiplier for upper limit. */

	/* Used internally by the kernel */
	unsigned long prev;
	u_int32_t credit;
	u_int32_t credit_cap, cost;

	/* Ugly, ugly fucker. */
	struct ipt_rateinfo *master;
};

  The problem point is *master, which, however, is for in-kernel use
only. What if we don't export that to userspace, ie. we define
ipt_rateinfo to not include that field, and use the current structure
internally only? In this case, the structure size check in checkentry()
would check if matchsize equals the size of the structure used by
userspace, but would cast matchinfo to the internal one.

  I know, I know, it's an ugly hack. However, I still think that
in-kernel data should not be stored in the structure describing the
match itself.

-- 
 Regards,
   Krisztian KOVACS

Re: sparc64 match module - bug id 94

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 391 bytes --]

Hi everyone,

dan radom wrote:

>Hi,
>
>This bug has been open for quite some time, and bugzilla doesn't really
>shows much beyond the initial bug report.  Is this something that will
>ever work?  I would love to use this feature on sparc64.
>  
>

Please, if I'm missing something, kill me softly. Is there anything 
wrong in fixing up the problem with the following patch?

regards,
Pablo

[-- Attachment #2: ipt_limit-fixup.patch --]
[-- Type: text/plain, Size: 1821 bytes --]

diff -u -r1.1.1.1 ipt_limit.c
--- a/net/ipv4/netfilter/ipt_limit.c	5 May 2004 01:30:27 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ipt_limit.c	6 May 2004 14:16:57 -0000
@@ -31,6 +31,10 @@
 
 static spinlock_t limit_lock = SPIN_LOCK_UNLOCKED;
 
+/* For SMP machines: Since every processor has a copy of the structure
+ * ipt_rateinfo, we must only work with one of them. */
+static struct ipt_rateinfo *master;
+
 /* Rusty: This is my (non-mathematically-inclined) understanding of
    this algorithm.  The `average rate' in jiffies becomes your initial
    amount of credit `credit' and the most credit you can ever have
@@ -70,17 +74,16 @@
 		int offset,
 		int *hotdrop)
 {
-	struct ipt_rateinfo *r = ((struct ipt_rateinfo *)matchinfo)->master;
 	unsigned long now = jiffies;
 
 	spin_lock_bh(&limit_lock);
-	r->credit += (now - xchg(&r->prev, now)) * CREDITS_PER_JIFFY;
-	if (r->credit > r->credit_cap)
-		r->credit = r->credit_cap;
+	master->credit += (now - xchg(&master->prev, now)) * CREDITS_PER_JIFFY;
+	if (master->credit > master->credit_cap)
+		master->credit = master->credit_cap;
 
-	if (r->credit >= r->cost) {
+	if (master->credit >= master->cost) {
 		/* We're not limited. */
-		r->credit -= r->cost;
+		master->credit -= master->cost;
 		spin_unlock_bh(&limit_lock);
 		return 1;
 	}
@@ -129,7 +132,7 @@
 	r->cost = user2credits(r->avg);
 
 	/* For SMP, we only want to use one set of counters. */
-	r->master = r;
+	master = r;
 
 	return 1;
 }
diff -u -r1.1.1.1 ipt_limit.h
--- a/include/linux/netfilter_ipv4/ipt_limit.h	5 May 2004 01:33:05 -0000	1.1.1.1
+++ b/include/linux/netfilter_ipv4/ipt_limit.h	6 May 2004 14:17:22 -0000
@@ -14,8 +14,5 @@
 	unsigned long prev;
 	u_int32_t credit;
 	u_int32_t credit_cap, cost;
-
-	/* Ugly, ugly fucker. */
-	struct ipt_rateinfo *master;
 };
 #endif /*_IPT_RATE_H*/

Re: sparc64 match module - bug id 94

[Pablo Neira]

Hi Chris,

Chris Wilson wrote:

>>Please, if I'm missing something, kill me softly. Is there anything 
>>wrong in fixing up the problem with the following patch?
>>    
>>
>
>Yes, unfortunately. The rate limit is supposed to be per-rule, not global. 
>If you do this then you can't really use more than one limit rule 
>in your ruleset, or you will get very unpredictable results (and very 
>different from the current behaviour).
>  
>

Oh, sure, I missed that. It's not so easy to fix up.

thanks!
Pablo

Re: sparc64 match module - bug id 94

[Chris Wilson]

Hi Pablo,

> Please, if I'm missing something, kill me softly. Is there anything 
> wrong in fixing up the problem with the following patch?

Yes, unfortunately. The rate limit is supposed to be per-rule, not global. 
If you do this then you can't really use more than one limit rule 
in your ruleset, or you will get very unpredictable results (and very 
different from the current behaviour).

Cheers, Chris.
-- 
_  __ __     _
 / __/ / ,__(_)_  | Chris Wilson -- UNIX Firewall Lead Developer |
/ (_  ,\/ _/ /_ \ | NetServers.co.uk http://www.netservers.co.uk |
\__/_/_/_//_/___/ | 21 Signet Court, Cambridge, UK. 01223 576516 |

Re: sparc64 match module - bug id 94

[Pablo Neira]

Hi Krisztian,

KOVACS Krisztian wrote:

>  A possible hack: let's define two separate structures, one for
>in-kernel use, and one for the userspace.
>  
>

I thought about that possibility, it's an interesting hack, but I think 
that the problem is much more specific, we have only this problem with 
ipt_limit. Actually, when we have some internal match information which 
is shared by some processors, we need to know which one is the selected 
to work with. So, we could add a pointer to the master of internal match 
data in struct ipt_entry_match. what do you think?

regards,
Pablo

Re: sparc64 match module - bug id 94

[Jozsef Kadlecsik]

On Thu, 6 May 2004, KOVACS Krisztian wrote:

> struct ipt_rateinfo {
> 	u_int32_t avg;    /* Average secs between packets * scale */
> 	u_int32_t burst;  /* Period multiplier for upper limit. */
>
> 	/* Used internally by the kernel */
> 	unsigned long prev;
> 	u_int32_t credit;
> 	u_int32_t credit_cap, cost;
>
> 	/* Ugly, ugly fucker. */
> 	struct ipt_rateinfo *master;
> };
>
>   The problem point is *master, which, however, is for in-kernel use
> only.

Just a thought: in 'pool' there was a similar problem and it was solved in
'set' by keeping the kernel info structures in an array. Then instead of
the pointer like above, the user part stored the index of the
corresponding structure in the array.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: sparc64 match module - bug id 94

[Henrik Nordstrom]

On Thu, 6 May 2004, Jozsef Kadlecsik wrote:

> Just a thought: in 'pool' there was a similar problem and it was solved in
> 'set' by keeping the kernel info structures in an array. Then instead of
> the pointer like above, the user part stored the index of the
> corresponding structure in the array.

Fully possible, but will break binary compatibility.

Better for now to simply have a suitable #ifdef _KERNEL_ to redefine 
*master in userspace on SPARC64 as "int64_t". The only question which has 
not been fully identified is how userspace can determine this is SPARC64 
with a 32-bit userspace, but actually it just needs to determine it is 
SPARC64..

Regards
Henrik

Re: sparc64 match module - bug id 94

[Henrik Nordstrom]

On Thu, 6 May 2004, Pablo Neira wrote:

> I thought about that possibility, it's an interesting hack, but I think 
> that the problem is much more specific, we have only this problem with 
> ipt_limit. Actually, when we have some internal match information which 
> is shared by some processors, we need to know which one is the selected 
> to work with. So, we could add a pointer to the master of internal match 
> data in struct ipt_entry_match. what do you think?

This is what it does and why there is problems. The problem is because the
size of that pointer is different between userspace and kernelspace on
SPARC64 with 32-bit userspace.

There is also an "unsigned long" which may be troublesome. This should be
a "u_int_32_t" I think. The pointer is however a little more troublesome
to convert to a fixed size datatype as it needs to match the size of a
pointer in kernelspace. The size of pointers in userspace is irrelevant as
it is only a placeholder in userspace. This calls for some #ifdefs.

Regards
Henrik

Re: sparc64 match module - bug id 94

[Pablo Neira]

Hi,

I think that if we use the autoconf stuff in iptables (see "[PATCH] 
Autoconf stuff for iptables"), we can workaround this problem by 
defining a different ipt_limit.h header in the iptables tree, actually a 
not so different header, just the same ipt_limit.h file but it contains 
in this case:

#ifdef __sparc64__
    u_int64_t master;
#else
    struct ipt_rateinfo *master;
#endif

so we shouldn't need to modify anything in kernel. This way we can keep 
those ifdef's in user space to workaround problems with tricky 
architectures which have different types in kernel and user space. Well, 
it's not so clean but it's easy.

regards,
Pablo