[PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Alex Gartrell]

It is possible that we bind against a local socket in early_demux when we
are actually going to want to forward it.  In this case, the socket serves
no purpose and only serves to confuse things (particularly functions which
implicitly expect sk_fullsock to be true, like ip_local_out).
Additionally, skb_set_owner_w is totally broken for non full-socks.

Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
 net/netfilter/ipvs/ip_vs_xmit.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index bf66a86..99d4a41 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -527,6 +527,21 @@ static inline int ip_vs_tunnel_xmit_prepare(struct sk_buff *skb,
 	return ret;
 }
 
+/* In the event of a remote destination, it's possible that we would have
+ * matches against an old socket (particularly a TIME-WAIT socket). This
+ * causes havoc down the line (ip_local_out et. al. expect regular sockets
+ * and invalid memory accesses will happen) so simply drop the association
+ * in this case.
+*/
+static inline void ip_vs_drop_early_demux_sk(struct sk_buff *skb)
+{
+	/* If dev is set, the packet came from the LOCAL_IN callback and
+	 * not from a local TCP socket.
+	 */
+	if (skb->dev)
+		skb_orphan(skb);
+}
+
 /* return NF_STOLEN (sent) or NF_ACCEPT if local=1 (not sent) */
 static inline int ip_vs_nat_send_or_cont(int pf, struct sk_buff *skb,
 					 struct ip_vs_conn *cp, int local)
@@ -538,12 +553,21 @@ static inline int ip_vs_nat_send_or_cont(int pf, struct sk_buff *skb,
 		ip_vs_notrack(skb);
 	else
 		ip_vs_update_conntrack(skb, cp, 1);
+
+	/* Remove the early_demux association unless it's bound for the
+	 * exact same port and address on this host after translation.
+	 */
+	if (!local || cp->vport != cp->dport ||
+	    !ip_vs_addr_equal(cp->af, &cp->vaddr, &cp->daddr))
+		ip_vs_drop_early_demux_sk(skb);
+
 	if (!local) {
 		skb_forward_csum(skb);
 		NF_HOOK(pf, NF_INET_LOCAL_OUT, NULL, skb,
 			NULL, skb_dst(skb)->dev, dst_output_sk);
 	} else
 		ret = NF_ACCEPT;
+
 	return ret;
 }
 
