Re: [PATCH] netfilter: Fix compiler warning.

Re: [PATCH] netfilter: Fix compiler warning.

[David Miller]

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)

Re: [PATCH] netfilter: Fix compiler warning.

[Eric Dumazet]

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);

Re: [PATCH] netfilter: Fix compiler warning.

[Patrick McHardy]

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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: Fix compiler warning.

[Laurent Chavey]

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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] netfilter: Fix compiler warning.

[Jan Engelhardt]

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?

Re: [PATCH] netfilter: Fix compiler warning.

[Eric Dumazet]

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 netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] netfilter: Fix compiler warning.

[chavey]

Fix compiler warning "discards qualifiers from pointer target type",
by allowing explicit discard of const qualifier thru type casting.
The const_cast() macro is taken from a patch from Kaveh R. Ghazi
[PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro
posted on 
http://old.nabble.com/-PATCH-:-Fix-problematic--Wcast-qual-cases-using-new-CONST_CAST--macro-td11824676.html
The proposed __nowarn__ keyword has not yet been implemented, 
(see http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01261.html).


Signed-off-by: Laurent Chavey <chavey@google.com>
---
 include/linux/kernel.h        |   12 ++++++++++++
 net/ipv4/netfilter/ipt_ULOG.c |    2 +-
 net/netfilter/xt_time.c       |    2 +-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 3fa4c59..0246771 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -710,4 +710,16 @@ struct sysinfo {
 # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
 #endif
 
+/* Cast away const-ness to pass -Wcast-qual */
+#ifdef __GNUC__
+union gcc_constcast
+{
+	const void *cv;
+	void *v;
+};
+#define const_cast(X) ((__extension__(union gcc_constcast)(const void *)(X)).v)
+#else
+#define const_cast(X) ((void *)(X))
+#endif
+
 #endif
diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c
index d32cc4b..c8f8b8b 100644
--- a/net/ipv4/netfilter/ipt_ULOG.c
+++ b/net/ipv4/netfilter/ipt_ULOG.c
@@ -210,7 +210,7 @@ static void ipt_ulog_packet(unsigned int hooknum,
 
 	/* We might not have a timestamp, get one */
 	if (skb->tstamp.tv64 == 0)
-		__net_timestamp((struct sk_buff *)skb);
+		__net_timestamp((struct sk_buff *)const_cast(skb));
 
 	/* copy hook, prefix, timestamp, payload, etc. */
 	pm->data_len = copy_len;
diff --git a/net/netfilter/xt_time.c b/net/netfilter/xt_time.c
index 93acaa5..8f34a4a 100644
--- a/net/netfilter/xt_time.c
+++ b/net/netfilter/xt_time.c
@@ -170,7 +170,7 @@ time_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 	 * it arrived at the right moment before 13:00.
 	 */
 	if (skb->tstamp.tv64 == 0)
-		__net_timestamp((struct sk_buff *)skb);
+		__net_timestamp((struct sk_buff *)const_cast(skb));
 
 	stamp = ktime_to_ns(skb->tstamp);
 	stamp = div_s64(stamp, NSEC_PER_SEC);

Re: [PATCH] netfilter: Fix compiler warning.

[Jan Engelhardt]

On Donnerstag 2009-12-10 22:44, chavey@google.com wrote:

>Fix compiler warning "discards qualifiers from pointer target type",
>by allowing explicit discard of const qualifier thru type casting.
>The const_cast() macro is taken from a patch from Kaveh R. Ghazi

Uhm, I am not getting any warning on xt_time with either gcc-4.4.1
or gcc-4.5_20091126, so what case are we actually trying to fix?

>--- a/net/netfilter/xt_time.c
>+++ b/net/netfilter/xt_time.c
>@@ -170,7 +170,7 @@ time_mt(const struct sk_buff *skb, const struct xt_match_param *par)
> 	 * it arrived at the right moment before 13:00.
> 	 */
> 	if (skb->tstamp.tv64 == 0)
>-		__net_timestamp((struct sk_buff *)skb);
>+		__net_timestamp((struct sk_buff *)const_cast(skb));
> 
> 	stamp = ktime_to_ns(skb->tstamp);
> 	stamp = div_s64(stamp, NSEC_PER_SEC);

Re: [PATCH] netfilter: Fix compiler warning.

[David Miller]

From: Jan Engelhardt <jengelh@medozas.de>
Date: Thu, 10 Dec 2009 23:06:32 +0100 (CET)

> 
> On Donnerstag 2009-12-10 22:44, chavey@google.com wrote:
> 
>>Fix compiler warning "discards qualifiers from pointer target type",
>>by allowing explicit discard of const qualifier thru type casting.
>>The const_cast() macro is taken from a patch from Kaveh R. Ghazi
> 
> Uhm, I am not getting any warning on xt_time with either gcc-4.4.1
> or gcc-4.5_20091126, so what case are we actually trying to fix?

I get this warning with gcc-4.3.x all the time.

Re: [PATCH] netfilter: Fix compiler warning.

[Jan Engelhardt]

On Donnerstag 2009-12-10 23:08, David Miller wrote:
>> 
>>>Fix compiler warning "discards qualifiers from pointer target type",
>>>by allowing explicit discard of const qualifier thru type casting.
>>>The const_cast() macro is taken from a patch from Kaveh R. Ghazi
>> 
>> Uhm, I am not getting any warning on xt_time with either gcc-4.4.1
>> or gcc-4.5_20091126, so what case are we actually trying to fix?
>
>I get this warning with gcc-4.3.x all the time.

Ok. Still, assuming it was a "bug" which got fixed in gcc-4.4,
should we really bother with it anymore? (Thankfully it's just
a warning, and not an error.)
If so, we can probably do better code-wise.

Re: [PATCH] netfilter: Fix compiler warning.

[Jan Engelhardt]

On Donnerstag 2009-12-10 23:08, David Miller wrote:
>> On Donnerstag 2009-12-10 22:44, chavey@google.com wrote:
>> 
>>>Fix compiler warning "discards qualifiers from pointer target type",
>>>by allowing explicit discard of const qualifier thru type casting.
>>>The const_cast() macro is taken from a patch from Kaveh R. Ghazi
>> 
>> Uhm, I am not getting any warning on xt_time with either gcc-4.4.1
>> or gcc-4.5_20091126, so what case are we actually trying to fix?
>
>I get this warning with gcc-4.3.x all the time.

Seems they fixed it in gcc-4.3 too?

00:41 borg:~/code/linux > make net/netfilter/xt_time.o -j8 CC=gcc-4.3
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CC      scripts/mod/empty.o
  MKELF   scripts/mod/elfconfig.h
  HOSTCC  scripts/mod/file2alias.o
  HOSTCC  scripts/mod/modpost.o
  HOSTCC  scripts/mod/sumversion.o
  HOSTLD  scripts/mod/modpost
  CC      kernel/bounds.s
  GEN     include/linux/bounds.h
  CC      arch/x86/kernel/asm-offsets.s
  GEN     include/asm/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC [M]  net/netfilter/xt_time.o
00:42 borg:~/code/linux > gcc-4.3 -v
Using built-in specs.
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info 
--mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64 
--enable-languages=c,c++,objc,fortran,obj-c++,java,ada 
--enable-checking=release --with-gxx-include-dir=/usr/include/c++/4.3 
--enable-ssp --disable-libssp --with-bugurl=http://bugs.opensuse.org/ 
--with-pkgversion='SUSE Linux' --disable-libgcj --disable-libmudflap 
--with-slibdir=/lib64 --with-system-zlib --enable-__cxa_atexit 
--enable-libstdcxx-allocator=new --disable-libstdcxx-pch 
--enable-version-specific-runtime-libs --program-suffix=-4.3 
--enable-linux-futex --without-system-libunwind --with-cpu=generic 
--build=x86_64-suse-linux
Thread model: posix
gcc version 4.3.4 (SUSE Linux)

Re: [PATCH] netfilter: Fix compiler warning.

[Patrick McHardy]

Jan Engelhardt wrote:
> On Donnerstag 2009-12-10 23:08, David Miller wrote:
>>> On Donnerstag 2009-12-10 22:44, chavey@google.com wrote:
>>>
>>>> Fix compiler warning "discards qualifiers from pointer target type",
>>>> by allowing explicit discard of const qualifier thru type casting.
>>>> The const_cast() macro is taken from a patch from Kaveh R. Ghazi
>>> Uhm, I am not getting any warning on xt_time with either gcc-4.4.1
>>> or gcc-4.5_20091126, so what case are we actually trying to fix?
>> I get this warning with gcc-4.3.x all the time.
> 
> Seems they fixed it in gcc-4.3 too?
> 
> 00:41 borg:~/code/linux > make net/netfilter/xt_time.o -j8 CC=gcc-4.3

Yes, I haven't seen it for a long time either, currently using
gcc version 4.3.4 (Debian 4.3.4-6) and gcc version 4.4.1
(Debian 4.4.1-3)

I don't think we need to bother with this.

Re: [PATCH] netfilter: Fix compiler warning.

[Andrew Morton]

On Thu, 10 Dec 2009 13:44:50 -0800
chavey@google.com wrote:

> Fix compiler warning "discards qualifiers from pointer target type",
> by allowing explicit discard of const qualifier thru type casting.
> The const_cast() macro is taken from a patch from Kaveh R. Ghazi
> [PATCH]: Fix problematic -Wcast-qual cases using new CONST_CAST macro
> posted on 
> http://old.nabble.com/-PATCH-:-Fix-problematic--Wcast-qual-cases-using-new-CONST_CAST--macro-td11824676.html
> The proposed __nowarn__ keyword has not yet been implemented, 
> (see http://gcc.gnu.org/ml/gcc-patches/2009-11/msg01261.html).
> 
> 
> Signed-off-by: Laurent Chavey <chavey@google.com>
> ---
>  include/linux/kernel.h        |   12 ++++++++++++
>  net/ipv4/netfilter/ipt_ULOG.c |    2 +-
>  net/netfilter/xt_time.c       |    2 +-
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 3fa4c59..0246771 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -710,4 +710,16 @@ struct sysinfo {
>  # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
>  #endif
>  
> +/* Cast away const-ness to pass -Wcast-qual */

This comment simply doesn't make sense.

I don't think it adequately explains why the macro exists, how and when
one should use it, etc.

> +#ifdef __GNUC__
> +union gcc_constcast
> +{

union gcc_constcast {

> +	const void *cv;
> +	void *v;
> +};
> +#define const_cast(X) ((__extension__(union gcc_constcast)(const void *)(X)).v)
> +#else
> +#define const_cast(X) ((void *)(X))
> +#endif

Why don't we just use the simple version on all compilers?  What's the
advantage in using the gcc-specific extension?

And I was wrong - this should be implemented in linux/compiler.h.  We
have infrastructure there to separate gcc-specific and non-gcc
implementations of the same interface.

Finally, are we sure that __extension__(union gcc_constcast) is
available on all gcc versions mentioned in Documentation/Changes?  If
not, this patch should use include/linux/compiler-gcc4.h, etc.