diff mbox

[nf] netfilter: conntrack: warn the user if there is a better helper to use

Message ID 78dd7b4675383719e968db577fd89094729641e3.1432216169.git.mleitner@redhat.com
State Accepted
Delegated to: Pablo Neira
Headers show

Commit Message

Marcelo Leitner May 21, 2015, 1:57 p.m. UTC 
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

After db29a9508a92 ("netfilter: conntrack: disable generic tracking for
known protocols"), if the specific helper is built but not loaded
(a standard for most distributions) systems with a restrictive firewall
but weak configuration regarding netfilter modules to load, will
silently stop working.

This patch then puts a warning message so the sysadmin knows where to
start looking into. It's a pr_warn_once regardless of protocol itself
but it should be enough to give a hint on where to look.

Cc: Florian Westphal <fw@strlen.de>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 net/netfilter/nf_conntrack_proto_generic.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Marcelo Leitner June 9, 2015, 5:01 p.m. UTC | #1 
Ahm, ping? :)

On Thu, May 21, 2015 at 10:57:12AM -0300, Marcelo Ricardo Leitner wrote:
> From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 
> After db29a9508a92 ("netfilter: conntrack: disable generic tracking for
> known protocols"), if the specific helper is built but not loaded
> (a standard for most distributions) systems with a restrictive firewall
> but weak configuration regarding netfilter modules to load, will
> silently stop working.
> 
> This patch then puts a warning message so the sysadmin knows where to
> start looking into. It's a pr_warn_once regardless of protocol itself
> but it should be enough to give a hint on where to look.
> 
> Cc: Florian Westphal <fw@strlen.de>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
>  net/netfilter/nf_conntrack_proto_generic.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
> index 60865f1103099383c4263a1a56e691b3c86c3720..2281be419a74b6d8abe0fd7da8d7e8b35d304600 100644
> --- a/net/netfilter/nf_conntrack_proto_generic.c
> +++ b/net/netfilter/nf_conntrack_proto_generic.c
> @@ -90,7 +90,13 @@ static int generic_packet(struct nf_conn *ct,
>  static bool generic_new(struct nf_conn *ct, const struct sk_buff *skb,
>  			unsigned int dataoff, unsigned int *timeouts)
>  {
> -	return nf_generic_should_process(nf_ct_protonum(ct));
> +	bool ret;
> +
> +	ret = nf_generic_should_process(nf_ct_protonum(ct));
> +	if (!ret)
> +		pr_warn_once("conntrack: generic helper won't handle protocol %d. Please consider loading the specific helper module.\n",
> +			     nf_ct_protonum(ct));
> +	return ret;
>  }
>  
>  #if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
> -- 
> 2.4.1
> 
> --
> 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
> 
--
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
Pablo Neira Ayuso June 12, 2015, 12:24 p.m. UTC | #2 
On Thu, May 21, 2015 at 10:57:12AM -0300, Marcelo Ricardo Leitner wrote:
> From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 
> After db29a9508a92 ("netfilter: conntrack: disable generic tracking for
> known protocols"), if the specific helper is built but not loaded
> (a standard for most distributions) systems with a restrictive firewall
> but weak configuration regarding netfilter modules to load, will
> silently stop working.
> 
> This patch then puts a warning message so the sysadmin knows where to
> start looking into. It's a pr_warn_once regardless of protocol itself
> but it should be enough to give a hint on where to look.

Applied to nf-next.

I'd rather see some evaluation on getting these helpers into the
nf_conntrack module in terms of extra size, just as we do for tcp, udp
and icmp. Moreover, these trackers (specifically DCCP and SCTP) got
not much care so some extra review would be good if we decide to get
this into core.

I'm telling this because assuming that people will look at this warn
once still seem weak assumption to me.

Thanks for your patience.

> Cc: Florian Westphal <fw@strlen.de>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
>  net/netfilter/nf_conntrack_proto_generic.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
> index 60865f1103099383c4263a1a56e691b3c86c3720..2281be419a74b6d8abe0fd7da8d7e8b35d304600 100644
> --- a/net/netfilter/nf_conntrack_proto_generic.c
> +++ b/net/netfilter/nf_conntrack_proto_generic.c
> @@ -90,7 +90,13 @@ static int generic_packet(struct nf_conn *ct,
>  static bool generic_new(struct nf_conn *ct, const struct sk_buff *skb,
>  			unsigned int dataoff, unsigned int *timeouts)
>  {
> -	return nf_generic_should_process(nf_ct_protonum(ct));
> +	bool ret;
> +
> +	ret = nf_generic_should_process(nf_ct_protonum(ct));
> +	if (!ret)
> +		pr_warn_once("conntrack: generic helper won't handle protocol %d. Please consider loading the specific helper module.\n",
> +			     nf_ct_protonum(ct));
> +	return ret;
>  }
>  
>  #if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
> -- 
> 2.4.1
> 
> --
> 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
--
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
Marcelo Leitner June 12, 2015, 1:50 p.m. UTC | #3 
On Fri, Jun 12, 2015 at 02:24:14PM +0200, Pablo Neira Ayuso wrote:
> On Thu, May 21, 2015 at 10:57:12AM -0300, Marcelo Ricardo Leitner wrote:
> > From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > 
> > After db29a9508a92 ("netfilter: conntrack: disable generic tracking for
> > known protocols"), if the specific helper is built but not loaded
> > (a standard for most distributions) systems with a restrictive firewall
> > but weak configuration regarding netfilter modules to load, will
> > silently stop working.
> > 
> > This patch then puts a warning message so the sysadmin knows where to
> > start looking into. It's a pr_warn_once regardless of protocol itself
> > but it should be enough to give a hint on where to look.
> 
> Applied to nf-next.
> 
> I'd rather see some evaluation on getting these helpers into the
> nf_conntrack module in terms of extra size, just as we do for tcp, udp
> and icmp. Moreover, these trackers (specifically DCCP and SCTP) got
> not much care so some extra review would be good if we decide to get
> this into core.

That would be much better yes.

> I'm telling this because assuming that people will look at this warn
> once still seem weak assumption to me.
> 
> Thanks for your patience.

Ok, thanks Pablo.

  Marcelo

> > Cc: Florian Westphal <fw@strlen.de>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > ---
> >  net/netfilter/nf_conntrack_proto_generic.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
> > index 60865f1103099383c4263a1a56e691b3c86c3720..2281be419a74b6d8abe0fd7da8d7e8b35d304600 100644
> > --- a/net/netfilter/nf_conntrack_proto_generic.c
> > +++ b/net/netfilter/nf_conntrack_proto_generic.c
> > @@ -90,7 +90,13 @@ static int generic_packet(struct nf_conn *ct,
> >  static bool generic_new(struct nf_conn *ct, const struct sk_buff *skb,
> >  			unsigned int dataoff, unsigned int *timeouts)
> >  {
> > -	return nf_generic_should_process(nf_ct_protonum(ct));
> > +	bool ret;
> > +
> > +	ret = nf_generic_should_process(nf_ct_protonum(ct));
> > +	if (!ret)
> > +		pr_warn_once("conntrack: generic helper won't handle protocol %d. Please consider loading the specific helper module.\n",
> > +			     nf_ct_protonum(ct));
> > +	return ret;
> >  }
> >  
> >  #if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
> > -- 
> > 2.4.1
> > 
> > --
> > 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
> 
--
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
diff mbox

Patch

diff --git a/net/netfilter/nf_conntrack_proto_generic.c b/net/netfilter/nf_conntrack_proto_generic.c
index 60865f1103099383c4263a1a56e691b3c86c3720..2281be419a74b6d8abe0fd7da8d7e8b35d304600 100644
--- a/net/netfilter/nf_conntrack_proto_generic.c
+++ b/net/netfilter/nf_conntrack_proto_generic.c
@@ -90,7 +90,13 @@  static int generic_packet(struct nf_conn *ct,
 static bool generic_new(struct nf_conn *ct, const struct sk_buff *skb,
 			unsigned int dataoff, unsigned int *timeouts)
 {
-	return nf_generic_should_process(nf_ct_protonum(ct));
+	bool ret;
+
+	ret = nf_generic_should_process(nf_ct_protonum(ct));
+	if (!ret)
+		pr_warn_once("conntrack: generic helper won't handle protocol %d. Please consider loading the specific helper module.\n",
+			     nf_ct_protonum(ct));
+	return ret;
 }
 
 #if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)