@@ -557,6 +581,7 @@ static inline int ip_vs_send_or_cont(int pf, struct sk_buff *skb,
 	if (likely(!(cp->flags & IP_VS_CONN_F_NFCT)))
 		ip_vs_notrack(skb);
 	if (!local) {
+		ip_vs_drop_early_demux_sk(skb);
 		skb_forward_csum(skb);
 		NF_HOOK(pf, NF_INET_LOCAL_OUT, NULL, skb,
 			NULL, skb_dst(skb)->dev, dst_output_sk);
@@ -845,6 +870,8 @@ ip_vs_prepare_tunneled_skb(struct sk_buff *skb, int skb_af,
 	struct ipv6hdr *old_ipv6h = NULL;
 #endif
 
+	ip_vs_drop_early_demux_sk(skb);
+
 	if (skb_headroom(skb) < max_headroom || skb_cloned(skb)) {
 		new_skb = skb_realloc_headroom(skb, max_headroom);
 		if (!new_skb)
-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Julian Anastasov]

	Hello,

On Sun, 5 Jul 2015, Alex Gartrell wrote:

> It is possible that we bind against a local socket in early_demux when we
> are actually going to want to forward it.  In this case, the socket serves
> no purpose and only serves to confuse things (particularly functions which
> implicitly expect sk_fullsock to be true, like ip_local_out).
> Additionally, skb_set_owner_w is totally broken for non full-socks.
> 
> Signed-off-by: Alex Gartrell <agartrell@fb.com>

	Thanks for fixing this problem!

Acked-by: Julian Anastasov <ja@ssi.bg>

	May be the patch fixes crashes? If yes, Simon
should apply it for ipvs/net tree, otherwise after
the merge window...

> ---
>  net/netfilter/ipvs/ip_vs_xmit.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> index bf66a86..99d4a41 100644
> --- a/net/netfilter/ipvs/ip_vs_xmit.c
> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> @@ -527,6 +527,21 @@ static inline int ip_vs_tunnel_xmit_prepare(struct sk_buff *skb,
>  	return ret;
>  }
>  
> +/* In the event of a remote destination, it's possible that we would have
> + * matches against an old socket (particularly a TIME-WAIT socket). This
> + * causes havoc down the line (ip_local_out et. al. expect regular sockets
> + * and invalid memory accesses will happen) so simply drop the association
> + * in this case.
> +*/
> +static inline void ip_vs_drop_early_demux_sk(struct sk_buff *skb)
> +{
> +	/* If dev is set, the packet came from the LOCAL_IN callback and
> +	 * not from a local TCP socket.
> +	 */
> +	if (skb->dev)
> +		skb_orphan(skb);
> +}
> +
>  /* return NF_STOLEN (sent) or NF_ACCEPT if local=1 (not sent) */
>  static inline int ip_vs_nat_send_or_cont(int pf, struct sk_buff *skb,
>  					 struct ip_vs_conn *cp, int local)
> @@ -538,12 +553,21 @@ static inline int ip_vs_nat_send_or_cont(int pf, struct sk_buff *skb,
>  		ip_vs_notrack(skb);
>  	else
>  		ip_vs_update_conntrack(skb, cp, 1);
> +
> +	/* Remove the early_demux association unless it's bound for the
> +	 * exact same port and address on this host after translation.
> +	 */
> +	if (!local || cp->vport != cp->dport ||
> +	    !ip_vs_addr_equal(cp->af, &cp->vaddr, &cp->daddr))
> +		ip_vs_drop_early_demux_sk(skb);
> +
>  	if (!local) {
>  		skb_forward_csum(skb);
>  		NF_HOOK(pf, NF_INET_LOCAL_OUT, NULL, skb,
>  			NULL, skb_dst(skb)->dev, dst_output_sk);
>  	} else
>  		ret = NF_ACCEPT;
> +
>  	return ret;
>  }
>  
> @@ -557,6 +581,7 @@ static inline int ip_vs_send_or_cont(int pf, struct sk_buff *skb,
>  	if (likely(!(cp->flags & IP_VS_CONN_F_NFCT)))
>  		ip_vs_notrack(skb);
>  	if (!local) {
> +		ip_vs_drop_early_demux_sk(skb);
>  		skb_forward_csum(skb);
>  		NF_HOOK(pf, NF_INET_LOCAL_OUT, NULL, skb,
>  			NULL, skb_dst(skb)->dev, dst_output_sk);
> @@ -845,6 +870,8 @@ ip_vs_prepare_tunneled_skb(struct sk_buff *skb, int skb_af,
>  	struct ipv6hdr *old_ipv6h = NULL;
>  #endif
>  
> +	ip_vs_drop_early_demux_sk(skb);
> +
>  	if (skb_headroom(skb) < max_headroom || skb_cloned(skb)) {
>  		new_skb = skb_realloc_headroom(skb, max_headroom);
>  		if (!new_skb)
> -- 
> Alex Gartrell <agartrell@fb.com>

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Alex Gartrell]

On Sun, Jul 5, 2015 at 3:13 PM, Julian Anastasov <ja@ssi.bg> wrote:
>         May be the patch fixes crashes? If yes, Simon
> should apply it for ipvs/net tree, otherwise after
> the merge window...

Yeah this is definitely a crash-fix and it's existed since at least 3.10.

-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Simon Horman]

On Sun, Jul 05, 2015 at 03:19:27PM -0700, Alex Gartrell wrote:
> On Sun, Jul 5, 2015 at 3:13 PM, Julian Anastasov <ja@ssi.bg> wrote:
> >         May be the patch fixes crashes? If yes, Simon
> > should apply it for ipvs/net tree, otherwise after
> > the merge window...
> 
> Yeah this is definitely a crash-fix and it's existed since at least 3.10.

Is it possible to get a 'Fixes:' tag?

Re: [PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Alex Gartrell]

On Sun, Jul 5, 2015 at 8:50 PM, Simon Horman <horms@verge.net.au> wrote:
> Is it possible to get a 'Fixes:' tag?

I suppose it'd be appropriate to say

Fixes: 41063e9dd119 ("ipv4: Early TCP socket demux.")

As that is what introduces tcp early_demux, but that's just a guess as
I haven't bisected it (not even sure my test would run on that code
base).

-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Simon Horman]

On Sun, Jul 05, 2015 at 09:14:38PM -0700, Alex Gartrell wrote:
> On Sun, Jul 5, 2015 at 8:50 PM, Simon Horman <horms@verge.net.au> wrote:
> > Is it possible to get a 'Fixes:' tag?
> 
> I suppose it'd be appropriate to say
> 
> Fixes: 41063e9dd119 ("ipv4: Early TCP socket demux.")
> 
> As that is what introduces tcp early_demux, but that's just a guess as
> I haven't bisected it (not even sure my test would run on that code
> base).

Thanks. The reason that I am asking about this is to ease getting
this fix, or a derivative of it, into the appropriate stable trees.

Is there any possibility you could investigate which stable trees are
effected by this bug?

Re: [PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Alex Gartrell]

On Tue, Jul 7, 2015 at 6:33 PM, Simon Horman <horms@verge.net.au> wrote:

> Is there any possibility you could investigate which stable trees are
> effected by this bug?

Well this was certainly a problem in 3.10 (we had our own hacky
solution at the time) and every kernel since then would have also had
this problem.  I'm pretty sure this isn't a problem in 3.4 or before.

-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Eric Dumazet]

On Wed, 2015-07-08 at 13:56 -0700, Alex Gartrell wrote:
> On Tue, Jul 7, 2015 at 6:33 PM, Simon Horman <horms@verge.net.au> wrote:
> 
> > Is there any possibility you could investigate which stable trees are
> > effected by this bug?
> 
> Well this was certainly a problem in 3.10 (we had our own hacky
> solution at the time) and every kernel since then would have also had
> this problem.  I'm pretty sure this isn't a problem in 3.4 or before.
> 

early demux was added in 3.6

Fixes: 41063e9dd119 ("ipv4: Early TCP socket demux.")

Re: [PATCH,v2 net-next] ipvs: skb_orphan in case of forwarding

[Simon Horman]

On Wed, Jul 08, 2015 at 11:17:52PM +0200, Eric Dumazet wrote:
> On Wed, 2015-07-08 at 13:56 -0700, Alex Gartrell wrote:
> > On Tue, Jul 7, 2015 at 6:33 PM, Simon Horman <horms@verge.net.au> wrote:
> > 
> > > Is there any possibility you could investigate which stable trees are
> > > effected by this bug?
> > 
> > Well this was certainly a problem in 3.10 (we had our own hacky
> > solution at the time) and every kernel since then would have also had
> > this problem.  I'm pretty sure this isn't a problem in 3.4 or before.
> > 
> 
> early demux was added in 3.6
> 
> Fixes: 41063e9dd119 ("ipv4: Early TCP socket demux.")

Thanks Eric, Thanks Alex,

I think things are as clear as they need to be.
I'll add the above Fixes tag and get things queued up.