Patchwork netfilter: Fix compiler warning.

login
register
mail settings
Submitter laurent chavey
Date Dec. 10, 2009, 1:25 a.m.
Message ID <pvmhbrzboyb.fsf@chavey.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/40773/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

laurent chavey - Dec. 10, 2009, 1:25 a.m.
Fix compiler warning "discards qualifiers from pointer target type".
The function prototype defines parameters as pointer to a constant.
Such parameters should not have their content modified in the
function.

Signed-off-by: Laurent Chavey <chavey@google.com>
---
 net/ipv4/netfilter/ipt_ULOG.c |    6 ++++--
 net/netfilter/xt_time.c       |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

--
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
David Miller - Dec. 10, 2009, 2:07 a.m.
From: chavey@google.com
Date: Wed, 09 Dec 2009 17:25:00 -0800

> Fix compiler warning "discards qualifiers from pointer target type".
> The function prototype defines parameters as pointer to a constant.
> Such parameters should not have their content modified in the
> function.
> 
> Signed-off-by: Laurent Chavey <chavey@google.com>

Please CC: netfilter patches to netfilter-devel, added...

> ---
>  net/ipv4/netfilter/ipt_ULOG.c |    6 ++++--
>  net/netfilter/xt_time.c       |    6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
> index d32cc4b..8490f9f 100644
> --- a/net/ipv4/netfilter/ipt_ULOG.c
> +++ b/net/ipv4/netfilter/ipt_ULOG.c
> @@ -166,6 +166,7 @@ static void ipt_ulog_packet(unsigned int hooknum,
>  	size_t size, copy_len;
>  	struct nlmsghdr *nlh;
>  	struct timeval tv;
> +	ktime_t	tstamp;
>  
>  	/* ffs == find first bit set, necessary because userspace
>  	 * is already shifting groupnumber, but we need unshifted.
> @@ -208,13 +209,14 @@ static void ipt_ulog_packet(unsigned int hooknum,
>  
>  	pm = NLMSG_DATA(nlh);
>  
> +	tstamp = skb->tstamp;
>  	/* We might not have a timestamp, get one */
>  	if (skb->tstamp.tv64 == 0)
> -		__net_timestamp((struct sk_buff *)skb);
> +		tstamp = ktime_get_real();
>  
>  	/* copy hook, prefix, timestamp, payload, etc. */
>  	pm->data_len = copy_len;
> -	tv = ktime_to_timeval(skb->tstamp);
> +	tv = ktime_to_timeval(tstamp);
>  	put_unaligned(tv.tv_sec, &pm->timestamp_sec);
>  	put_unaligned(tv.tv_usec, &pm->timestamp_usec);
>  	put_unaligned(skb->mark, &pm->mark);
> diff --git a/net/netfilter/xt_time.c b/net/netfilter/xt_time.c
> index 93acaa5..66e2a75 100644
> --- a/net/netfilter/xt_time.c
> +++ b/net/netfilter/xt_time.c
> @@ -159,6 +159,7 @@ time_mt(const struct sk_buff *skb, const struct xt_match_param *par)
>  	unsigned int packet_time;
>  	struct xtm current_time;
>  	s64 stamp;
> +	ktime_t	tstamp;
>  
>  	/*
>  	 * We cannot use get_seconds() instead of __net_timestamp() here.
> @@ -169,10 +170,11 @@ time_mt(const struct sk_buff *skb, const struct xt_match_param *par)
>  	 * may happen that the same packet matches both rules if
>  	 * it arrived at the right moment before 13:00.
>  	 */
> +	tstamp = skb->tstamp;
>  	if (skb->tstamp.tv64 == 0)
> -		__net_timestamp((struct sk_buff *)skb);
> +		tstamp = ktime_get_real();
>  
> -	stamp = ktime_to_ns(skb->tstamp);
> +	stamp = ktime_to_ns(tstamp);
>  	stamp = div_s64(stamp, NSEC_PER_SEC);
>  
>  	if (info->flags & XT_TIME_LOCAL_TZ)
--
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
Eric Dumazet - Dec. 10, 2009, 4:25 a.m.
Le 10/12/2009 02:25, chavey@google.com a écrit :
> Fix compiler warning "discards qualifiers from pointer target type".
> The function prototype defines parameters as pointer to a constant.
> Such parameters should not have their content modified in the
> function.
> 
> Signed-off-by: Laurent Chavey <chavey@google.com>

This is not the right fix IMHO.

We want an unique timestamp for the whole netfilter matches, because several 'time' rules
could get 'interesting' effects.

The 'const' attribute is a debugging aid, and the skb->tstamp 'write-once' is a valid exception.

Read again the comment in time_mt() :

vi +163 net/netfilter/xt_time.c

static bool
time_mt(const struct sk_buff *skb, const struct xt_match_param *par)
{
        const struct xt_time_info *info = par->matchinfo;
        unsigned int packet_time;
        struct xtm current_time;
        s64 stamp;

        /*
         * We cannot use get_seconds() instead of __net_timestamp() here.
         * Suppose you have two rules:
         *      1. match before 13:00
         *      2. match after 13:00
         * If you match against processing time (get_seconds) it
         * may happen that the same packet matches both rules if
         * it arrived at the right moment before 13:00.
         */
        if (skb->tstamp.tv64 == 0)
                __net_timestamp((struct sk_buff *)skb);

--
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
Patrick McHardy - Dec. 10, 2009, 11:26 a.m.
Eric Dumazet wrote:
> Le 10/12/2009 02:25, chavey@google.com a écrit :
>> Fix compiler warning "discards qualifiers from pointer target type".
>> The function prototype defines parameters as pointer to a constant.
>> Such parameters should not have their content modified in the
>> function.
>>
>> Signed-off-by: Laurent Chavey <chavey@google.com>
> 
> This is not the right fix IMHO.
> 
> We want an unique timestamp for the whole netfilter matches, because several 'time' rules
> could get 'interesting' effects.
> 
> The 'const' attribute is a debugging aid, and the skb->tstamp 'write-once' is a valid exception.

I agree, the timestamps should be consistent among multiple
matches. The cast should also surpress the warning, but some
gcc versions ignore it.
--
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
laurent chavey - Dec. 10, 2009, 5:50 p.m.
On Wed, Dec 9, 2009 at 8:25 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le 10/12/2009 02:25, chavey@google.com a écrit :
>> Fix compiler warning "discards qualifiers from pointer target type".
>> The function prototype defines parameters as pointer to a constant.
>> Such parameters should not have their content modified in the
>> function.
>>
>> Signed-off-by: Laurent Chavey <chavey@google.com>
>
> This is not the right fix IMHO.
>
> We want an unique timestamp for the whole netfilter matches, because several 'time' rules
> could get 'interesting' effects.
>
> The 'const' attribute is a debugging aid, and the skb->tstamp 'write-once' is a valid exception.
>
> Read again the comment in time_mt() :

good point.  I  agree with the need for the exception, I would just
like it to be more explicit
in the code itself (like a turn off check around that particular
statement) so we do not have to
scrub thru the compiler output to filter out good / bad warning.
question:
  why do we not force the timestamp in the skb before going thru the
chain ? it looks to me
  that the check for (skb->tstamp.tv64 == 0) should be done once
>
> vi +163 net/netfilter/xt_time.c
>
> static bool
> time_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> {
>        const struct xt_time_info *info = par->matchinfo;
>        unsigned int packet_time;
>        struct xtm current_time;
>        s64 stamp;
>
>        /*
>         * We cannot use get_seconds() instead of __net_timestamp() here.
>         * Suppose you have two rules:
>         *      1. match before 13:00
>         *      2. match after 13:00
>         * If you match against processing time (get_seconds) it
>         * may happen that the same packet matches both rules if
>         * it arrived at the right moment before 13:00.
>         */
>        if (skb->tstamp.tv64 == 0)
>                __net_timestamp((struct sk_buff *)skb);
>
>
--
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
Jan Engelhardt - Dec. 10, 2009, 6:53 p.m.
On Donnerstag 2009-12-10 18:50, Laurent Chavey wrote:
>
>good point.  I  agree with the need for the exception, I would just
>like it to be more explicit in the code itself (like a turn off
>check around that particular statement) so we do not have to scrub
>thru the compiler output to filter out good / bad warning. question:
>  why do we not force the timestamp in the skb before going thru the
>chain ?

Because it is probably too expensive to do, unless you employ
things like xt_time?
--
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
Eric Dumazet - Dec. 10, 2009, 7:31 p.m.
Le 10/12/2009 19:53, Jan Engelhardt a écrit :
> 
> On Donnerstag 2009-12-10 18:50, Laurent Chavey wrote:
>>
>> good point.  I  agree with the need for the exception, I would just
>> like it to be more explicit in the code itself (like a turn off
>> check around that particular statement) so we do not have to scrub
>> thru the compiler output to filter out good / bad warning. question:
>>  why do we not force the timestamp in the skb before going thru the
>> chain ?
> 
> Because it is probably too expensive to do, unless you employ
> things like xt_time?

Right. On some platforms, timestamps are quite expensive (say, a fraction 
of one micro second to get one of them), we try to avoid them as much as possible.
--
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

Patch

diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index d32cc4b..8490f9f 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -166,6 +166,7 @@  static void ipt_ulog_packet(unsigned int hooknum,
 	size_t size, copy_len;
 	struct nlmsghdr *nlh;
 	struct timeval tv;
+	ktime_t	tstamp;
 
 	/* ffs == find first bit set, necessary because userspace
 	 * is already shifting groupnumber, but we need unshifted.
@@ -208,13 +209,14 @@  static void ipt_ulog_packet(unsigned int hooknum,
 
 	pm = NLMSG_DATA(nlh);
 
+	tstamp = skb->tstamp;
 	/* We might not have a timestamp, get one */
 	if (skb->tstamp.tv64 == 0)
-		__net_timestamp((struct sk_buff *)skb);
+		tstamp = ktime_get_real();
 
 	/* copy hook, prefix, timestamp, payload, etc. */
 	pm->data_len = copy_len;
-	tv = ktime_to_timeval(skb->tstamp);
+	tv = ktime_to_timeval(tstamp);
 	put_unaligned(tv.tv_sec, &pm->timestamp_sec);
 	put_unaligned(tv.tv_usec, &pm->timestamp_usec);
 	put_unaligned(skb->mark, &pm->mark);
diff --git a/net/netfilter/xt_time.c b/net/netfilter/xt_time.c
index 93acaa5..66e2a75 100644
--- a/net/netfilter/xt_time.c
+++ b/net/netfilter/xt_time.c
@@ -159,6 +159,7 @@  time_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 	unsigned int packet_time;
 	struct xtm current_time;
 	s64 stamp;
+	ktime_t	tstamp;
 
 	/*
 	 * We cannot use get_seconds() instead of __net_timestamp() here.
@@ -169,10 +170,11 @@  time_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 	 * may happen that the same packet matches both rules if
 	 * it arrived at the right moment before 13:00.
 	 */
+	tstamp = skb->tstamp;
 	if (skb->tstamp.tv64 == 0)
-		__net_timestamp((struct sk_buff *)skb);
+		tstamp = ktime_get_real();
 
-	stamp = ktime_to_ns(skb->tstamp);
+	stamp = ktime_to_ns(tstamp);
 	stamp = div_s64(stamp, NSEC_PER_SEC);
 
 	if (info->flags & XT_TIME_LOCAL_TZ)