[PATCH] netfilter: xt_LOG: avoid using old-style "<.>" printk prefix

[PATCH] netfilter: xt_LOG: avoid using old-style "<.>" printk prefix

[Romain Francoise]

Since commit 04d2c8c83d ("printk: convert the format for KERN_<LEVEL> to
a 2 byte pattern"), printk no longer uses a "<.>" string prefix
internally, so the call in xt_LOG ends up emitting the prefix as part of
the log itself (and possibly not at the configured level).

To avoid having to worry about printk's internal formatting, switch to
printk_emit(), specifying the desired level directly.

Signed-off-by: Romain Francoise <romain@orebokech.com>
---
 include/net/netfilter/xt_log.h |    7 +++++--
 net/netfilter/xt_LOG.c         |    6 +++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/xt_log.h b/include/net/netfilter/xt_log.h
index 9d9756c..343f5bb 100644
--- a/include/net/netfilter/xt_log.h
+++ b/include/net/netfilter/xt_log.h
@@ -39,11 +39,14 @@ static struct sbuff *sb_open(void)
 	return m;
 }
 
-static void sb_close(struct sbuff *m)
+static void sb_emit(struct sbuff *m, int level)
 {
 	m->buf[m->count] = 0;
-	printk("%s\n", m->buf);
+	printk_emit(0, level, NULL, 0, "%s\n", m->buf);
+}
 
