[nf-next] netfilter: nf_defrag_ipv4: Skip defrag if NOTRACK is set

Message ID 1510623446-9115-1-git-send-email-subashab@codeaurora.org
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • [nf-next] netfilter: nf_defrag_ipv4: Skip defrag if NOTRACK is set
Related show

Commit Message

Subash Abhinov Kasiviswanathan Nov. 14, 2017, 1:37 a.m.
conntrack defrag is needed only if some module like CONNTRACK or NAT
explicitly requests it. For plain forwarding scenarios, defrag is
not needed and can be skipped if NOTRACK is set in a rule.

Since conntrack defrag is currently higher priority than raw table,
setting NOTRACK is not sufficient. We need to introduce a lower
priority rule and deprecate the existing rule.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
---
 include/uapi/linux/netfilter_ipv4.h | 3 ++-
 net/ipv4/netfilter/nf_defrag_ipv4.c | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Pablo Neira Ayuso Dec. 9, 2017, 3:10 p.m. | #1
Hi Subash,

Sorry it took a while, but I've been discussing this with Jozsef too,
he's now on Cc.

On Mon, Nov 13, 2017 at 06:37:26PM -0700, Subash Abhinov Kasiviswanathan wrote:
> conntrack defrag is needed only if some module like CONNTRACK or NAT
> explicitly requests it. For plain forwarding scenarios, defrag is
> not needed and can be skipped if NOTRACK is set in a rule.
> 
> Since conntrack defrag is currently higher priority than raw table,
> setting NOTRACK is not sufficient. We need to introduce a lower
> priority rule and deprecate the existing rule.
> 
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> ---
>  include/uapi/linux/netfilter_ipv4.h | 3 ++-
>  net/ipv4/netfilter/nf_defrag_ipv4.c | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter_ipv4.h b/include/uapi/linux/netfilter_ipv4.h
> index e6b1a84..cedf7b4 100644
> --- a/include/uapi/linux/netfilter_ipv4.h
> +++ b/include/uapi/linux/netfilter_ipv4.h
> @@ -57,9 +57,10 @@
>  
>  enum nf_ip_hook_priorities {
>  	NF_IP_PRI_FIRST = INT_MIN,
> -	NF_IP_PRI_CONNTRACK_DEFRAG = -400,
> +	NF_IP_PRI_CONNTRACK_DEFRAG = -400, /* deprecated */
>  	NF_IP_PRI_RAW = -300,
>  	NF_IP_PRI_SELINUX_FIRST = -225,
> +	NF_IP_PRI_CONNTRACK_DEFRAG_V1 = -210,

Would it work for you if this is specific via global modparam? I'm
telling this because:

1) This is changing the default behaviour, which is always tricky.
2) This is already solved in nftables, so whatever solution that we
   apply, it should be iptables specific.

If modparam is fine, just placing a line into
/etc/modprobe.d/options.conf (or similar) should be good enough to
store that you're requesting raw hook registration before defrag.

Let me know,
Thanks!
--
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
Subash Abhinov Kasiviswanathan Dec. 10, 2017, 2:06 a.m. | #2
> Would it work for you if this is specific via global modparam? I'm
> telling this because:
> 
> 1) This is changing the default behaviour, which is always tricky.
> 2) This is already solved in nftables, so whatever solution that we
>    apply, it should be iptables specific.
> 
> If modparam is fine, just placing a line into
> /etc/modprobe.d/options.conf (or similar) should be good enough to
> store that you're requesting raw hook registration before defrag.
> 
> Let me know,
> Thanks!

Hi Pablo

Can you explain a bit more about the /etc/modprobe.d/ option and how
it would be configured for this? /etc/modprobe.d/ doesnt exist on
Android based Linux systems so it might be a problem for me.

Would it be an acceptable solution to create a kernel config for this
particular feature instead?
Pablo Neira Ayuso Dec. 10, 2017, 2:53 p.m. | #3
On Sat, Dec 09, 2017 at 07:06:14PM -0700, Subash Abhinov Kasiviswanathan wrote:
> >Would it work for you if this is specific via global modparam? I'm
> >telling this because:
> >
> >1) This is changing the default behaviour, which is always tricky.
> >2) This is already solved in nftables, so whatever solution that we
> >   apply, it should be iptables specific.
> >
> >If modparam is fine, just placing a line into
> >/etc/modprobe.d/options.conf (or similar) should be good enough to
> >store that you're requesting raw hook registration before defrag.
> >
> >Let me know,
> >Thanks!
> 
> Hi Pablo
> 
> Can you explain a bit more about the /etc/modprobe.d/ option and how
> it would be configured for this? /etc/modprobe.d/ doesnt exist on
> Android based Linux systems so it might be a problem for me.
> 
> Would it be an acceptable solution to create a kernel config for this
> particular feature instead?

I'm actually refering to module_param(), that is specified at modprobe
time. Such parameter would set an alternative hook priority for the
raw table, ie. before the defrag hook. I guess there must be a way to
store these module parameters in Android, so whenever modprobe is
invoked, either explicitly or via module autoload, this module
parameter is passed to the iptable_raw module.
--
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
Subash Abhinov Kasiviswanathan Dec. 11, 2017, 7:13 a.m. | #4
> I'm actually refering to module_param(), that is specified at modprobe
> time. Such parameter would set an alternative hook priority for the
> raw table, ie. before the defrag hook. I guess there must be a way to
> store these module parameters in Android, so whenever modprobe is
> invoked, either explicitly or via module autoload, this module
> parameter is passed to the iptable_raw module.

Thanks for the clarification Pablo. I'll try this and send a new patch.

Patch

diff --git a/include/uapi/linux/netfilter_ipv4.h b/include/uapi/linux/netfilter_ipv4.h
index e6b1a84..cedf7b4 100644
--- a/include/uapi/linux/netfilter_ipv4.h
+++ b/include/uapi/linux/netfilter_ipv4.h
@@ -57,9 +57,10 @@ 
 
 enum nf_ip_hook_priorities {
 	NF_IP_PRI_FIRST = INT_MIN,
-	NF_IP_PRI_CONNTRACK_DEFRAG = -400,
+	NF_IP_PRI_CONNTRACK_DEFRAG = -400, /* deprecated */
 	NF_IP_PRI_RAW = -300,
 	NF_IP_PRI_SELINUX_FIRST = -225,
+	NF_IP_PRI_CONNTRACK_DEFRAG_V1 = -210,
 	NF_IP_PRI_CONNTRACK = -200,
 	NF_IP_PRI_MANGLE = -150,
 	NF_IP_PRI_NAT_DST = -100,
diff --git a/net/ipv4/netfilter/nf_defrag_ipv4.c b/net/ipv4/netfilter/nf_defrag_ipv4.c
index 37fe1616..6496890 100644
--- a/net/ipv4/netfilter/nf_defrag_ipv4.c
+++ b/net/ipv4/netfilter/nf_defrag_ipv4.c
@@ -80,7 +80,7 @@  static unsigned int ipv4_conntrack_defrag(void *priv,
 #endif
 #endif
 	/* Gather fragments. */
-	if (ip_is_fragment(ip_hdr(skb))) {
+	if (skb->_nfct != IP_CT_UNTRACKED && ip_is_fragment(ip_hdr(skb))) {
 		enum ip_defrag_users user =
 			nf_ct_defrag_user(state->hook, skb);
 
@@ -95,13 +95,13 @@  static unsigned int ipv4_conntrack_defrag(void *priv,
 		.hook		= ipv4_conntrack_defrag,
 		.pf		= NFPROTO_IPV4,
 		.hooknum	= NF_INET_PRE_ROUTING,
-		.priority	= NF_IP_PRI_CONNTRACK_DEFRAG,
+		.priority	= NF_IP_PRI_CONNTRACK_DEFRAG_V1,
 	},
 	{
 		.hook           = ipv4_conntrack_defrag,
 		.pf             = NFPROTO_IPV4,
 		.hooknum        = NF_INET_LOCAL_OUT,
-		.priority       = NF_IP_PRI_CONNTRACK_DEFRAG,
+		.priority       = NF_IP_PRI_CONNTRACK_DEFRAG_V1,
 	},
 };