From: Patrick McHardy on
Simon Horman wrote:
> @@ -219,19 +358,23 @@ static int ip_vs_ftp_out(struct ip_vs_ap
>
> buf_len = strlen(buf);
>
> + ct = nf_ct_get(skb, &ctinfo);
> + ret = nf_nat_mangle_tcp_packet(skb,
> + ct,
> + ctinfo,
> + start-data,
> + end-start,
> + buf,
> + buf_len);
> +
> + if (ct && ct != &nf_conntrack_untracked)
>
This does not make sense, you're already using the conntrack above
in the call to nf_nat_mangle_tcp_packet(), so the check should
probably happen before that. You also should be checking the
return value of nf_nat_mangle_tcp_packet() before setting up the
expectation.

> + ip_vs_expect_related(skb, ct, n_cp,
> + IPPROTO_TCP, NULL, 0);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Simon Horman on
On Tue, Jul 06, 2010 at 01:43:44PM +0200, Patrick McHardy wrote:
> Simon Horman wrote:
> >@@ -219,19 +358,23 @@ static int ip_vs_ftp_out(struct ip_vs_ap
> > buf_len = strlen(buf);
> >+ ct = nf_ct_get(skb, &ctinfo);
> >+ ret = nf_nat_mangle_tcp_packet(skb,
> >+ ct,
> >+ ctinfo,
> >+ start-data,
> >+ end-start,
> >+ buf,
> >+ buf_len);
> >+
> >+ if (ct && ct != &nf_conntrack_untracked)
> This does not make sense, you're already using the conntrack above
> in the call to nf_nat_mangle_tcp_packet(), so the check should
> probably happen before that. You also should be checking the
> return value of nf_nat_mangle_tcp_packet() before setting up the
> expectation.
>
> >+ ip_vs_expect_related(skb, ct, n_cp,
> >+ IPPROTO_TCP, NULL, 0);

Good point. Is this better?

ct = nf_ct_get(skb, &ctinfo);
if (ct && !nf_ct_is_untracked()) {
ret = nf_nat_mangle_tcp_packet(skb, ct, ctinfo,
start-data, end-start,
buf, buf_len);
if (ret)
ip_vs_expect_related(skb, ct, n_cp,
IPPROTO_TCP, NULL, 0);
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Patrick McHardy on
Am 07.07.2010 08:53, schrieb Simon Horman:
> On Tue, Jul 06, 2010 at 01:43:44PM +0200, Patrick McHardy wrote:
>> Simon Horman wrote:
>>> @@ -219,19 +358,23 @@ static int ip_vs_ftp_out(struct ip_vs_ap
>>> buf_len = strlen(buf);
>>> + ct = nf_ct_get(skb, &ctinfo);
>>> + ret = nf_nat_mangle_tcp_packet(skb,
>>> + ct,
>>> + ctinfo,
>>> + start-data,
>>> + end-start,
>>> + buf,
>>> + buf_len);
>>> +
>>> + if (ct && ct != &nf_conntrack_untracked)
>> This does not make sense, you're already using the conntrack above
>> in the call to nf_nat_mangle_tcp_packet(), so the check should
>> probably happen before that. You also should be checking the
>> return value of nf_nat_mangle_tcp_packet() before setting up the
>> expectation.
>>
>>> + ip_vs_expect_related(skb, ct, n_cp,
>>> + IPPROTO_TCP, NULL, 0);
>
> Good point. Is this better?
>
> ct = nf_ct_get(skb, &ctinfo);
> if (ct && !nf_ct_is_untracked()) {
> ret = nf_nat_mangle_tcp_packet(skb, ct, ctinfo,
> start-data, end-start,
> buf, buf_len);
> if (ret)
> ip_vs_expect_related(skb, ct, n_cp,
> IPPROTO_TCP, NULL, 0);

Yes, that's better, although we're usually dropping packets
when mangling fails. This can only happen under memory pressure,
the assumption is that we might be able to properly mangle
the packet when it is retransmitted.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Simon Horman on
On Fri, Jul 09, 2010 at 05:24:56PM +0200, Patrick McHardy wrote:
> Am 07.07.2010 08:53, schrieb Simon Horman:
> > On Tue, Jul 06, 2010 at 01:43:44PM +0200, Patrick McHardy wrote:
> >> Simon Horman wrote:
> >>> @@ -219,19 +358,23 @@ static int ip_vs_ftp_out(struct ip_vs_ap
> >>> buf_len = strlen(buf);
> >>> + ct = nf_ct_get(skb, &ctinfo);
> >>> + ret = nf_nat_mangle_tcp_packet(skb,
> >>> + ct,
> >>> + ctinfo,
> >>> + start-data,
> >>> + end-start,
> >>> + buf,
> >>> + buf_len);
> >>> +
> >>> + if (ct && ct != &nf_conntrack_untracked)
> >> This does not make sense, you're already using the conntrack above
> >> in the call to nf_nat_mangle_tcp_packet(), so the check should
> >> probably happen before that. You also should be checking the
> >> return value of nf_nat_mangle_tcp_packet() before setting up the
> >> expectation.
> >>
> >>> + ip_vs_expect_related(skb, ct, n_cp,
> >>> + IPPROTO_TCP, NULL, 0);
> >
> > Good point. Is this better?
> >
> > ct = nf_ct_get(skb, &ctinfo);
> > if (ct && !nf_ct_is_untracked()) {
> > ret = nf_nat_mangle_tcp_packet(skb, ct, ctinfo,
> > start-data, end-start,
> > buf, buf_len);
> > if (ret)
> > ip_vs_expect_related(skb, ct, n_cp,
> > IPPROTO_TCP, NULL, 0);
>
> Yes, that's better, although we're usually dropping packets
> when mangling fails. This can only happen under memory pressure,
> the assumption is that we might be able to properly mangle
> the packet when it is retransmitted.

I didn't notice this either, but ret will be end up being the return value
of ip_vs_ftp_out(), and if that is zero the packet will be dropped.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Simon Horman on
On Sat, Jul 10, 2010 at 12:01:00PM +0900, Simon Horman wrote:
> From: Hannes Eder <heder(a)google.com>
>
> Use nf_conntrack/nf_nat code to do the packet mangling and the TCP
> sequence adjusting. The function 'ip_vs_skb_replace' is now dead
> code, so it is removed.
>
> To SNAT FTP, use something like:
>
> % iptables -t nat -A POSTROUTING -m ipvs --vaddr 192.168.100.30/32 \
> > --vport 21 -j SNAT --to-source 192.168.10.10
>
> and for the data connections in passive mode:
>
> % iptables -t nat -A POSTROUTING -m ipvs --vaddr 192.168.100.30/32 \
> > --vportctl 21 -j SNAT --to-source 192.168.10.10
>
> using '-m state --state RELATED' would also works.
>
> Make sure the kernel modules ip_vs_ftp, nf_conntrack_ftp, and
> nf_nat_ftp are loaded.

[snip]

> Index: nf-next-2.6/net/netfilter/ipvs/ip_vs_ftp.c
> ===================================================================
> --- nf-next-2.6.orig/net/netfilter/ipvs/ip_vs_ftp.c 2010-07-10 11:48:54.000000000 +0900
> +++ nf-next-2.6/net/netfilter/ipvs/ip_vs_ftp.c 2010-07-10 11:59:19.000000000 +0900

[snip]

> @@ -43,6 +57,16 @@
> #define SERVER_STRING "227 Entering Passive Mode ("
> #define CLIENT_STRING "PORT "
>
> +#define FMT_TUPLE "%pI4:%u->%pI4:%u/%u"
> +#define ARG_TUPLE(T) (T)->src.u3.ip, ntohs((T)->src.u.all), \
> + (T)->dst.u3.ip, ntohs((T)->dst.u.all), \
> + (T)->dst.protonum
> +
> +#define FMT_CONN "%pI4:%u->%pI4:%u->%pI4:%u/%u:%u"
> +#define ARG_CONN(C) (C)->caddr, ntohs((C)->cport), \
> + (C)->vaddr, ntohs((C)->vport), \
> + (C)->daddr, ntohs((C)->dport), \
> + (C)->protocol, (C)->state
>
> /*
> * List of ports (up to IP_VS_APP_MAX_PORTS) to be handled by helper

The argument to the %pI4 needs to be a pointer
so (T)->src.u3.ip should be &(T)->src.u3.ip and
(C)->caddr should be &(C)->caddr.ip.

I'm not sure how this slipped through the cracks so far.
I will repost.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/