Re: [PATCH nf] netfilter: ipvs: flag ct as needing s/dnat in original direction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



	Hello,

On Sat, 24 Feb 2018, Florian Westphal wrote:

> FTP passive mode got broken by this change:
> - if (.. && nfct_nat(ct)) {
> + if (.. (ct->status & IPS_NAT_MASK)) {

	Looks like this check was needed to avoid crash in 2.6.35,
see commit 7bcbf81a2296a8 ("ipvs: avoid oops for passive FTP").

	I just tested such fix and it should be enough
to fix the passive FTP:

diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
index 3e17d32..58d5d05 100644
--- a/net/netfilter/ipvs/ip_vs_ftp.c
+++ b/net/netfilter/ipvs/ip_vs_ftp.c
@@ -260,7 +260,7 @@ 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 && (ct->status & IPS_NAT_MASK)) {
+		if (ct) {
 			bool mangled;
 
 			/* If mangling fails this function will return 0

	If it looks ok to you and if you prefer you can submit it
as patch, otherwise I'll do it in the following days...

> The PASV reply sent by real server need to be translated to contain
> the load balancers address, but they are passed unchanged.
> 
> IPS_NAT_MASK should be true for connections where reverse
> of reply tuple isn't the original tupe (i.e. connection had
> its source/destination changed), but ipvs uses a different function
> to replace the reply tuple address, and did not set SRC/DST NAT bit.
> 
> Reported-by: Li Shuang <shuali@xxxxxxxxxx>
> Fixes: 41390895e50bc4 ("netfilter: ipvs: don't check for presence of nat extension")
> Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
> ---
>  net/netfilter/ipvs/ip_vs_nfct.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_nfct.c b/net/netfilter/ipvs/ip_vs_nfct.c
> index 6cf3fd81a5ec..0ceac36819d9 100644
> --- a/net/netfilter/ipvs/ip_vs_nfct.c
> +++ b/net/netfilter/ipvs/ip_vs_nfct.c
> @@ -119,13 +119,17 @@ ip_vs_update_conntrack(struct sk_buff *skb, struct ip_vs_conn *cp, int outin)
>  	if (outin) {
>  		new_tuple.src.u3 = cp->daddr;
>  		if (new_tuple.dst.protonum != IPPROTO_ICMP &&
> -		    new_tuple.dst.protonum != IPPROTO_ICMPV6)
> +		    new_tuple.dst.protonum != IPPROTO_ICMPV6) {
>  			new_tuple.src.u.tcp.port = cp->dport;
> +			ct->status |= IPS_SRC_NAT;

	These bits should be reversed, 'outin' means 'from client
towards real server', i.e. like DNAT rule in original direction:

			ct->status |= IPS_DST_NAT;

> +		}
>  	} else {
>  		new_tuple.dst.u3 = cp->vaddr;
>  		if (new_tuple.dst.protonum != IPPROTO_ICMP &&
> -		    new_tuple.dst.protonum != IPPROTO_ICMPV6)
> +		    new_tuple.dst.protonum != IPPROTO_ICMPV6) {
>  			new_tuple.dst.u.tcp.port = cp->vport;
> +			ct->status |= IPS_DST_NAT;

	here too:
			ct->status |= IPS_SRC_NAT;

	But for now we do not need to set them. If your patch
works, it was because the bits helped for the IPS_NAT_MASK
check to allow the nf_nat_mangle_tcp_packet call.

Regards

--
Julian Anastasov <ja@xxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Netfitler Users]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux