[PATCH] ipvs: avoid oops in nf_ct_seqadj_set when called from ip_vs_ftp helper

[PATCH] ipvs: avoid oops in nf_ct_seqadj_set when called from ip_vs_ftp helper

[Jesper Dangaard Brouer]

The IPVS FTP helper ip_vs_ftp can trigger an OOPS in nf_ct_seqadj_set,
after commit 41d73ec053d2 (netfilter: nf_conntrack: make sequence
number adjustments usuable without NAT).

We can avoid the oops in nf_ct_seqadj_set() by in ip_vs_ftp_out()
instead of calling nf_nat_mangle_tcp_packet() we simply call
__nf_nat_mangle_tcp_packet() with a "false" last parameter, which
indicate not invoking the seqadj code.

After this fix, I've tested that FTP over IPVS, with module ip_vs_ftp
loaded, works for both passive and active FTP.

Fixes: 41d73ec053d2 (netfilter: nf_conntrack: make sequence number adjustments usuable without NAT)
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

---
I'm uncertain if this is the correct fix.  Perhaps the ip_vs_ftp
helper need to allocate/init the seqadj extension instead?

 net/netfilter/ipvs/ip_vs_ftp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
index 77c1732..b8f9573 100644
--- a/net/netfilter/ipvs/ip_vs_ftp.c
+++ b/net/netfilter/ipvs/ip_vs_ftp.c
@@ -268,10 +268,10 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp,
 			 * packet.
 			 */
 			rcu_read_lock();
