[PATCH nf-next 0/4] netfilter: request related nf_log module when we add TRACE rule

[PATCH nf-next 0/4] netfilter: request related nf_log module when we add TRACE rule

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

This patch set solve such problem, for example, if we add a following
iptables rule:
  # iptables -t raw -I PREROUTING -j TRACE
And nf_log_ipv4 kernel module is not installed, no trace log
will be generated, until we install the nf_log_ipv4 module manully.

This is not friendly, so we add nf_logger_find_get call explicitly
when xt_TRACE target is created. Nft nftrace meta has the same
problem.

And in order to avoid special treatment of NFPROTO_INET family again
and again, I move the special logic to the inside of nf_logger_find_get
and nf_logger_put, so caller can ignore it. 

Liping Zhang (4):
  netfilter: nf_log: handle NFPROTO_INET properly in
    nf_logger_[find_get|put]
  netfilter: nft_log: no need to deal with NFPROTO_INET family
  netfilter: xt_TRACE: add explicitly nf_logger_find_get call
  netfilter: nft_meta: add explicitly nf_logger_find_get call

 net/netfilter/nf_log.c   | 20 ++++++++++++++++++++
 net/netfilter/nft_log.c  | 21 +--------------------
 net/netfilter/nft_meta.c | 12 ++++++++++--
 net/netfilter/xt_TRACE.c | 25 +++++++++++++++++++------
 4 files changed, 50 insertions(+), 28 deletions(-)

-- 
2.5.5

[PATCH nf-next 1/4] netfilter: nf_log: handle NFPROTO_INET properly in nf_logger_[find_get|put]

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

When we request NFPROTO_INET, it means both NFPROTO_IPV4 and NFPROTO_IPV6.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nf_log.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index a5d41df..73b845d 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -159,6 +159,20 @@ int nf_logger_find_get(int pf, enum nf_log_type type)
 	struct nf_logger *logger;
 	int ret = -ENOENT;
 
+	if (pf == NFPROTO_INET) {
+		ret = nf_logger_find_get(NFPROTO_IPV4, type);
+		if (ret < 0)
+			return ret;
+
+		ret = nf_logger_find_get(NFPROTO_IPV6, type);
+		if (ret < 0) {
+			nf_logger_put(NFPROTO_IPV4, type);
+			return ret;
+		}
+
+		return 0;
+	}
+
 	if (rcu_access_pointer(loggers[pf][type]) == NULL)
 		request_module("nf-logger-%u-%u", pf, type);
 
