ipt_time fixes (resend, sorry)

ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4850 bytes --]

Hello,

My two patches fixes two problem I have just found. I tried to 
migrate from 2.4.x to the 2.6.11-rc1 kernel. Patches are against 
patch-o-matic-ng-20050105 but can be cleanly applied to the latest 
patch-o-matic-ng (20050113).

1. Fix compilation, without this patch I got:

net/ipv4/netfilter/ipt_time.c:128: warning: initialization makes integer from pointer without a cast
net/ipv4/netfilter/ipt_time.c:128: error: initializer element is not computable at load time
net/ipv4/netfilter/ipt_time.c:128: error: (near initialization for `time_match.revision')
net/ipv4/netfilter/ipt_time.c:128: warning: initialization from incompatible pointer type
net/ipv4/netfilter/ipt_time.c:128: warning: initialization from incompatible pointer type
make[3]: *** [net/ipv4/netfilter/ipt_time.o] Error 1
make[2]: *** [net/ipv4/netfilter] Error 2
make[1]: *** [net/ipv4] Error 2
make: *** [net] Error 2

Anyway, should we use "match"/"checkentry" or "&match"/"&checkentry"?
I noticed different netfilter modules use different syntax.

diff -Nur patch-o-matic-ng-20050105-orig/time/linux/net/ipv4/netfilter/ipt_time.c patch-o-matic-ng-20050105/time/linux/net/ipv4/netfilter/ipt_time.c
--- patch-o-matic-ng-20050105-orig/time/linux/net/ipv4/netfilter/ipt_time.c	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050105/time/linux/net/ipv4/netfilter/ipt_time.c	2005-01-14 12:15:09.000000000 +0100
@@ -48,8 +48,6 @@
         const struct net_device *out,
         const void *matchinfo,
         int offset,
-      const void *hdr,
-      u_int16_t datalen,
         int *hotdrop)
   {
   	const struct ipt_time_info *info = matchinfo;   /* match info for rule */
@@ -124,8 +122,12 @@
   	return 1;
   }

-static struct ipt_match time_match
-= { { NULL, NULL }, "time", &match, &checkentry, NULL, THIS_MODULE };
+static struct ipt_match time_match = {
+	.name		= "time",
+	.match		= match,
+	.checkentry	= checkentry,
+	.me		= THIS_MODULE,
+};

   static int __init init(void)
   {


2. Fix time match when skb->stamp.tv_sec is 0. I don't know why but on 
2.6.11-rc1 (and probably others 2.6.x kernels) skb->stamp.tv_sec seems to 
be allways 0, even on PRE_ROUTING and LOCAL_IN. So I fixed it by allways 
use kernel time byt only if skb->stamp.tv_sec is 0 (like in ULOG). With 
such modification we no longer needs kerneltime in the "struct ipt_time_info"
but I'm afraid we should keep it for binary compability, shouldn't we?

BTW, why we cannot use ipt_time on POSTROUTING?

diff -Nur patch-o-matic-ng-20050105-orig/time/linux/include/linux/netfilter_ipv4/ipt_time.h patch-o-matic-ng-20050105/time/linux/include/linux/netfilter_ipv4/ipt_time.h
--- patch-o-matic-ng-20050105-orig/time/linux/include/linux/netfilter_ipv4/ipt_time.h	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050105/time/linux/include/linux/netfilter_ipv4/ipt_time.h	2005-01-14 13:48:46.000000000 +0100
@@ -6,7 +6,10 @@
   	u_int8_t  days_match;   /* 1 bit per day. -SMTWTFS                      */
   	u_int16_t time_start;   /* 0 < time_start < 23*60+59 = 1439             */
   	u_int16_t time_stop;    /* 0:0 < time_stat < 23:59                      */
+
+				/* FIXME: Keep this one for userspace iptables binary compability: */
   	u_int8_t  kerneltime;   /* ignore skb time (and use kerneltime) or not. */
+
   	time_t    date_start;
   	time_t    date_stop;
   };
diff -Nur patch-o-matic-ng-20050105-orig/time/linux/net/ipv4/netfilter/ipt_time.c patch-o-matic-ng-20050105/time/linux/net/ipv4/netfilter/ipt_time.c
--- patch-o-matic-ng-20050105-orig/time/linux/net/ipv4/netfilter/ipt_time.c	2005-01-14 12:15:09.000000000 +0100
+++ patch-o-matic-ng-20050105/time/linux/net/ipv4/netfilter/ipt_time.c	2005-01-14 13:46:47.000000000 +0100
@@ -57,13 +57,11 @@
   	struct timeval kerneltimeval;
   	time_t packet_local_time;

-	/* if kerneltime=1, we don't read the skb->timestamp but kernel time instead */
-	if (info->kerneltime)
-	{
+	/* We might not have a timestamp, get one */
+	if (skb->stamp.tv_sec == 0) {
   		do_gettimeofday(&kerneltimeval);
   		packet_local_time = kerneltimeval.tv_sec;
-	}
-	else
+	} else
   		packet_local_time = skb->stamp.tv_sec;

   	/* First we make sure we are in the date start-stop boundaries */
@@ -102,11 +100,6 @@
   		printk("ipt_time: error, only valid for PRE_ROUTING, LOCAL_IN, FORWARD and OUTPUT)\n");
   		return 0;
   	}
-	/* we use the kerneltime if we are in forward or output */
-	info->kerneltime = 1;
-	if (hook_mask & ~((1 << NF_IP_FORWARD) | (1 << NF_IP_LOCAL_OUT))) 
-		/* we use the skb time */
-		info->kerneltime = 0;

   	/* Check the size */
   	if (matchsize != IPT_ALIGN(sizeof(struct ipt_time_info)))


Both patches are atatched. Tested on 2.6.11-rc1.

Best regards,


  			Krzysztof Olędzki

[-- Attachment #2: Type: TEXT/PLAIN, Size: 961 bytes --]

diff -Nur patch-o-matic-ng-20050105-orig/time/linux/net/ipv4/netfilter/ipt_time.c patch-o-matic-ng-20050105/time/linux/net/ipv4/netfilter/ipt_time.c
--- patch-o-matic-ng-20050105-orig/time/linux/net/ipv4/netfilter/ipt_time.c	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050105/time/linux/net/ipv4/netfilter/ipt_time.c	2005-01-14 12:15:09.000000000 +0100
@@ -48,8 +48,6 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *hdr,
-      u_int16_t datalen,
       int *hotdrop)
 {
 	const struct ipt_time_info *info = matchinfo;   /* match info for rule */
@@ -124,8 +122,12 @@
 	return 1;
 }
 
-static struct ipt_match time_match
-= { { NULL, NULL }, "time", &match, &checkentry, NULL, THIS_MODULE };
+static struct ipt_match time_match = {
+	.name		= "time",
+	.match		= match,
+	.checkentry	= checkentry,
+	.me		= THIS_MODULE,
+};
 
 static int __init init(void)
 {

[-- Attachment #3: Type: TEXT/PLAIN, Size: 2183 bytes --]

diff -Nur patch-o-matic-ng-20050105-orig/time/linux/include/linux/netfilter_ipv4/ipt_time.h patch-o-matic-ng-20050105/time/linux/include/linux/netfilter_ipv4/ipt_time.h
--- patch-o-matic-ng-20050105-orig/time/linux/include/linux/netfilter_ipv4/ipt_time.h	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050105/time/linux/include/linux/netfilter_ipv4/ipt_time.h	2005-01-14 13:48:46.000000000 +0100
@@ -6,7 +6,10 @@
 	u_int8_t  days_match;   /* 1 bit per day. -SMTWTFS                      */
 	u_int16_t time_start;   /* 0 < time_start < 23*60+59 = 1439             */
 	u_int16_t time_stop;    /* 0:0 < time_stat < 23:59                      */
+
+				/* FIXME: Keep this one for userspace iptables binary compability: */
 	u_int8_t  kerneltime;   /* ignore skb time (and use kerneltime) or not. */
+
 	time_t    date_start;
 	time_t    date_stop;
 };
diff -Nur patch-o-matic-ng-20050105-orig/time/linux/net/ipv4/netfilter/ipt_time.c patch-o-matic-ng-20050105/time/linux/net/ipv4/netfilter/ipt_time.c
--- patch-o-matic-ng-20050105-orig/time/linux/net/ipv4/netfilter/ipt_time.c	2005-01-14 12:15:09.000000000 +0100
+++ patch-o-matic-ng-20050105/time/linux/net/ipv4/netfilter/ipt_time.c	2005-01-14 13:46:47.000000000 +0100
@@ -57,13 +57,11 @@
 	struct timeval kerneltimeval;
 	time_t packet_local_time;
 
-	/* if kerneltime=1, we don't read the skb->timestamp but kernel time instead */
-	if (info->kerneltime)
-	{
+	/* We might not have a timestamp, get one */
+	if (skb->stamp.tv_sec == 0) {
 		do_gettimeofday(&kerneltimeval);
 		packet_local_time = kerneltimeval.tv_sec;
-	}
-	else
+	} else
 		packet_local_time = skb->stamp.tv_sec;
 
 	/* First we make sure we are in the date start-stop boundaries */
@@ -102,11 +100,6 @@
 		printk("ipt_time: error, only valid for PRE_ROUTING, LOCAL_IN, FORWARD and OUTPUT)\n");
 		return 0;
 	}
-	/* we use the kerneltime if we are in forward or output */
-	info->kerneltime = 1;
-	if (hook_mask & ~((1 << NF_IP_FORWARD) | (1 << NF_IP_LOCAL_OUT))) 
-		/* we use the skb time */
-		info->kerneltime = 0;
 
 	/* Check the size */
 	if (matchsize != IPT_ALIGN(sizeof(struct ipt_time_info)))

Re: ipt_time fixes (resend, sorry)

[Bill Rugolsky Jr.]

On Fri, Jan 14, 2005 at 03:04:38PM +0100, Krzysztof Oledzki wrote:
> Hello,
> 
> My two patches fixes two problem I have just found. I tried to 
> migrate from 2.4.x to the 2.6.11-rc1 kernel. Patches are against 
> patch-o-matic-ng-20050105 but can be cleanly applied to the latest 
> patch-o-matic-ng (20050113).
> 
> 1. Fix compilation, without this patch I got:

Yes, I also sent a patch for this.

> 
> 2. Fix time match when skb->stamp.tv_sec is 0. I don't know why but on 
> 2.6.11-rc1 (and probably others 2.6.x kernels) skb->stamp.tv_sec seems to 
> be allways 0, even on PRE_ROUTING and LOCAL_IN. So I fixed it by allways 
> use kernel time byt only if skb->stamp.tv_sec is 0 (like in ULOG). With 
> such modification we no longer needs kerneltime in the "struct 
> ipt_time_info"
> but I'm afraid we should keep it for binary compability, shouldn't we?

This is most likely due to Andi Kleen's net timestamp optimization, which
avoids calling gettimeofday in the network fastpath unless it is needed.

net/core/dev.c:1003:

/* When > 0 there are consumers of rx skb time stamps */
static atomic_t netstamp_needed = ATOMIC_INIT(0);

void net_enable_timestamp(void)
{
        atomic_inc(&netstamp_needed);
}

void net_disable_timestamp(void)
{
        atomic_dec(&netstamp_needed);
}

static inline void net_timestamp(struct timeval *stamp)
{
        if (atomic_read(&netstamp_needed))
                do_gettimeofday(stamp);
        else {
                stamp->tv_sec = 0;
                stamp->tv_usec = 0;
        }
}


Regards,

	Bill Rugolsky
 

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1560 bytes --]



On Fri, 14 Jan 2005, Bill Rugolsky Jr. wrote:

> On Fri, Jan 14, 2005 at 03:04:38PM +0100, Krzysztof Oledzki wrote:
>> Hello,
>>
>> My two patches fixes two problem I have just found. I tried to
>> migrate from 2.4.x to the 2.6.11-rc1 kernel. Patches are against
>> patch-o-matic-ng-20050105 but can be cleanly applied to the latest
>> patch-o-matic-ng (20050113).
>>
>> 1. Fix compilation, without this patch I got:
>
> Yes, I also sent a patch for this.
Ah, sorry :(

Anyway, which version is the correct one? With or without "&"?

+static struct ipt_match time_match = {
+	.name = "time",
+	.match = &match,
+	.checkentry = &checkentry,
+	.me = THIS_MODULE
+};

+static struct ipt_match time_match = {
+	.name		= "time",
+	.match		= match,
+	.checkentry	= checkentry,
+	.me		= THIS_MODULE,
+};


>> 2. Fix time match when skb->stamp.tv_sec is 0. I don't know why but on
>> 2.6.11-rc1 (and probably others 2.6.x kernels) skb->stamp.tv_sec seems to
>> be allways 0, even on PRE_ROUTING and LOCAL_IN. So I fixed it by allways
>> use kernel time byt only if skb->stamp.tv_sec is 0 (like in ULOG). With
>> such modification we no longer needs kerneltime in the "struct
>> ipt_time_info"
>> but I'm afraid we should keep it for binary compability, shouldn't we?
>
> This is most likely due to Andi Kleen's net timestamp optimization, which
> avoids calling gettimeofday in the network fastpath unless it is needed.

I see... So, where we should fix this in netfilter code?

Best regards,


 			Krzysztof Olędzki

Re: ipt_time fixes (resend, sorry)

[Samuel Jean]

On Fri, January 14, 2005 9:48 am, Krzysztof Oledzki said:
>> This is most likely due to Andi Kleen's net timestamp optimization,
>> which
>> avoids calling gettimeofday in the network fastpath unless it is needed.
>
> I see... So, where we should fix this in netfilter code?

Am sorry, I haven't looked much at your patches. But here's my opinion.

Netfilter shouldn't enable this for the unique reason a match/target
will possibly rely on this.

I think it's the job of the first rule (of a given module) seeing that
'new' packet to keep counting the last-seen time. Yes, the stamp is a bit
later than it should. But that's not dramatic and still consistant among
multiple rules (of a common module) needing that reference.

I use an ugly hack of this kind:

   /* We might not have a timestamp, get one */
   if (skb->stamp.tv_sec == 0)
      do_gettimeofday((struct timeval *)&skb->stamp);

   now = timeval_to_jiffies(&skb->stamp);


   if (struct->curr != now)
      struct->prev = xchg(&struct->curr, now);

As of writing it, I only tested it under nfsim while increasing kernel
time before each hook. Seemed to work.

Each of my rules were using struct->prev as the last-seen time of the
packet. And the first rule always update this stamp.

If I am way of target, just slap me 8)

> Best regards,
>
>
>  			Krzysztof Olêdzki

Cheers,

Samuel

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3843 bytes --]



On Fri, 14 Jan 2005, Samuel Jean wrote:

> On Fri, January 14, 2005 9:48 am, Krzysztof Oledzki said:
>>> This is most likely due to Andi Kleen's net timestamp optimization,
>>> which
>>> avoids calling gettimeofday in the network fastpath unless it is needed.
>>
>> I see... So, where we should fix this in netfilter code?
>
> Am sorry, I haven't looked much at your patches. But here's my opinion.
>
> Netfilter shouldn't enable this for the unique reason a match/target
> will possibly rely on this.
>
> I think it's the job of the first rule (of a given module) seeing that
> 'new' packet to keep counting the last-seen time. Yes, the stamp is a bit
> later than it should. But that's not dramatic and still consistant among
> multiple rules (of a common module) needing that reference.

OK, How about...

diff -Nur patch-o-matic-ng-20050113-orig/time/linux/include/linux/netfilter_ipv4/ipt_time.h patch-o-matic-ng-20050113/time/linux/include/linux/netfilter_ipv4/ipt_time.h
--- patch-o-matic-ng-20050113-orig/time/linux/include/linux/netfilter_ipv4/ipt_time.h	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050113/time/linux/include/linux/netfilter_ipv4/ipt_time.h	2005-01-14 16:06:54.000000000 +0100
@@ -6,7 +6,10 @@
  	u_int8_t  days_match;   /* 1 bit per day. -SMTWTFS                      */
  	u_int16_t time_start;   /* 0 < time_start < 23*60+59 = 1439             */
  	u_int16_t time_stop;    /* 0:0 < time_stat < 23:59                      */
+
+				/* FIXME: Keep this one for userspace iptables binary compability: */
  	u_int8_t  kerneltime;   /* ignore skb time (and use kerneltime) or not. */
+
  	time_t    date_start;
  	time_t    date_stop;
  };
diff -Nur patch-o-matic-ng-20050113-orig/time/linux/net/ipv4/netfilter/ipt_time.c patch-o-matic-ng-20050113/time/linux/net/ipv4/netfilter/ipt_time.c
--- patch-o-matic-ng-20050113-orig/time/linux/net/ipv4/netfilter/ipt_time.c	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050113/time/linux/net/ipv4/netfilter/ipt_time.c	2005-01-14 16:15:30.000000000 +0100
@@ -56,24 +56,17 @@
  	struct tm currenttime;                          /* time human readable */
  	u_int8_t days_of_week[7] = {64, 32, 16, 8, 4, 2, 1};
  	u_int16_t packet_time;
-	struct timeval kerneltimeval;
-	time_t packet_local_time;

-	/* if kerneltime=1, we don't read the skb->timestamp but kernel time instead */
-	if (info->kerneltime)
-	{
-		do_gettimeofday(&kerneltimeval);
-		packet_local_time = kerneltimeval.tv_sec;
-	}
-	else
-		packet_local_time = skb->stamp.tv_sec;
+	/* We might not have a timestamp, get one */
+	if (skb->stamp.tv_sec == 0)
+		do_gettimeofday((struct timeval *)&skb->stamp);

  	/* First we make sure we are in the date start-stop boundaries */
-	if ((packet_local_time < info->date_start) || (packet_local_time > info->date_stop))
+	if ((skb->stamp.tv_sec < info->date_start) || (skb->stamp.tv_sec > info->date_stop))
  		return 0; /* We are outside the date boundaries */

  	/* Transform the timestamp of the packet, in a human readable form */
-	localtime(&packet_local_time, &currenttime);
+	localtime(&skb->stamp.tv_sec, &currenttime);

  	/* check if we match this timestamp, we start by the days... */
  	if ((days_of_week[currenttime.tm_wday] & info->days_match) != days_of_week[currenttime.tm_wday])
@@ -104,11 +97,6 @@
  		printk("ipt_time: error, only valid for PRE_ROUTING, LOCAL_IN, FORWARD and OUTPUT)\n");
  		return 0;
  	}
-	/* we use the kerneltime if we are in forward or output */
-	info->kerneltime = 1;
-	if (hook_mask & ~((1 << NF_IP_FORWARD) | (1 << NF_IP_LOCAL_OUT))) 
-		/* we use the skb time */
-		info->kerneltime = 0;

  	/* Check the size */
  	if (matchsize != IPT_ALIGN(sizeof(struct ipt_time_info)))

Best regards,

 			Krzysztof Olędzki

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2954 bytes --]

diff -Nur patch-o-matic-ng-20050113-orig/time/linux/include/linux/netfilter_ipv4/ipt_time.h patch-o-matic-ng-20050113/time/linux/include/linux/netfilter_ipv4/ipt_time.h
--- patch-o-matic-ng-20050113-orig/time/linux/include/linux/netfilter_ipv4/ipt_time.h	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050113/time/linux/include/linux/netfilter_ipv4/ipt_time.h	2005-01-14 16:06:54.000000000 +0100
@@ -6,7 +6,10 @@
 	u_int8_t  days_match;   /* 1 bit per day. -SMTWTFS                      */
 	u_int16_t time_start;   /* 0 < time_start < 23*60+59 = 1439             */
 	u_int16_t time_stop;    /* 0:0 < time_stat < 23:59                      */
+
+				/* FIXME: Keep this one for userspace iptables binary compability: */
 	u_int8_t  kerneltime;   /* ignore skb time (and use kerneltime) or not. */
+
 	time_t    date_start;
 	time_t    date_stop;
 };
diff -Nur patch-o-matic-ng-20050113-orig/time/linux/net/ipv4/netfilter/ipt_time.c patch-o-matic-ng-20050113/time/linux/net/ipv4/netfilter/ipt_time.c
--- patch-o-matic-ng-20050113-orig/time/linux/net/ipv4/netfilter/ipt_time.c	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050113/time/linux/net/ipv4/netfilter/ipt_time.c	2005-01-14 16:15:30.000000000 +0100
@@ -56,24 +56,17 @@
 	struct tm currenttime;                          /* time human readable */
 	u_int8_t days_of_week[7] = {64, 32, 16, 8, 4, 2, 1};
 	u_int16_t packet_time;
-	struct timeval kerneltimeval;
-	time_t packet_local_time;
 
-	/* if kerneltime=1, we don't read the skb->timestamp but kernel time instead */
-	if (info->kerneltime)
-	{
-		do_gettimeofday(&kerneltimeval);
-		packet_local_time = kerneltimeval.tv_sec;
-	}
-	else
-		packet_local_time = skb->stamp.tv_sec;
+	/* We might not have a timestamp, get one */
+	if (skb->stamp.tv_sec == 0)
+		do_gettimeofday((struct timeval *)&skb->stamp);
 
 	/* First we make sure we are in the date start-stop boundaries */
-	if ((packet_local_time < info->date_start) || (packet_local_time > info->date_stop))
+	if ((skb->stamp.tv_sec < info->date_start) || (skb->stamp.tv_sec > info->date_stop))
 		return 0; /* We are outside the date boundaries */
 
 	/* Transform the timestamp of the packet, in a human readable form */
-	localtime(&packet_local_time, &currenttime);
+	localtime(&skb->stamp.tv_sec, &currenttime);
 
 	/* check if we match this timestamp, we start by the days... */
 	if ((days_of_week[currenttime.tm_wday] & info->days_match) != days_of_week[currenttime.tm_wday])
@@ -104,11 +97,6 @@
 		printk("ipt_time: error, only valid for PRE_ROUTING, LOCAL_IN, FORWARD and OUTPUT)\n");
 		return 0;
 	}
-	/* we use the kerneltime if we are in forward or output */
-	info->kerneltime = 1;
-	if (hook_mask & ~((1 << NF_IP_FORWARD) | (1 << NF_IP_LOCAL_OUT))) 
-		/* we use the skb time */
-		info->kerneltime = 0;
 
 	/* Check the size */
 	if (matchsize != IPT_ALIGN(sizeof(struct ipt_time_info)))

Re: ipt_time fixes (resend, sorry)

[Brad Fisher]

Krzysztof Oledzki wrote:

>
> -        packet_local_time = skb->stamp.tv_sec;
> +    /* We might not have a timestamp, get one */
> +    if (skb->stamp.tv_sec == 0)
> +        do_gettimeofday((struct timeval *)&skb->stamp);
>
>      /* First we make sure we are in the date start-stop boundaries */
> -    if ((packet_local_time < info->date_start) || (packet_local_time 
> > info->date_stop))
> +    if ((skb->stamp.tv_sec < info->date_start) || (skb->stamp.tv_sec 
> > info->date_stop))
>          return 0; /* We are outside the date boundaries */
>
I'd like to see the above like become:

+    if ((skb->stamp.tv_sec >= info->date_start) && (skb->stamp.tv_sec 
<= info->date_stop))
         return 0; /* We are outside the date boundaries */

This will have the same logic in most situations, and fix a problem with 
the time match where you need 2 rules to match something like all times 
between 20:00-04:00 (ie. the start time is "greater" than the end 
time).  I've used something similar for years and enjoy not needing 
multiple rules for this.  I also have a patch (and I believe I've 
submitted it in the past) for grabbing the kernel time if no time is 
known (and with that patch, or what's being discussed here, I believe 
the time match should work in any chain).

-Brad Fisher

Re: ipt_time fixes (resend, sorry)

[Brad Fisher]

Brad Fisher wrote:

> Krzysztof Oledzki wrote:
>
>>
>> -        packet_local_time = skb->stamp.tv_sec;
>> +    /* We might not have a timestamp, get one */
>> +    if (skb->stamp.tv_sec == 0)
>> +        do_gettimeofday((struct timeval *)&skb->stamp);
>>
>>      /* First we make sure we are in the date start-stop boundaries */
>> -    if ((packet_local_time < info->date_start) || (packet_local_time 
>> > info->date_stop))
>> +    if ((skb->stamp.tv_sec < info->date_start) || (skb->stamp.tv_sec 
>> > info->date_stop))
>>          return 0; /* We are outside the date boundaries */
>>
> I'd like to see the above like become:
>
> +    if ((skb->stamp.tv_sec >= info->date_start) && (skb->stamp.tv_sec 
> <= info->date_stop))
>         return 0; /* We are outside the date boundaries */
>
> This will have the same logic in most situations, and fix a problem 
> with the time match where you need 2 rules to match something like all 
> times between 20:00-04:00 (ie. the start time is "greater" than the 
> end time).  I've used something similar for years and enjoy not 
> needing multiple rules for this.  I also have a patch (and I believe 
> I've submitted it in the past) for grabbing the kernel time if no time 
> is known (and with that patch, or what's being discussed here, I 
> believe the time match should work in any chain).
>
> -Brad Fisher
>
>
> !DSPAM:41e7f37a167171105578672!
>

Sorry for the additional spam :(
 
The check should be changed to something like the following:

+      if (info->date_start <= info->date_stop) {
+              /* normal order: start <= stop */
+              if ((skb->stamp.tv_sec < info->date_start) || 
(skb->stamp.tv_sec > info->date_stop))
+                        return 0;
+        } else {
+              /* reversed order: stop < start */
+                if ((skb->stamp.tv_sec < info->date_start) && 
(skb->stamp.tv_sec > info->date_stop))
+                        return 0;
+        }

Sorry about the previous crap :)  Anyway, this only adds one more if 
test per packet, which in my opintion is acceptable when the alternative 
is an additional rule.

-Brad

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2448 bytes --]



On Fri, 14 Jan 2005, Brad Fisher wrote:

> Brad Fisher wrote:
>
>> Krzysztof Oledzki wrote:
>> 
>>> 
>>> -        packet_local_time = skb->stamp.tv_sec;
>>> +    /* We might not have a timestamp, get one */
>>> +    if (skb->stamp.tv_sec == 0)
>>> +        do_gettimeofday((struct timeval *)&skb->stamp);
>>> 
>>>      /* First we make sure we are in the date start-stop boundaries */
>>> -    if ((packet_local_time < info->date_start) || (packet_local_time > 
>>> info->date_stop))
>>> +    if ((skb->stamp.tv_sec < info->date_start) || (skb->stamp.tv_sec > 
>>> info->date_stop))
>>>          return 0; /* We are outside the date boundaries */
>>> 
>> I'd like to see the above like become:
>> 
>> +    if ((skb->stamp.tv_sec >= info->date_start) && (skb->stamp.tv_sec <= 
>> info->date_stop))
>>         return 0; /* We are outside the date boundaries */
>> 
>> This will have the same logic in most situations, and fix a problem with 
>> the time match where you need 2 rules to match something like all times 
>> between 20:00-04:00 (ie. the start time is "greater" than the end time). 
>> I've used something similar for years and enjoy not needing multiple rules 
>> for this.  I also have a patch (and I believe I've submitted it in the 
>> past) for grabbing the kernel time if no time is known (and with that 
>> patch, or what's being discussed here, I believe the time match should work 
>> in any chain).
>> 
>> -Brad Fisher
>> 
>> 
>> !DSPAM:41e7f37a167171105578672!
>> 
>
> Sorry for the additional spam :(
>
> The check should be changed to something like the following:
>
> +      if (info->date_start <= info->date_stop) {
> +              /* normal order: start <= stop */
> +              if ((skb->stamp.tv_sec < info->date_start) || 
> (skb->stamp.tv_sec > info->date_stop))
> +                        return 0;
> +        } else {
> +              /* reversed order: stop < start */
> +                if ((skb->stamp.tv_sec < info->date_start) && 
> (skb->stamp.tv_sec > info->date_stop))
> +                        return 0;
> +        }
>
> Sorry about the previous crap :)  Anyway, this only adds one more if test per 
> packet, which in my opintion is acceptable when the alternative is an 
> additional rule.


OK.. You need to match 20:00-4:00, right? So why you just cannot reverse 
the match of 4:00-20:00?

Best regards,

 				Krzysztof Olędzki

Re: ipt_time fixes (resend, sorry)

[Brad Fisher]

Krzysztof Oledzki wrote:

>>
>> Sorry for the additional spam :(
>>
>> The check should be changed to something like the following:
>>
>> +      if (info->date_start <= info->date_stop) {
>> +              /* normal order: start <= stop */
>> +              if ((skb->stamp.tv_sec < info->date_start) || 
>> (skb->stamp.tv_sec > info->date_stop))
>> +                        return 0;
>> +        } else {
>> +              /* reversed order: stop < start */
>> +                if ((skb->stamp.tv_sec < info->date_start) && 
>> (skb->stamp.tv_sec > info->date_stop))
>> +                        return 0;
>> +        }
>>
>> Sorry about the previous crap :)  Anyway, this only adds one more if 
>> test per packet, which in my opintion is acceptable when the 
>> alternative is an additional rule.
>
>
>
> OK.. You need to match 20:00-4:00, right? So why you just cannot 
> reverse the match of 4:00-20:00?
>
> Best regards,
>
>                 Krzysztof Olędzki
>
20:00 - 4:00 is very different from 4:00 - 20:00, one is a range of 8 
hours (20,21,22,23,0,1,2,3), the other is a range of 16 
(4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19)...  They are complementary 
to each other in terms of a 24 hour clock.

To match 20:00 - 4:00 you currently need 2 rules:
  1) match 20:00 - 23:59
  2) match 0:00 - 4:00
What I'm proposing is to allow this to be reduced to one rule.

-Brad Fisher

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2009 bytes --]



On Thu, 20 Jan 2005, Brad Fisher wrote:

> Krzysztof Oledzki wrote:
>
>>> 
>>> Sorry for the additional spam :(
>>> 
>>> The check should be changed to something like the following:
>>> 
>>> +      if (info->date_start <= info->date_stop) {
>>> +              /* normal order: start <= stop */
>>> +              if ((skb->stamp.tv_sec < info->date_start) || 
>>> (skb->stamp.tv_sec > info->date_stop))
>>> +                        return 0;
>>> +        } else {
>>> +              /* reversed order: stop < start */
>>> +                if ((skb->stamp.tv_sec < info->date_start) && 
>>> (skb->stamp.tv_sec > info->date_stop))
>>> +                        return 0;
>>> +        }
>>> 
>>> Sorry about the previous crap :)  Anyway, this only adds one more if test 
>>> per packet, which in my opintion is acceptable when the alternative is an 
>>> additional rule.
>> 
>> 
>> 
>> OK.. You need to match 20:00-4:00, right? So why you just cannot reverse 
>> the match of 4:00-20:00?
>> 
>> Best regards,
>> 
>>                 Krzysztof Olędzki
>> 
> 20:00 - 4:00 is very different from 4:00 - 20:00, one is a range of 8 hours 
> (20,21,22,23,0,1,2,3), the other is a range of 16
True...

> (4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19)...  They are complementary to 
> each other in terms of a 24 hour clock.
True..

> To match 20:00 - 4:00 you currently need 2 rules:
> 1) match 20:00 - 23:59
> 2) match 0:00 - 4:00
> What I'm proposing is to allow this to be reduced to one rule.

So, again.. You need something like invert flag in -m like in the -p:

iptables -A something ! -m time --timestart 4:01 --timestop 19:59 -j DoSomething

This one should match 20:00-20:59 and 0:00-4:00, true?

Right now you can use:

iptables -N Match20_4
iptables -A Match20_4 -m time --timestart 4:01 --timestop 19:59 -j RETURN
iptables -A Match20_4 -j DoSomething

This one should match 20:00-20:59 and 0:00-4:00, true?

Best regards,

 			Krzysztof Olędzki

Re: ipt_time fixes (resend, sorry)

[Samuel Jean]

Hi!

On Thu, January 20, 2005 12:18 pm, Krzysztof Oledzki said:

..snip..

> On Thu, 20 Jan 2005, Brad Fisher wrote:
>> To match 20:00 - 4:00 you currently need 2 rules:
>> 1) match 20:00 - 23:59
>> 2) match 0:00 - 4:00
>> What I'm proposing is to allow this to be reduced to one rule.

What Brad proposes makes so much sense, IMHO. I haven't looked at this
code yet (ipt_time) but I thought it already behaves that way. Apperently
not, since you both complain about it.

>
> So, again.. You need something like invert flag in -m like in the -p:
>
> iptables -A something ! -m time --timestart 4:01 --timestop 19:59 -j
> DoSomething
>
> This one should match 20:00-20:59 and 0:00-4:00, true?

I wasn't aware you can negate --match options. Are you sure we can do this ?

>
> Right now you can use:
>
> iptables -N Match20_4
> iptables -A Match20_4 -m time --timestart 4:01 --timestop 19:59 -j RETURN
> iptables -A Match20_4 -j DoSomething
>
> This one should match 20:00-20:59 and 0:00-4:00, true?

Yes, but rather ugly :) I strongly suggest adding support to it. As Brad
did mention, another 'if' condition produces less latency than superfluous
packet iteration.

>
> Best regards,
>
>  			Krzysztof Olêdzki

Cheers!

Samuel

Re: ipt_time fixes (resend, sorry)

[Brad Fisher]

Samuel Jean wrote:

>Hi!
>
>On Thu, January 20, 2005 12:18 pm, Krzysztof Oledzki said:
>
>..snip..
>
>  
>
>>On Thu, 20 Jan 2005, Brad Fisher wrote:
>>    
>>
>>>To match 20:00 - 4:00 you currently need 2 rules:
>>>1) match 20:00 - 23:59
>>>2) match 0:00 - 4:00
>>>What I'm proposing is to allow this to be reduced to one rule.
>>>      
>>>
>
>What Brad proposes makes so much sense, IMHO. I haven't looked at this
>code yet (ipt_time) but I thought it already behaves that way. Apperently
>not, since you both complain about it.
>
>  
>
>>So, again.. You need something like invert flag in -m like in the -p:
>>
>>iptables -A something ! -m time --timestart 4:01 --timestop 19:59 -j
>>DoSomething
>>
>>This one should match 20:00-20:59 and 0:00-4:00, true?
>>    
>>
>
>I wasn't aware you can negate --match options. Are you sure we can do this ?
>
>  
>

I haven't found any generic capability to negate matches.  I could see 
this being possibly useful, and could reduce the need for every 
individual match to implement some sort of negation internally.  Of 
course, it may not make sense to be able to negate some types of matches 
(no examples come to mind though).

>>Right now you can use:
>>
>>iptables -N Match20_4
>>iptables -A Match20_4 -m time --timestart 4:01 --timestop 19:59 -j RETURN
>>iptables -A Match20_4 -j DoSomething
>>
>>This one should match 20:00-20:59 and 0:00-4:00, true?
>>    
>>
>
>Yes, but rather ugly :) I strongly suggest adding support to it. As Brad
>did mention, another 'if' condition produces less latency than superfluous
>packet iteration.
>  
>
It also makes the ruleset more complex by adding a custom chain and two 
rules where I'd like to only have one rule.  Your example may work fine 
and not add much complexity when multiple rules may jump to the new 
chain, but if you need to match multiple time ranges this way, it could 
really bulk up your ruleset for no real reason.  It'd just be nice if 
the time match handled this in a way that doesn't require such 
workarounds in the first place.

>>Best regards,
>>
>> 			Krzysztof Olêdzki
>>    
>>
>
>Cheers!
>
>Samuel
>  
>
-Brad

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3011 bytes --]



On Thu, 20 Jan 2005, Samuel Jean wrote:

> Hi!
>
> On Thu, January 20, 2005 12:18 pm, Krzysztof Oledzki said:
>
> ..snip..
>
>> On Thu, 20 Jan 2005, Brad Fisher wrote:
>>> To match 20:00 - 4:00 you currently need 2 rules:
>>> 1) match 20:00 - 23:59
>>> 2) match 0:00 - 4:00
>>> What I'm proposing is to allow this to be reduced to one rule.
>
> What Brad proposes makes so much sense, IMHO. I haven't looked at this
> code yet (ipt_time) but I thought it already behaves that way. Apperently
> not, since you both complain about it.
>
>>
>> So, again.. You need something like invert flag in -m like in the -p:
>>
>> iptables -A something ! -m time --timestart 4:01 --timestop 19:59 -j
>> DoSomething
>>
>> This one should match 20:00-20:59 and 0:00-4:00, true?
>
> I wasn't aware you can negate --match options. Are you sure we can do this ?

No, unfortunately we can't AFAIK. For unknown reasons to me, we can only 
negate -p (protocol) but not the -m. Please, look at the iptables.c:

(...)
switch (c) {
         /*
          * Command selection
          */
(...)
case 'p':
         check_inverse(optarg, &invert, &optind, argc);
         set_option(&options, OPT_PROTOCOL, &fw.ip.invflags,
                    invert);

         /* Canonicalize into lower case */
         for (protocol = argv[optind-1]; *protocol; protocol++)
                 *protocol = tolower(*protocol);

         protocol = argv[optind-1];
         fw.ip.proto = parse_protocol(protocol);

         if (fw.ip.proto == 0
             && (fw.ip.invflags & IPT_INV_PROTO))
                 exit_error(PARAMETER_PROBLEM,
                            "rule would never match protocol");
         fw.nfcache |= NFC_IP_PROTO;
         break;
(...)
case 'm': {
         size_t size;

         if (invert)
                 exit_error(PARAMETER_PROBLEM,
                            "unexpected ! flag before --match");

         m = find_match(optarg, LOAD_MUST_SUCCEED, &matches);
         size = IPT_ALIGN(sizeof(struct ipt_entry_match))
                          + m->size;
         m->m = fw_calloc(1, size);
         m->m->u.match_size = size;
         strcpy(m->m->u.user.name, m->name);
         m->init(m->m, &fw.nfcache);
         opts = merge_options(opts, m->extra_opts, &m->option_offset);
}
break;
(...)


>> Right now you can use:
>>
>> iptables -N Match20_4
>> iptables -A Match20_4 -m time --timestart 4:01 --timestop 19:59 -j RETURN
>> iptables -A Match20_4 -j DoSomething
>>
>> This one should match 20:00-20:59 and 0:00-4:00, true?
>
> Yes, but rather ugly :) I strongly suggest adding support to it. As Brad
> did mention, another 'if' condition produces less latency than superfluous
> packet iteration.

I'm not sure if adding a negation-like-workaround to ipt_time (and 
possible other matches like ipt_limit) is a good idea. My qestion is: why 
we cannot simply negate all matches?


Best regards,

 			Krzysztof Olędzki

Re: ipt_time fixes (resend, sorry)

[Samuel Jean]

Krzysztof Oledzki wrote:
> I'm not sure if adding a negation-like-workaround to ipt_time (and 
> possible other matches like ipt_limit) is a good idea.

Obviously, I would need to peek at the code to continue following this 
thread. However, my brain is already spinning on other codes.

> My qestion is: 
> why we cannot simply negate all matches?

I guess it would confuse the user when the match uses multiple options.
Am confused just thinking about it :)

But am also in the favor that all matches should accept and behave the 
right way with negation unless negation really doesn't make sense nor 
does anything usefull.

Maybe I missed a match that really doesn't make sense with negation.
(I surely did)

> 
> 
> Best regards,
> 
>             Krzysztof Olędzki

Cheers,

Samuel

Re: ipt_time fixes (resend, sorry)

[Harald Welte]

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

On Fri, Jan 14, 2005 at 04:24:01PM +0100, Krzysztof Oledzki wrote:
> OK, How about...

Could you please coordinate with Fabrice Marie (the author of the
module?) and send me (personally) the final patch?

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 462 bytes --]



On Tue, 1 Feb 2005, Harald Welte wrote:

> On Fri, Jan 14, 2005 at 04:24:01PM +0100, Krzysztof Oledzki wrote:
>> OK, How about...
>
> Could you please coordinate with Fabrice Marie (the author of the
> module?) and send me (personally) the final patch?
Of course! What is the correct Fabrice's email address? My last patches 
was also send to fabrice%netfilter*org (s/%/@/, s/\*/./), but no one 
responded.

Best regards,

 				Krzysztof Olędzki

Re: ipt_time fixes (resend, sorry)

[Harald Welte]

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

On Tue, Feb 01, 2005 at 05:52:38PM +0100, Krzysztof Oledzki wrote:
> 
> 
> On Tue, 1 Feb 2005, Harald Welte wrote:
> 
> >On Fri, Jan 14, 2005 at 04:24:01PM +0100, Krzysztof Oledzki wrote:
> >>OK, How about...
> >
> >Could you please coordinate with Fabrice Marie (the author of the
> >module?) and send me (personally) the final patch?
> Of course! What is the correct Fabrice's email address? My last patches 
> was also send to fabrice%netfilter*org (s/%/@/, s/\*/./), but no one 
> responded.

mh, mayb fabrice is inactive these days... so mabe time is sort-of
unmaintained. 

Pleaes retry, the last email I got from him (fabrice@netfilter.org) was
Nov 02 2004, not so long ago.

> Best regards,
> 				Krzysztof Ol?dzki

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4122 bytes --]



On Tue, 1 Feb 2005, Harald Welte wrote:

> On Fri, Jan 14, 2005 at 04:24:01PM +0100, Krzysztof Oledzki wrote:
>> OK, How about...
>
> Could you please coordinate with Fabrice Marie (the author of the
> module?) and send me (personally) the final patch?

Hello,

I think it is good time to commit my ipt_time-fix-timestamp2.patch. I 
believe it was ACKed by the author of ipt_time module:

---------- Forwarded message ----------
Date: Tue, 15 Feb 2005 23:23:08 +0800
From: Fabrice MARIE <fabrice@netfilter.org>
To: Krzysztof Oledzki <olenf@ans.pl>
Cc: netfilter-devel@lists.netfilter.org
Subject: Re: ipt_time fixes (resend, sorry)


Hello,

On Tuesday 15 February 2005 09:20, Krzysztof Oledzki wrote:
> Any progress? ;-)

Yep. Sorry for the delay. The patch looks alright although I didn't test 
it.

-------- Forwarded message end --------

This patch fixes time match when skb->stamp.tv_sec is 0, due to timestamp 
optimization. It also simplifies time read logic. I believe this patch 
does not break anything, even binary iptables.


diff -Nur patch-o-matic-ng-20050113-orig/time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h patch-o-matic-ng-20050113/time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h
--- patch-o-matic-ng-20050113-orig/time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050113/time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h	2005-01-14 16:06:54.000000000 +0100
@@ -6,7 +6,10 @@
  	u_int8_t  days_match;   /* 1 bit per day. -SMTWTFS                      */
  	u_int16_t time_start;   /* 0 < time_start < 23*60+59 = 1439             */
  	u_int16_t time_stop;    /* 0:0 < time_stat < 23:59                      */
+
+				/* FIXME: Keep this one for userspace iptables binary compability: */
  	u_int8_t  kerneltime;   /* ignore skb time (and use kerneltime) or not. */
+
  	time_t    date_start;
  	time_t    date_stop;
  };
diff -Nur patch-o-matic-ng-20050113-orig/time/linux-2.6/net/ipv4/netfilter/ipt_time.c patch-o-matic-ng-20050113/time/linux-2.6/net/ipv4/netfilter/ipt_time.c
--- patch-o-matic-ng-20050113-orig/time/linux-2.6/net/ipv4/netfilter/ipt_time.c	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050113/time/linux-2.6/net/ipv4/netfilter/ipt_time.c	2005-01-14 16:15:30.000000000 +0100
@@ -56,24 +56,17 @@
  	struct tm currenttime;                          /* time human readable */
  	u_int8_t days_of_week[7] = {64, 32, 16, 8, 4, 2, 1};
  	u_int16_t packet_time;
-	struct timeval kerneltimeval;
-	time_t packet_local_time;

-	/* if kerneltime=1, we don't read the skb->timestamp but kernel time instead */
-	if (info->kerneltime)
-	{
-		do_gettimeofday(&kerneltimeval);
-		packet_local_time = kerneltimeval.tv_sec;
-	}
-	else
-		packet_local_time = skb->stamp.tv_sec;
+	/* We might not have a timestamp, get one */
+	if (skb->stamp.tv_sec == 0)
+		do_gettimeofday((struct timeval *)&skb->stamp);

  	/* First we make sure we are in the date start-stop boundaries */
-	if ((packet_local_time < info->date_start) || (packet_local_time > info->date_stop))
+	if ((skb->stamp.tv_sec < info->date_start) || (skb->stamp.tv_sec > info->date_stop))
  		return 0; /* We are outside the date boundaries */

  	/* Transform the timestamp of the packet, in a human readable form */
-	localtime(&packet_local_time, &currenttime);
+	localtime(&skb->stamp.tv_sec, &currenttime);

  	/* check if we match this timestamp, we start by the days... */
  	if ((days_of_week[currenttime.tm_wday] & info->days_match) != days_of_week[currenttime.tm_wday])
@@ -104,11 +97,6 @@
  		printk("ipt_time: error, only valid for PRE_ROUTING, LOCAL_IN, FORWARD and OUTPUT)\n");
  		return 0;
  	}
-	/* we use the kerneltime if we are in forward or output */
-	info->kerneltime = 1;
-	if (hook_mask & ~((1 << NF_IP_FORWARD) | (1 << NF_IP_LOCAL_OUT))) 
-		/* we use the skb time */
-		info->kerneltime = 0;

  	/* Check the size */
  	if (matchsize != IPT_ALIGN(sizeof(struct ipt_time_info)))

Best regards,


 			Krzysztof Olędzki

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2986 bytes --]

diff -Nur patch-o-matic-ng-20050113-orig/time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h patch-o-matic-ng-20050113/time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h
--- patch-o-matic-ng-20050113-orig/time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050113/time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h	2005-01-14 16:06:54.000000000 +0100
@@ -6,7 +6,10 @@
 	u_int8_t  days_match;   /* 1 bit per day. -SMTWTFS                      */
 	u_int16_t time_start;   /* 0 < time_start < 23*60+59 = 1439             */
 	u_int16_t time_stop;    /* 0:0 < time_stat < 23:59                      */
+
+				/* FIXME: Keep this one for userspace iptables binary compability: */
 	u_int8_t  kerneltime;   /* ignore skb time (and use kerneltime) or not. */
+
 	time_t    date_start;
 	time_t    date_stop;
 };
diff -Nur patch-o-matic-ng-20050113-orig/time/linux-2.6/net/ipv4/netfilter/ipt_time.c patch-o-matic-ng-20050113/time/linux-2.6/net/ipv4/netfilter/ipt_time.c
--- patch-o-matic-ng-20050113-orig/time/linux-2.6/net/ipv4/netfilter/ipt_time.c	2004-05-05 12:11:24.000000000 +0200
+++ patch-o-matic-ng-20050113/time/linux-2.6/net/ipv4/netfilter/ipt_time.c	2005-01-14 16:15:30.000000000 +0100
@@ -56,24 +56,17 @@
 	struct tm currenttime;                          /* time human readable */
 	u_int8_t days_of_week[7] = {64, 32, 16, 8, 4, 2, 1};
 	u_int16_t packet_time;
-	struct timeval kerneltimeval;
-	time_t packet_local_time;
 
-	/* if kerneltime=1, we don't read the skb->timestamp but kernel time instead */
-	if (info->kerneltime)
-	{
-		do_gettimeofday(&kerneltimeval);
-		packet_local_time = kerneltimeval.tv_sec;
-	}
-	else
-		packet_local_time = skb->stamp.tv_sec;
+	/* We might not have a timestamp, get one */
+	if (skb->stamp.tv_sec == 0)
+		do_gettimeofday((struct timeval *)&skb->stamp);
 
 	/* First we make sure we are in the date start-stop boundaries */
-	if ((packet_local_time < info->date_start) || (packet_local_time > info->date_stop))
+	if ((skb->stamp.tv_sec < info->date_start) || (skb->stamp.tv_sec > info->date_stop))
 		return 0; /* We are outside the date boundaries */
 
 	/* Transform the timestamp of the packet, in a human readable form */
-	localtime(&packet_local_time, &currenttime);
+	localtime(&skb->stamp.tv_sec, &currenttime);
 
 	/* check if we match this timestamp, we start by the days... */
 	if ((days_of_week[currenttime.tm_wday] & info->days_match) != days_of_week[currenttime.tm_wday])
@@ -104,11 +97,6 @@
 		printk("ipt_time: error, only valid for PRE_ROUTING, LOCAL_IN, FORWARD and OUTPUT)\n");
 		return 0;
 	}
-	/* we use the kerneltime if we are in forward or output */
-	info->kerneltime = 1;
-	if (hook_mask & ~((1 << NF_IP_FORWARD) | (1 << NF_IP_LOCAL_OUT))) 
-		/* we use the skb time */
-		info->kerneltime = 0;
 
 	/* Check the size */
 	if (matchsize != IPT_ALIGN(sizeof(struct ipt_time_info)))

Re: ipt_time fixes (resend, sorry)

[Harald Welte]

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

On Tue, Apr 05, 2005 at 10:18:34AM +0200, Krzysztof Oledzki wrote:
> 
> 
> On Tue, 1 Feb 2005, Harald Welte wrote:
> 
> >On Fri, Jan 14, 2005 at 04:24:01PM +0100, Krzysztof Oledzki wrote:
> >>OK, How about...
> >Could you please coordinate with Fabrice Marie (the author of the
> >module?) and send me (personally) the final patch?
> 
> Hello,
> 
> I think it is good time to commit my ipt_time-fix-timestamp2.patch. I believe 
> it was ACKed by the author of ipt_time module:

yes.  Unfortunately the patch didn't apply:

patching file time/linux/net/ipv4/netfilter/ipt_time.c
Hunk #1 FAILED at 48.

can you please provide a current verison of the patc and send it as
mime-attachment?

thanks!
-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1229 bytes --]



On Sun, 10 Apr 2005, Harald Welte wrote:

> On Tue, Apr 05, 2005 at 10:18:34AM +0200, Krzysztof Oledzki wrote:
>>
>>
>> On Tue, 1 Feb 2005, Harald Welte wrote:
>>
>>> On Fri, Jan 14, 2005 at 04:24:01PM +0100, Krzysztof Oledzki wrote:
>>>> OK, How about...
>>> Could you please coordinate with Fabrice Marie (the author of the
>>> module?) and send me (personally) the final patch?
>>
>> Hello,
>>
>> I think it is good time to commit my ipt_time-fix-timestamp2.patch. I believe
>> it was ACKed by the author of ipt_time module:
>
> yes.  Unfortunately the patch didn't apply:
Hm... strange...

> patching file time/linux/net/ipv4/netfilter/ipt_time.c
> Hunk #1 FAILED at 48.
olenf@bizon:~/patch-o-matic-ng-20050411$ cat ../40-ipt_time-fix-timestamp2.patch|patch -p1
patching file time/linux-2.6/include/linux/netfilter_ipv4/ipt_time.h
patching file time/linux-2.6/net/ipv4/netfilter/ipt_time.c
Hunk #1 succeeded at 54 (offset -2 lines).

> can you please provide a current verison of the patc and send it as
> mime-attachment?
I sent it both as MIME-attachment (for easy patching) and inlinde (for 
easy reviewing). The first one should apply cleanly...

Best regards,

 			Krzysztof Olędzki

Re: ipt_time fixes (resend, sorry)

[Samuel Jean]

I wrote:
> I use an ugly hack of this kind:
> 
>    /* We might not have a timestamp, get one */
>    if (skb->stamp.tv_sec == 0)
>       do_gettimeofday((struct timeval *)&skb->stamp);
> 
>    now = timeval_to_jiffies(&skb->stamp);
> 
> 
>    if (struct->curr != now)
>       struct->prev = xchg(&struct->curr, now);
> 

Hey, just realized it really doesn't make sense in that module 
(ipt_time). Sorry, I think I was blinded by my current module.

Please discard my post.

Samuel

Re: ipt_time fixes (resend, sorry)

[Fabrice MARIE]

Hello,

On Tuesday 15 February 2005 09:20, Krzysztof Oledzki wrote:
> Any progress? ;-)

Yep. Sorry for the delay. The patch looks alright although I didn't test it.

As for the rest of the thread, right now like Brad mentioned, you need
2 rules to match the time period:
> To match 20:00 - 4:00 you currently need 2 rules:
>  1) match 20:00 - 23:59
>  2) match 0:00 - 4:00
> What I'm proposing is to allow this to be reduced to one rule.

The cleanest way to solve that in my opinion is to implement
the negative form of time match, in other words, to allow the usage
of the negative option (!) in the time match which is right now not allowed.
It shouldn't be very straight forward. On the other hand, this will require an additional
variable in the time structure passed from userspace to kernelspace,
and this would break binary compatibility with previously built iptables userpace
programs. But if you ask me, patches on extensions like ipt_time shouldn't bother
too much about backward compatibility headaches, since they are not shipped
in the kernel by default (and I guess never will). Backward compatibility for
standard-kernel-modules is important though.

Sorry again for the delay,

Have a nice day,

Fabrice.
--
"Silly hacker, root is for administrators" 
       -Unknown

Re: ipt_time fixes (resend, sorry)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1636 bytes --]



On Tue, 15 Feb 2005, Fabrice MARIE wrote:

>
> Hello,
>
> On Tuesday 15 February 2005 09:20, Krzysztof Oledzki wrote:
>> Any progress? ;-)
>
> Yep. Sorry for the delay. The patch looks alright although I didn't test it.
;-)

> As for the rest of the thread, right now like Brad mentioned, you need
> 2 rules to match the time period:
>> To match 20:00 - 4:00 you currently need 2 rules:
>>  1) match 20:00 - 23:59
>>  2) match 0:00 - 4:00
>> What I'm proposing is to allow this to be reduced to one rule.
>
> The cleanest way to solve that in my opinion is to implement
> the negative form of time match, in other words, to allow the usage
> of the negative option (!) in the time match which is right now not allowed.
True. Talking about negation of -m (match). Excuse me for my question, 
maybe this is obvious, but.. why we don't have a negation for -m 
(match) like for -p (protocol)?

> It shouldn't be very straight forward. On the other hand, this will require an additional
> variable in the time structure passed from userspace to kernelspace,
> and this would break binary compatibility with previously built iptables userpace
> programs.
Oh. With my patch we have free "u_int8_t  kerneltime" ;-) So no problem, 
we can easy store additional flag(s).

> But if you ask me, patches on extensions like ipt_time shouldn't bother
> too much about backward compatibility headaches, since they are not shipped
> in the kernel by default (and I guess never will). Backward compatibility for
> standard-kernel-modules is important though.
True, true ;)

Best regards,

 			Krzysztof Olędzki

Re: ipt_time fixes (resend, sorry)

[Fabrice MARIE]

Hi,

On Tuesday 15 February 2005 23:58, Krzysztof Oledzki wrote:
> True. Talking about negation of -m (match). Excuse me for my question, 
> maybe this is obvious, but.. why we don't have a negation for -m 
> (match) like for -p (protocol)?

There is one actually. If you look at the existing code of libipt_time.c
you'll see that I explicitly prevented the negation operator long time ago.
But it's very straight forward to enable it, and implement a check
in kernelspace to see if the match is negated or not prior to doing the matching.

Have a nice day,

Fabrice.
--
Fabrice MARIE

"Silly hacker, root is for administrators"
       -Unknown