diff mbox

ipvs: avoid oops in nf_ct_seqadj_set when called from ip_vs_ftp helper

Message ID 20131213214023.14478.74738.stgit@dragon
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Jesper Dangaard Brouer Dec. 13, 2013, 9:37 p.m. UTC 
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(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Julian Anastasov Dec. 14, 2013, 10:25 p.m. UTC | #1 
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>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesper Dangaard Brouer Dec. 16, 2013, 4:09 p.m. UTC | #2 
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(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julian Anastasov Dec. 16, 2013, 9:11 p.m. UTC | #3 
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>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman Dec. 17, 2013, 1:36 a.m. UTC | #4 
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>
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman Dec. 27, 2013, 3:23 a.m. UTC | #5 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" 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/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,