| Message ID | alpine.LFD.2.11.1312151709260.6463@ja.home.ssi.bg |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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> -- 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 --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