+static void sb_close(struct sbuff *m)
+{
 	if (likely(m != &emergency))
 		kfree(m);
 	else {
diff --git a/net/netfilter/xt_LOG.c b/net/netfilter/xt_LOG.c
index ff5f75f..d33ff9f 100644
--- a/net/netfilter/xt_LOG.c
+++ b/net/netfilter/xt_LOG.c
@@ -436,7 +436,7 @@ log_packet_common(struct sbuff *m,
 		  const struct nf_loginfo *loginfo,
 		  const char *prefix)
 {
-	sb_add(m, "<%d>%sIN=%s OUT=%s ", loginfo->u.log.level,
+	sb_add(m, "%sIN=%s OUT=%s ",
 	       prefix,
 	       in ? in->name : "",
 	       out ? out->name : "");
@@ -477,6 +477,8 @@ ipt_log_packet(u_int8_t pf,
 
 	dump_ipv4_packet(m, loginfo, skb, 0);
 
+	sb_emit(m, loginfo->u.log.level);
+
 	sb_close(m);
 }
 
@@ -807,6 +809,8 @@ ip6t_log_packet(u_int8_t pf,
 
 	dump_ipv6_packet(m, loginfo, skb, skb_network_offset(skb), 1);
 
+	sb_emit(m, loginfo->u.log.level);
+
 	sb_close(m);
 }
 #endif
-- 
1.7.10.4

Re: [PATCH] netfilter: xt_LOG: avoid using old-style "<.>" printk prefix

[Pablo Neira Ayuso]

Hi,

CC'ing Joe Perches and Eric Dumazet, they are discussing a similar
patch.

Would you be OK with the prink_emit variant to solve this? (Once
several issues are resolved).

On Mon, Sep 10, 2012 at 07:29:06PM +0200, Romain Francoise wrote:
> Since commit 04d2c8c83d ("printk: convert the format for KERN_<LEVEL> to
> a 2 byte pattern"), printk no longer uses a "<.>" string prefix
> internally, so the call in xt_LOG ends up emitting the prefix as part of
> the log itself (and possibly not at the configured level).
> 
> To avoid having to worry about printk's internal formatting, switch to
> printk_emit(), specifying the desired level directly.

ebt_log chunk is missing.

> Signed-off-by: Romain Francoise <romain@orebokech.com>
> ---
>  include/net/netfilter/xt_log.h |    7 +++++--
>  net/netfilter/xt_LOG.c         |    6 +++++-
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/netfilter/xt_log.h b/include/net/netfilter/xt_log.h
> index 9d9756c..343f5bb 100644
> --- a/include/net/netfilter/xt_log.h
> +++ b/include/net/netfilter/xt_log.h
> @@ -39,11 +39,14 @@ static struct sbuff *sb_open(void)
>  	return m;
>  }
>  
> -static void sb_close(struct sbuff *m)
> +static void sb_emit(struct sbuff *m, int level)
>  {
>  	m->buf[m->count] = 0;
> -	printk("%s\n", m->buf);
> +	printk_emit(0, level, NULL, 0, "%s\n", m->buf);

I think it should be printk_emit(-1, ... Note facility is -1.

Thus, we skip the syslog prefix stripping (we can skip it and save
some cycles).

> +}
>  
> +static void sb_close(struct sbuff *m)
> +{
>  	if (likely(m != &emergency))
>  		kfree(m);
>  	else {
> diff --git a/net/netfilter/xt_LOG.c b/net/netfilter/xt_LOG.c
> index ff5f75f..d33ff9f 100644
> --- a/net/netfilter/xt_LOG.c
> +++ b/net/netfilter/xt_LOG.c
> @@ -436,7 +436,7 @@ log_packet_common(struct sbuff *m,
>  		  const struct nf_loginfo *loginfo,
>  		  const char *prefix)
>  {
> -	sb_add(m, "<%d>%sIN=%s OUT=%s ", loginfo->u.log.level,
> +	sb_add(m, "%sIN=%s OUT=%s ",
>  	       prefix,
>  	       in ? in->name : "",
>  	       out ? out->name : "");
> @@ -477,6 +477,8 @@ ipt_log_packet(u_int8_t pf,
>  
>  	dump_ipv4_packet(m, loginfo, skb, 0);
>  
> +	sb_emit(m, loginfo->u.log.level);
> +
>  	sb_close(m);
>  }
>  
> @@ -807,6 +809,8 @@ ip6t_log_packet(u_int8_t pf,
>  
>  	dump_ipv6_packet(m, loginfo, skb, skb_network_offset(skb), 1);
>  
> +	sb_emit(m, loginfo->u.log.level);
> +
>  	sb_close(m);
>  }
>  #endif
> -- 
> 1.7.10.4
> 
> 
> --
> 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: xt_LOG: avoid using old-style "<.>" printk prefix

[Romain Francoise]

Hi Pablo,

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> ebt_log chunk is missing.

Sorry, didn't notice that it has the same bug. Do you want that in the
same patch, or separate?

>>  	m->buf[m->count] = 0;
>> -	printk("%s\n", m->buf);
>> +	printk_emit(0, level, NULL, 0, "%s\n", m->buf);

> I think it should be printk_emit(-1, ... Note facility is -1.

> Thus, we skip the syslog prefix stripping (we can skip it and save
> some cycles).

I don't think that's possible, the facility is copied down to user-space
consumers and it has to be zero for the message to be identified as
originating from the kernel (0 is LOG_KERN).

As it boils down to a single test for KERN_SOH_ASCII I don't think it
matters very much.

Thanks!

Re: [PATCH] netfilter: xt_LOG: avoid using old-style "<.>" printk prefix

[Pablo Neira Ayuso]

Replying from different email, @netfilter.org is currently down due to
some power supply problems.

On Wed, Sep 12, 2012 at 03:29:39PM +0200, Romain Francoise wrote:
> Hi Pablo,
> 
> Pablo Neira Ayuso <pablo@netfilter.org> writes:
> 
> > ebt_log chunk is missing.
> 
> Sorry, didn't notice that it has the same bug. Do you want that in the
> same patch, or separate?
> 
> >>  	m->buf[m->count] = 0;
> >> -	printk("%s\n", m->buf);
> >> +	printk_emit(0, level, NULL, 0, "%s\n", m->buf);
> 
> > I think it should be printk_emit(-1, ... Note facility is -1.
> 
> > Thus, we skip the syslog prefix stripping (we can skip it and save
> > some cycles).
> 
> I don't think that's possible, the facility is copied down to user-space
> consumers and it has to be zero for the message to be identified as
> originating from the kernel (0 is LOG_KERN).

I see.

> As it boils down to a single test for KERN_SOH_ASCII I don't think it
> matters very much.

I'm going to take Joe's patch, I prefer using the printk interface.

Thanks anyway.