[PATCH] net: ipv4: ip_send_unicast_reply should set oif only if it is L3 master

[PATCH] net: ipv4: ip_send_unicast_reply should set oif only if it is L3 master

[David Ahern]

Lorenzo noted an Android unit test failed due to commit e0d56fdd7342:
  "The expectation in the test was that the RST replying to a SYN sent to a
  closed port should be generated with oif=0. In other words it should not
  prefer the interface where the SYN came in on, but instead should follow
  whatever the routing table says it should do."

Since this a change in behavior, revert the change to
ip_send_unicast_reply such that the oif in the flow is set to the skb_iif
only if skb_iif is an L3 master.

Fixes: e0d56fdd7342 ("net: l3mdev: remove redundant calls")
Reported-by: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/ip_output.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 49714010ac2e..9403fa3850be 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1577,7 +1577,8 @@ void ip_send_unicast_reply(struct sock *sk, struct sk_buff *skb,
 	}
 
 	oif = arg->bound_dev_if;
-	oif = oif ? : skb->skb_iif;
+	if (!oif && netif_index_is_l3_master(net, skb->skb_iif))
+		oif = skb->skb_iif;
 
 	flowi4_init_output(&fl4, oif,
 			   IP4_REPLY_MARK(net, skb->mark),
-- 
2.1.4

Re: [PATCH] net: ipv4: ip_send_unicast_reply should set oif only if it is L3 master

[Lorenzo Colitti]

On Wed, Nov 9, 2016 at 7:50 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> @@ -1577,7 +1577,8 @@ void ip_send_unicast_reply(struct sock *sk, struct sk_buff *skb,

Tested-by: Lorenzo Colitti <lorenzo@google.com>

This fixes the IPv4 test, thanks. I notice that 4.8 didn't have
e0d56fdd73, so if this patch can get into 4.9 then there will be no
release that had the behaviour change. Not sure if that's possible any
more though.

Can you also fix tcp_v6_send_response, which suffers from the same
problem? Perhaps revert this hunk of e0d56fdd73 ("net: l3mdev: remove
redundant calls"):

@@ -818,12 +818,8 @@ static void tcp_v6_send_response(const struct
sock *sk, struct sk_buff *skb, u32
        fl6.flowi6_proto = IPPROTO_TCP;
        if (rt6_need_strict(&fl6.daddr) && !oif)
                fl6.flowi6_oif = tcp_v6_iif(skb);
-       else {
-               if (!oif && netif_index_is_l3_master(net, skb->skb_iif))
-                       oif = skb->skb_iif;
-
-               fl6.flowi6_oif = oif;
-       }
+       else
+               fl6.flowi6_oif = oif ? : skb->skb_iif;

        fl6.flowi6_mark = IP6_REPLY_MARK(net, skb->mark);
        fl6.fl6_dport = t1->dest;

Re: [PATCH] net: ipv4: ip_send_unicast_reply should set oif only if it is L3 master

[David Ahern]

On 11/8/16 11:38 PM, Lorenzo Colitti wrote:
> On Wed, Nov 9, 2016 at 7:50 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
>> @@ -1577,7 +1577,8 @@ void ip_send_unicast_reply(struct sock *sk, struct sk_buff *skb,
> 
> Tested-by: Lorenzo Colitti <lorenzo@google.com>
> 
> This fixes the IPv4 test, thanks. I notice that 4.8 didn't have
> e0d56fdd73, so if this patch can get into 4.9 then there will be no
> release that had the behaviour change. Not sure if that's possible any
> more though.

This patch is for net so 4.9.

> 
> Can you also fix tcp_v6_send_response, which suffers from the same
> problem? Perhaps revert this hunk of e0d56fdd73 ("net: l3mdev: remove
> redundant calls"):
> 

Let me add it to this patch and re-send.

Re: [PATCH] net: ipv4: ip_send_unicast_reply should set oif only if it is L3 master

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Tue,  8 Nov 2016 14:50:31 -0800

> Lorenzo noted an Android unit test failed due to commit e0d56fdd7342:
>   "The expectation in the test was that the RST replying to a SYN sent to a
>   closed port should be generated with oif=0. In other words it should not
>   prefer the interface where the SYN came in on, but instead should follow
>   whatever the routing table says it should do."
> 
> Since this a change in behavior, revert the change to
> ip_send_unicast_reply such that the oif in the flow is set to the skb_iif
> only if skb_iif is an L3 master.
> 
> Fixes: e0d56fdd7342 ("net: l3mdev: remove redundant calls")
> Reported-by: Lorenzo Colitti <lorenzo@google.com>
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>

David, I'm assuming that a new spin of this patch is coming.

Re: [PATCH] net: ipv4: ip_send_unicast_reply should set oif only if it is L3 master

[David Ahern]

On 11/9/16 7:48 PM, David Miller wrote:
> From: David Ahern <dsa@cumulusnetworks.com>
> Date: Tue,  8 Nov 2016 14:50:31 -0800
> 
>> Lorenzo noted an Android unit test failed due to commit e0d56fdd7342:
>>   "The expectation in the test was that the RST replying to a SYN sent to a
>>   closed port should be generated with oif=0. In other words it should not
>>   prefer the interface where the SYN came in on, but instead should follow
>>   whatever the routing table says it should do."
>>
>> Since this a change in behavior, revert the change to
>> ip_send_unicast_reply such that the oif in the flow is set to the skb_iif
>> only if skb_iif is an L3 master.
>>
>> Fixes: e0d56fdd7342 ("net: l3mdev: remove redundant calls")
>> Reported-by: Lorenzo Colitti <lorenzo@google.com>
>> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> 
> David, I'm assuming that a new spin of this patch is coming.
> 

yes. posted this morning; Lorenzo tested and ack'ed an hour or so ago. Since it expanded to include IPv6, the subject line changed so it won't be readily apparent.