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

[Julian Anastasov <ja@xxxxxx>]

        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 lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html