-			ret = nf_nat_mangle_tcp_packet(skb, ct, ctinfo,
+			ret = __nf_nat_mangle_tcp_packet(skb, ct, ctinfo,
 						       iph->ihl * 4,
 						       start-data, end-start,
-						       buf, buf_len);
+						       buf, buf_len, false);
 			rcu_read_unlock();
 			if (ret) {
 				ip_vs_nfct_expect_related(skb, ct, n_cp,

Re: [PATCH] ipvs: avoid oops in nf_ct_seqadj_set when called from ip_vs_ftp helper

[Julian Anastasov]

	Hello,

On Fri, 13 Dec 2013, Jesper Dangaard Brouer wrote:

> The IPVS FTP helper ip_vs_ftp can trigger an OOPS in nf_ct_seqadj_set,
> after commit 41d73ec053d2 (netfilter: nf_conntrack: make sequence
> number adjustments usuable without NAT).
> 
> We can avoid the oops in nf_ct_seqadj_set() by in ip_vs_ftp_out()
> instead of calling nf_nat_mangle_tcp_packet() we simply call
> __nf_nat_mangle_tcp_packet() with a "false" last parameter, which
> indicate not invoking the seqadj code.
> 
> After this fix, I've tested that FTP over IPVS, with module ip_vs_ftp
> loaded, works for both passive and active FTP.

	There is still a problem when RIP and VIP differ in
text length because now ack_seq sent to RIP is not
adjusted after receiving the the 227 Entering Passive Mode
reply. I tried also to set *diff, ack_seq was corrected
but the result was bad checksum.

> Fixes: 41d73ec053d2 (netfilter: nf_conntrack: make sequence number adjustments usuable without NAT)
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> ---
> I'm uncertain if this is the correct fix.  Perhaps the ip_vs_ftp
> helper need to allocate/init the seqadj extension instead?

	Agreed, lets try nfct_seqadj_ext_add() + nf_ct_seqadj_init()
if nfct_seqadj(ct) is NULL.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: avoid oops in nf_ct_seqadj_set when called from ip_vs_ftp helper

[Julian Anastasov]

	Hello,

On Fri, 13 Dec 2013, Jesper Dangaard Brouer wrote:

> The IPVS FTP helper ip_vs_ftp can trigger an OOPS in nf_ct_seqadj_set,
> after commit 41d73ec053d2 (netfilter: nf_conntrack: make sequence
> number adjustments usuable without NAT).
> 
> We can avoid the oops in nf_ct_seqadj_set() by in ip_vs_ftp_out()
> instead of calling nf_nat_mangle_tcp_packet() we simply call
> __nf_nat_mangle_tcp_packet() with a "false" last parameter, which
> indicate not invoking the seqadj code.
> 
> After this fix, I've tested that FTP over IPVS, with module ip_vs_ftp
> loaded, works for both passive and active FTP.
> 
> Fixes: 41d73ec053d2 (netfilter: nf_conntrack: make sequence number adjustments usuable without NAT)
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> ---
> I'm uncertain if this is the correct fix.  Perhaps the ip_vs_ftp
> helper need to allocate/init the seqadj extension instead?

	I hope I'll save you some time... What do you
think about such change:

[PATCH net] ipvs: use the new seqadj interface from ip_vs_ftp

diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
index 77c1732..9c2074d 100644
--- a/net/netfilter/ipvs/ip_vs_ftp.c
+++ b/net/netfilter/ipvs/ip_vs_ftp.c
@@ -34,6 +34,7 @@
 #include <linux/netfilter.h>
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_expect.h>
+#include <net/netfilter/nf_conntrack_seqadj.h>
 #include <net/netfilter/nf_nat.h>
 #include <net/netfilter/nf_nat_helper.h>
 #include <linux/gfp.h>
@@ -260,7 +261,8 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp,
 		buf_len = strlen(buf);
 
 		ct = nf_ct_get(skb, &ctinfo);
-		if (ct && !nf_ct_is_untracked(ct) && nfct_nat(ct)) {
+		if (ct && !nf_ct_is_untracked(ct) && nfct_nat(ct) &&
+		    nfct_seqadj(ct)) {
 			/* If mangling fails this function will return 0
 			 * which will cause the packet to be dropped.
 			 * Mangling can only fail under memory pressure,
diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c
index c8beafd..5a355a4 100644
--- a/net/netfilter/ipvs/ip_vs_nfct.c
+++ b/net/netfilter/ipvs/ip_vs_nfct.c
@@ -63,6 +63,7 @@
 #include <net/ip_vs.h>
 #include <net/netfilter/nf_conntrack_core.h>
 #include <net/netfilter/nf_conntrack_expect.h>
+#include <net/netfilter/nf_conntrack_seqadj.h>
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_zones.h>
 
@@ -97,6 +98,11 @@ ip_vs_update_conntrack(struct sk_buff *skb, struct ip_vs_conn *cp, int outin)
 	if (CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL)
 		return;
 
+	/* Applications may adjust TCP seqs */
+	if (cp->app && nf_ct_protonum(ct) == IPPROTO_TCP &&
+	    !nfct_seqadj(ct) && !nfct_seqadj_ext_add(ct))
+		return;
+
 	/*
 	 * The connection is not yet in the hashtable, so we update it.
 	 * CIP->VIP will remain the same, so leave the tuple in
-- 
1.8.4.2

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH] ipvs: avoid oops in nf_ct_seqadj_set when called from ip_vs_ftp helper

[Jesper Dangaard Brouer]

On Sun, 15 Dec 2013 18:10:42 +0200 (EET)
Julian Anastasov <ja@ssi.bg> wrote:

> On Fri, 13 Dec 2013, Jesper Dangaard Brouer wrote:
> 
> > The IPVS FTP helper ip_vs_ftp can trigger an OOPS in nf_ct_seqadj_set,
> > after commit 41d73ec053d2 (netfilter: nf_conntrack: make sequence
> > number adjustments usuable without NAT).
> > 
> > We can avoid the oops in nf_ct_seqadj_set() by in ip_vs_ftp_out()
> > instead of calling nf_nat_mangle_tcp_packet() we simply call
> > __nf_nat_mangle_tcp_packet() with a "false" last parameter, which
> > indicate not invoking the seqadj code.
> > 
> > After this fix, I've tested that FTP over IPVS, with module ip_vs_ftp
> > loaded, works for both passive and active FTP.
> > 
> > Fixes: 41d73ec053d2 (netfilter: nf_conntrack: make sequence number adjustments usuable without NAT)
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > 
> > ---
> > I'm uncertain if this is the correct fix.  Perhaps the ip_vs_ftp
> > helper need to allocate/init the seqadj extension instead?
> 
> 	I hope I'll save you some time... What do you
> think about such change:

Thanks a lot!


> [PATCH net] ipvs: use the new seqadj interface from ip_vs_ftp
> 
> diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
> index 77c1732..9c2074d 100644
> --- a/net/netfilter/ipvs/ip_vs_ftp.c
> +++ b/net/netfilter/ipvs/ip_vs_ftp.c
> @@ -34,6 +34,7 @@
>  #include <linux/netfilter.h>
>  #include <net/netfilter/nf_conntrack.h>
>  #include <net/netfilter/nf_conntrack_expect.h>
> +#include <net/netfilter/nf_conntrack_seqadj.h>
>  #include <net/netfilter/nf_nat.h>
>  #include <net/netfilter/nf_nat_helper.h>
>  #include <linux/gfp.h>
> @@ -260,7 +261,8 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp,
>  		buf_len = strlen(buf);
>  
>  		ct = nf_ct_get(skb, &ctinfo);
> -		if (ct && !nf_ct_is_untracked(ct) && nfct_nat(ct)) {
> +		if (ct && !nf_ct_is_untracked(ct) && nfct_nat(ct) &&
> +		    nfct_seqadj(ct)) {

If we add the other section, then this check should not be necessary.


>  			/* If mangling fails this function will return 0
>  			 * which will cause the packet to be dropped.
>  			 * Mangling can only fail under memory pressure,
> diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c
> index c8beafd..5a355a4 100644
> --- a/net/netfilter/ipvs/ip_vs_nfct.c
> +++ b/net/netfilter/ipvs/ip_vs_nfct.c
> @@ -63,6 +63,7 @@
>  #include <net/ip_vs.h>
>  #include <net/netfilter/nf_conntrack_core.h>
>  #include <net/netfilter/nf_conntrack_expect.h>
> +#include <net/netfilter/nf_conntrack_seqadj.h>
>  #include <net/netfilter/nf_conntrack_helper.h>
>  #include <net/netfilter/nf_conntrack_zones.h>
>  
> @@ -97,6 +98,11 @@ ip_vs_update_conntrack(struct sk_buff *skb, struct ip_vs_conn *cp, int outin)
>  	if (CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL)
>  		return;
>  
> +	/* Applications may adjust TCP seqs */
> +	if (cp->app && nf_ct_protonum(ct) == IPPROTO_TCP &&
> +	    !nfct_seqadj(ct) && !nfct_seqadj_ext_add(ct))
> +		return;
> +

It will work.

I'm just thinking if we will allocate a seqadj ext, in too many
situations, were its not really needed... double checking, I see that
"cp->app" will limit us a lot, as it seems that FTP is the only one
using register_ip_vs_app(),

And above this, we do check:
	if (IP_VS_FWD_METHOD(cp) != IP_VS_CONN_F_MASQ)
		return;
So, we only do this for IPVS masq case, good.
I think we are good.

Now, I'm just wondering if SIP will work... (but I don't have a lab to
test this).

I'll submit a new patch/fix soon.
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH] ipvs: avoid oops in nf_ct_seqadj_set when called from ip_vs_ftp helper

[Julian Anastasov]

	Hello,

On Mon, 16 Dec 2013, Jesper Dangaard Brouer wrote:

> On Sun, 15 Dec 2013 18:10:42 +0200 (EET)
> Julian Anastasov <ja@ssi.bg> wrote:

> > @@ -260,7 +261,8 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp,
> >  		buf_len = strlen(buf);
> >  
> >  		ct = nf_ct_get(skb, &ctinfo);
> > -		if (ct && !nf_ct_is_untracked(ct) && nfct_nat(ct)) {
> > +		if (ct && !nf_ct_is_untracked(ct) && nfct_nat(ct) &&
> > +		    nfct_seqadj(ct)) {
> 
> If we add the other section, then this check should not be necessary.

	In the common case - yes. nfct_seqadj_ext_add needs
to be called before confirmation, that is why we place it
in ip_vs_update_conntrack(). There is another case when
synced connection comes to backup in established state.
I'm not sure what happens in netfilter in this case, we
do not provide any initial seq and offsets to be used by
the seqadj code. We have to think more about this case.

	So, we need nfct_seqadj check in ip_vs_ftp_out or in
nf_ct_seqadj_set as in your patch 1/2 to avoid oops for
synced connections.

> > +	/* Applications may adjust TCP seqs */
> > +	if (cp->app && nf_ct_protonum(ct) == IPPROTO_TCP &&
> > +	    !nfct_seqadj(ct) && !nfct_seqadj_ext_add(ct))
> > +		return;
> > +
> 
> It will work.
> 
> I'm just thinking if we will allocate a seqadj ext, in too many
> situations, were its not really needed... double checking, I see that
> "cp->app" will limit us a lot, as it seems that FTP is the only one
> using register_ip_vs_app(),

	Yes. And we can not do it just before nf_nat_mangle_tcp_packet,
it should happen before conntrack is confirmed, on SYN.

> And above this, we do check:
> 	if (IP_VS_FWD_METHOD(cp) != IP_VS_CONN_F_MASQ)
> 		return;
> So, we only do this for IPVS masq case, good.
> I think we are good.
> 
> Now, I'm just wondering if SIP will work... (but I don't have a lab to
> test this).

	It is only for UDP, for now...

Regards

--
Julian Anastasov <ja@ssi.bg>

[net PATCH 0/2] Fixing OOPSes in seqadj code

[Jesper Dangaard Brouer]

The following series address issues with netfilter conntrack sequence
number adjustments, introduced with commit 41d73ec053d2 (netfilter:
nf_conntrack: make sequence number adjustments usuable without NAT).

 Patch1: give us a WARN splash when using seqadj incorrectly

 Patch2: fixes a wrong usage of seqadj in IPVS code

I'm not sure, which maintainer will take these fixes?!?

---

Jesper Dangaard Brouer (2):
      ipvs: correct usage/allocation of seqadj ext in ipvs
      netfilter: WARN about wrong usage of sequence number adjustments


 net/netfilter/ipvs/ip_vs_nfct.c     |    6 ++++++
 net/netfilter/nf_conntrack_seqadj.c |    5 +++++
 2 files changed, 11 insertions(+), 0 deletions(-)

[net PATCH 1/2] netfilter: WARN about wrong usage of sequence number adjustments

[Jesper Dangaard Brouer]

Since commit 41d73ec053d2 (netfilter: nf_conntrack: make sequence
number adjustments usuable without NAT), the sequence number extension
is dynamically allocated.

Instead of dying, give a WARN splash, in case of wrong usage of the
seqadj code, e.g. when forgetting to allocate via nfct_seqadj_ext_add().

Wrong usage have been seen in the IPVS code path.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---

 net/netfilter/nf_conntrack_seqadj.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/nf_conntrack_seqadj.c b/net/netfilter/nf_conntrack_seqadj.c
index 17c1bcb..b2d38da 100644
--- a/net/netfilter/nf_conntrack_seqadj.c
+++ b/net/netfilter/nf_conntrack_seqadj.c
@@ -36,6 +36,11 @@ int nf_ct_seqadj_set(struct nf_conn *ct, enum ip_conntrack_info ctinfo,
 	if (off == 0)
 		return 0;
 
+	if (unlikely(!seqadj)) {
+		WARN(1, "Wrong seqadj usage, missing nfct_seqadj_ext_add()\n");
+		return 0;
+	}
+
 	set_bit(IPS_SEQ_ADJUST_BIT, &ct->status);
 
 	spin_lock_bh(&ct->lock);

[net PATCH 2/2] ipvs: correct usage/allocation of seqadj ext in ipvs

[Jesper Dangaard Brouer]

The IPVS FTP helper ip_vs_ftp could trigger an OOPS in nf_ct_seqadj_set,
after commit 41d73ec053d2 (netfilter: nf_conntrack: make sequence number
adjustments usuable without NAT).

This is because, the seqadj ext is now allocated dynamically, and the
IPVS code didn't handle this situation.  Fix this in the IPVS nfct
code by invoking the alloc function nfct_seqadj_ext_add().

Fixes: 41d73ec053d2 (netfilter: nf_conntrack: make sequence number adjustments usuable without NAT)
Suggested-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---

 net/netfilter/ipvs/ip_vs_nfct.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c
index d5f4151..5882bbf 100644
--- a/net/netfilter/ipvs/ip_vs_nfct.c
+++ b/net/netfilter/ipvs/ip_vs_nfct.c
@@ -62,6 +62,7 @@
 #include <net/ip_vs.h>
 #include <net/netfilter/nf_conntrack_core.h>
 #include <net/netfilter/nf_conntrack_expect.h>
+#include <net/netfilter/nf_conntrack_seqadj.h>
 #include <net/netfilter/nf_conntrack_helper.h>
 #include <net/netfilter/nf_conntrack_zones.h>
 
@@ -96,6 +97,11 @@ ip_vs_update_conntrack(struct sk_buff *skb, struct ip_vs_conn *cp, int outin)
 	if (CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL)
 		return;
 
+	/* Applications may adjust TCP seqs */
+	if (cp->app && nf_ct_protonum(ct) == IPPROTO_TCP &&
+	    !nfct_seqadj(ct) && !nfct_seqadj_ext_add(ct))
+		return;
+
 	/*
 	 * The connection is not yet in the hashtable, so we update it.
 	 * CIP->VIP will remain the same, so leave the tuple in

Re: [net PATCH 0/2] Fixing OOPSes in seqadj code

[Julian Anastasov]

	Hello,

On Mon, 16 Dec 2013, Jesper Dangaard Brouer wrote:

> The following series address issues with netfilter conntrack sequence
> number adjustments, introduced with commit 41d73ec053d2 (netfilter:
> nf_conntrack: make sequence number adjustments usuable without NAT).
> 
>  Patch1: give us a WARN splash when using seqadj incorrectly
> 
>  Patch2: fixes a wrong usage of seqadj in IPVS code

	Both patches look good to me,

Acked-by: Julian Anastasov <ja@ssi.bg>

> I'm not sure, which maintainer will take these fixes?!?

	I think, Simon can take them, if there are no
objections...

> Jesper Dangaard Brouer (2):
>       ipvs: correct usage/allocation of seqadj ext in ipvs
>       netfilter: WARN about wrong usage of sequence number adjustments
> 
> 
>  net/netfilter/ipvs/ip_vs_nfct.c     |    6 ++++++
>  net/netfilter/nf_conntrack_seqadj.c |    5 +++++
>  2 files changed, 11 insertions(+), 0 deletions(-)

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [net PATCH 0/2] Fixing OOPSes in seqadj code

[Simon Horman]

On Mon, Dec 16, 2013 at 11:11:51PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 16 Dec 2013, Jesper Dangaard Brouer wrote:
> 
> > The following series address issues with netfilter conntrack sequence
> > number adjustments, introduced with commit 41d73ec053d2 (netfilter:
> > nf_conntrack: make sequence number adjustments usuable without NAT).
> > 
> >  Patch1: give us a WARN splash when using seqadj incorrectly
> > 
> >  Patch2: fixes a wrong usage of seqadj in IPVS code
> 
> 	Both patches look good to me,
> 
> Acked-by: Julian Anastasov <ja@ssi.bg>
> 
> > I'm not sure, which maintainer will take these fixes?!?
> 
> 	I think, Simon can take them, if there are no
> objections...

I have no objections.
I'll wait a day to see if anyone else does.

> 
> > Jesper Dangaard Brouer (2):
> >       ipvs: correct usage/allocation of seqadj ext in ipvs
> >       netfilter: WARN about wrong usage of sequence number adjustments
> > 
> > 
> >  net/netfilter/ipvs/ip_vs_nfct.c     |    6 ++++++
> >  net/netfilter/nf_conntrack_seqadj.c |    5 +++++
> >  2 files changed, 11 insertions(+), 0 deletions(-)
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>
> 

Re: [net PATCH 0/2] Fixing OOPSes in seqadj code

[Simon Horman]

On Tue, Dec 17, 2013 at 10:36:37AM +0900, Simon Horman wrote:
> On Mon, Dec 16, 2013 at 11:11:51PM +0200, Julian Anastasov wrote:
> > 
> > 	Hello,
> > 
> > On Mon, 16 Dec 2013, Jesper Dangaard Brouer wrote:
> > 
> > > The following series address issues with netfilter conntrack sequence
> > > number adjustments, introduced with commit 41d73ec053d2 (netfilter:
> > > nf_conntrack: make sequence number adjustments usuable without NAT).
> > > 
> > >  Patch1: give us a WARN splash when using seqadj incorrectly
> > > 
> > >  Patch2: fixes a wrong usage of seqadj in IPVS code
> > 
> > 	Both patches look good to me,
> > 
> > Acked-by: Julian Anastasov <ja@ssi.bg>
> > 
> > > I'm not sure, which maintainer will take these fixes?!?
> > 
> > 	I think, Simon can take them, if there are no
> > objections...
> 
> I have no objections.
> I'll wait a day to see if anyone else does.

It was much more than a day but I have taken these changes now.