@@ -179,6 +193,12 @@ void nf_logger_put(int pf, enum nf_log_type type)
 {
 	struct nf_logger *logger;
 
+	if (pf == NFPROTO_INET) {
+		nf_logger_put(NFPROTO_IPV4, type);
+		nf_logger_put(NFPROTO_IPV6, type);
+		return;
+	}
+
 	BUG_ON(loggers[pf][type] == NULL);
 
 	rcu_read_lock();
-- 
2.5.5

Re: [PATCH nf-next 1/4] netfilter: nf_log: handle NFPROTO_INET properly in nf_logger_[find_get|put]

[Pablo Neira Ayuso]

On Wed, Jun 08, 2016 at 08:43:17PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> When we request NFPROTO_INET, it means both NFPROTO_IPV4 and NFPROTO_IPV6.
>
> Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
> ---
>  net/netfilter/nf_log.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index a5d41df..73b845d 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -159,6 +159,20 @@ int nf_logger_find_get(int pf, enum nf_log_type type)
>  	struct nf_logger *logger;
>  	int ret = -ENOENT;
>  
> +	if (pf == NFPROTO_INET) {
> +		ret = nf_logger_find_get(NFPROTO_IPV4, type);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = nf_logger_find_get(NFPROTO_IPV6, type);
> +		if (ret < 0) {
> +			nf_logger_put(NFPROTO_IPV4, type);
> +			return ret;
> +		}
> +
> +		return 0;

This is already done from nft_log_init().

Are you observing any problem there?

Re: [PATCH nf-next 1/4] netfilter: nf_log: handle NFPROTO_INET properly in nf_logger_[find_get|put]

[Pablo Neira Ayuso]

On Thu, Jun 23, 2016 at 01:22:05PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jun 08, 2016 at 08:43:17PM +0800, Liping Zhang wrote:
> > From: Liping Zhang <liping.zhang@spreadtrum.com>
> > 
> > When we request NFPROTO_INET, it means both NFPROTO_IPV4 and NFPROTO_IPV6.
> >
> > Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
> > ---
> >  net/netfilter/nf_log.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> > index a5d41df..73b845d 100644
> > --- a/net/netfilter/nf_log.c
> > +++ b/net/netfilter/nf_log.c
> > @@ -159,6 +159,20 @@ int nf_logger_find_get(int pf, enum nf_log_type type)
> >  	struct nf_logger *logger;
> >  	int ret = -ENOENT;
> >  
> > +	if (pf == NFPROTO_INET) {
> > +		ret = nf_logger_find_get(NFPROTO_IPV4, type);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = nf_logger_find_get(NFPROTO_IPV6, type);
> > +		if (ret < 0) {
> > +			nf_logger_put(NFPROTO_IPV4, type);
> > +			return ret;
> > +		}
> > +
> > +		return 0;
> 
> This is already done from nft_log_init().
> 
> Are you observing any problem there?

Oh, I see, you get rid of that code in your follow up patch.

I'm going to merge 1/4 and 2/4, this change should be introduced in
one go as they are part of the same logical change.

No need to resend. Thanks.

[PATCH nf-next 2/4] netfilter: nft_log: no need to deal with NFPROTO_INET family

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Since nf_logger_find_get and nf_logger_put can handle NFPROTO_INET
properly, we can remove the special process logic now.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nft_log.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index 319c22b..713d668 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -52,7 +52,6 @@ static int nft_log_init(const struct nft_ctx *ctx,
 	struct nft_log *priv = nft_expr_priv(expr);
 	struct nf_loginfo *li = &priv->loginfo;
 	const struct nlattr *nla;
-	int ret;
 
 	nla = tb[NFTA_LOG_PREFIX];
 	if (nla != NULL) {
@@ -97,19 +96,6 @@ static int nft_log_init(const struct nft_ctx *ctx,
 		break;
 	}
 
-	if (ctx->afi->family == NFPROTO_INET) {
-		ret = nf_logger_find_get(NFPROTO_IPV4, li->type);
-		if (ret < 0)
-			return ret;
-
-		ret = nf_logger_find_get(NFPROTO_IPV6, li->type);
-		if (ret < 0) {
-			nf_logger_put(NFPROTO_IPV4, li->type);
-			return ret;
-		}
-		return 0;
-	}
-
 	return nf_logger_find_get(ctx->afi->family, li->type);
 }
 
@@ -122,12 +108,7 @@ static void nft_log_destroy(const struct nft_ctx *ctx,
 	if (priv->prefix != nft_log_null_prefix)
 		kfree(priv->prefix);
 
-	if (ctx->afi->family == NFPROTO_INET) {
-		nf_logger_put(NFPROTO_IPV4, li->type);
-		nf_logger_put(NFPROTO_IPV6, li->type);
-	} else {
-		nf_logger_put(ctx->afi->family, li->type);
-	}
+	nf_logger_put(ctx->afi->family, li->type);
 }
 
 static int nft_log_dump(struct sk_buff *skb, const struct nft_expr *expr)
-- 
2.5.5

[PATCH nf-next 3/4] netfilter: xt_TRACE: add explicitly nf_logger_find_get call

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Consider such situation, if nf_log_ipv4 kernel module is not installed,
and the user add a following iptables rule:
  # iptables -t raw -I PREROUTING -j TRACE

There will be no trace log generated until the user install nf_log_ipv4
module manully. So we should add request related nf_log module
appropriately here.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/xt_TRACE.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/xt_TRACE.c b/net/netfilter/xt_TRACE.c
index df48967..858d189 100644
--- a/net/netfilter/xt_TRACE.c
+++ b/net/netfilter/xt_TRACE.c
@@ -4,12 +4,23 @@
 #include <linux/skbuff.h>
 
 #include <linux/netfilter/x_tables.h>
+#include <net/netfilter/nf_log.h>
 
 MODULE_DESCRIPTION("Xtables: packet flow tracing");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("ipt_TRACE");
 MODULE_ALIAS("ip6t_TRACE");
 
+static int trace_tg_check(const struct xt_tgchk_param *par)
+{
+	return nf_logger_find_get(par->family, NF_LOG_TYPE_LOG);
+}
+
+static void trace_tg_destroy(const struct xt_tgdtor_param *par)
+{
+	nf_logger_put(par->family, NF_LOG_TYPE_LOG);
+}
+
 static unsigned int
 trace_tg(struct sk_buff *skb, const struct xt_action_param *par)
 {
@@ -18,12 +29,14 @@ trace_tg(struct sk_buff *skb, const struct xt_action_param *par)
 }
 
 static struct xt_target trace_tg_reg __read_mostly = {
-	.name       = "TRACE",
-	.revision   = 0,
-	.family     = NFPROTO_UNSPEC,
-	.table      = "raw",
-	.target     = trace_tg,
-	.me         = THIS_MODULE,
+	.name		= "TRACE",
+	.revision	= 0,
+	.family		= NFPROTO_UNSPEC,
+	.table		= "raw",
+	.target		= trace_tg,
+	.checkentry	= trace_tg_check,
+	.destroy	= trace_tg_destroy,
+	.me		= THIS_MODULE,
 };
 
 static int __init trace_tg_init(void)
-- 
2.5.5

Re: [PATCH nf-next 3/4] netfilter: xt_TRACE: add explicitly nf_logger_find_get call

[Florian Westphal]

Liping Zhang <zlpnobody@163.com> wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Consider such situation, if nf_log_ipv4 kernel module is not installed,
> and the user add a following iptables rule:
>   # iptables -t raw -I PREROUTING -j TRACE
> 
> There will be no trace log generated until the user install nf_log_ipv4
> module manully.

Right, this is a constant source of confusion.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH nf-next 3/4] netfilter: xt_TRACE: add explicitly nf_logger_find_get call

[Pablo Neira Ayuso]

On Wed, Jun 08, 2016 at 08:43:19PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Consider such situation, if nf_log_ipv4 kernel module is not installed,
> and the user add a following iptables rule:
>   # iptables -t raw -I PREROUTING -j TRACE
> 
> There will be no trace log generated until the user install nf_log_ipv4
> module manully. So we should add request related nf_log module
> appropriately here.

Applied, thanks.

[PATCH nf-next 4/4] netfilter: nft_meta: add explicitly nf_logger_find_get call

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Like xt_TRACE do, we should request related nf_log module appropriately.
Otherwise, if the nf_log_ipv4 kernel module is not installed, and the
user add the following nft rule:
  # nft add rule filter input nftrace set 1

There will be no trace log generated until we install nf_log_ipv4 module
manually.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nft_meta.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index f4bad9d..abeb2ef 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -24,6 +24,7 @@
 #include <net/tcp_states.h> /* for TCP_TIME_WAIT */
 #include <net/netfilter/nf_tables.h>
 #include <net/netfilter/nf_tables_core.h>
+#include <net/netfilter/nf_log.h>
 #include <net/netfilter/nft_meta.h>
 
 #include <uapi/linux/netfilter_bridge.h> /* NF_BR_PRE_ROUTING */
@@ -348,8 +349,13 @@ int nft_meta_set_init(const struct nft_ctx *ctx,
 	if (err < 0)
 		return err;
 
-	if (priv->key == NFT_META_NFTRACE)
+	if (priv->key == NFT_META_NFTRACE) {
+		err = nf_logger_find_get(ctx->afi->family, NF_LOG_TYPE_LOG);
+		if (err < 0)
+			return err;
+
 		static_branch_inc(&nft_trace_enabled);
+	}
 
 	return 0;
 }
@@ -393,8 +399,10 @@ void nft_meta_set_destroy(const struct nft_ctx *ctx,
 {
 	const struct nft_meta *priv = nft_expr_priv(expr);
 
-	if (priv->key == NFT_META_NFTRACE)
+	if (priv->key == NFT_META_NFTRACE) {
 		static_branch_dec(&nft_trace_enabled);
+		nf_logger_put(ctx->afi->family, NF_LOG_TYPE_LOG);
+	}
 }
 EXPORT_SYMBOL_GPL(nft_meta_set_destroy);
 
-- 
2.5.5

Re: [PATCH nf-next 4/4] netfilter: nft_meta: add explicitly nf_logger_find_get call

[Florian Westphal]

Liping Zhang <zlpnobody@163.com> wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Like xt_TRACE do, we should request related nf_log module appropriately.
> Otherwise, if the nf_log_ipv4 kernel module is not installed, and the
> user add the following nft rule:
>   # nft add rule filter input nftrace set 1
> 
> There will be no trace log generated until we install nf_log_ipv4 module
> manually.

With nftables we have a new infrastructure in place that emits trace info via
nfnetlink.

So loading nf_log_ipX isn't needed anymore in nft.

Re: [PATCH nf-next 4/4] netfilter: nft_meta: add explicitly nf_logger_find_get call

[Liping Zhang]

Hi Florian,

At 2016-06-08 20:59:32, "Florian Westphal" <fw@strlen.de> wrote:
>
>With nftables we have a new infrastructure in place that emits trace info via
>nfnetlink.
>
>So loading nf_log_ipX isn't needed anymore in nft.

Yes, in nftables, user can use "nft monitor" to get the trace info.
But I think it is a little choas now, sometimes we can see trace info 
in kmsg(when nf_log_ipX is loaded), sometimes there's nothing in
kmsg(when nf_log_ipX is not installed).

This is confusing, especially for newbie.

Re: [PATCH nf-next 4/4] netfilter: nft_meta: add explicitly nf_logger_find_get call

[Pablo Neira Ayuso]

On Tue, Jun 14, 2016 at 08:35:29PM +0800, Liping Zhang wrote:
> Hi Florian,
> 
> At 2016-06-08 20:59:32, "Florian Westphal" <fw@strlen.de> wrote:
> >
> >With nftables we have a new infrastructure in place that emits trace info via
> >nfnetlink.
> >
> >So loading nf_log_ipX isn't needed anymore in nft.
> 
> Yes, in nftables, user can use "nft monitor" to get the trace info.
> But I think it is a little choas now, sometimes we can see trace info 
> in kmsg(when nf_log_ipX is loaded), sometimes there's nothing in
> kmsg(when nf_log_ipX is not installed).
> 
> This is confusing, especially for newbie.

Now that we got nft monitor, I think we need a way to deprecate the
old mode, I suggest a /proc interface (enabled by default) to disable
the ring buffer log mode. We can document this in the nftables HOWTO
on the wiki site.

I'm going to keep this back by now. We have the Netfilter Workshop
next week en Netherlands, I will be talking on the existing logging
infrastructure and this.

Will get back to you with feedback.