[PATCH V2 0/2] net: socket: return a proper error code when source address becomes nonlocal

[PATCH V2 0/2] net: socket: return a proper error code when source address becomes nonlocal

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

This patch version 2 spilt the original patch into 2 patches, because it fix
two separate problems actually.

Liping Zhang (2):
  net: socket: return EADDRNOTAVAIL when source address becomes
    nonlocal
  net: socket: return EADDRNOTAVAIL when IPV6_PKTINFO's ipi6_addr is
    not available

 net/ipv4/route.c    |    6 ++++--
 net/ipv6/datagram.c |    2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[PATCH V2 1/2] net: socket: return EADDRNOTAVAIL when source address becomes nonlocal

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

A socket can use bind(directly) or connect(indirectly) to bind to a local
ip address, and later if the network becomes down, that cause the source
address becomes nonlocal, then send() call will fail and return EINVAL.
But this error code is confusing, acctually we did not pass any invalid
arguments. Furthermore, send() maybe return ok at first, it now returns
fail just because of a temporary network problem, i.e. when the network
recovery, send() call will become ok. Return EADDRNOTAVAIL instead of
EINVAL in such situation is better.

Take the ping utility for example, we can use -I option to specify the
source address (e.g ping -I 10.19.145.26 117.135.169.41), when network
becomes down, error message will be printed out as follows:
    64 bytes from  (117.135.169.41): icmp_seq=9 ttl=54 time=46.7 ms
    ping: sendmsg: Invalid argument
    ping: sendmsg: Invalid argument

Apply this patch, error message will become:
    64 bytes from  (117.135.169.41): icmp_seq=5 ttl=54 time=47.5 ms
    ping: sendmsg: Cannot assign requested address
    ping: sendmsg: Cannot assign requested address

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/ipv4/route.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 02c6229..857f7b3 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2149,11 +2149,13 @@ struct rtable *__ip_route_output_key_hash(struct net *net, struct flowi4 *fl4,
 
 	rcu_read_lock();
 	if (fl4->saddr) {
-		rth = ERR_PTR(-EINVAL);
+		rth = ERR_PTR(-EADDRNOTAVAIL);
 		if (ipv4_is_multicast(fl4->saddr) ||
 		    ipv4_is_lbcast(fl4->saddr) ||
-		    ipv4_is_zeronet(fl4->saddr))
+		    ipv4_is_zeronet(fl4->saddr)) {
+			rth = ERR_PTR(-EINVAL);
 			goto out;
+		}
 
 		/* I removed check for oif == dev_out->oif here.
 		   It was wrong for two reasons:
-- 
1.7.9.5

[PATCH V2 2/2] net: socket: return EADDRNOTAVAIL when IPV6_PKTINFO's ipi6_addr is not available

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

We can use IPV6_PKTINFO to specify the ipv6 source address when call
sendmsg() to send packet, but if the address is not available, call will
fail and EINVAL is returned. This error code is not very appropriate,
it failed maybe just because of a temporary network problem, i.e. when
the network recovery, sendmsg() call will become ok. Also RFC3542,
section 6.6 describe an example in error scenario that returns
EADDRNOTAVAIL: "ipi6_ifindex specifies an interface but the address
ipi6_addr is not available for use on that interface.". So return
EADDRNOTAVAIL instead of EINVAL here.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/ipv6/datagram.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index a73d701..20a968b 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -753,7 +753,7 @@ int ip6_datagram_send_ctl(struct net *net, struct sock *sk,
 						   strict ? dev : NULL, 0) &&
 				    !ipv6_chk_acast_addr_src(net, dev,
 							     &src_info->ipi6_addr))
-					err = -EINVAL;
+					err = -EADDRNOTAVAIL;
 				else
 					fl6->saddr = src_info->ipi6_addr;
 			}
-- 
1.7.9.5

Re: [PATCH V2 1/2] net: socket: return EADDRNOTAVAIL when source address becomes nonlocal

[David Miller]

From: Liping Zhang <zlpnobody@163.com>
Date: Tue,  5 Apr 2016 22:19:45 +0800

> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> A socket can use bind(directly) or connect(indirectly) to bind to a local
> ip address, and later if the network becomes down, that cause the source
> address becomes nonlocal, then send() call will fail and return EINVAL.
> But this error code is confusing, acctually we did not pass any invalid
> arguments. Furthermore, send() maybe return ok at first, it now returns
> fail just because of a temporary network problem, i.e. when the network
> recovery, send() call will become ok. Return EADDRNOTAVAIL instead of
> EINVAL in such situation is better.
> 
> Take the ping utility for example, we can use -I option to specify the
> source address (e.g ping -I 10.19.145.26 117.135.169.41), when network
> becomes down, error message will be printed out as follows:
>     64 bytes from  (117.135.169.41): icmp_seq=9 ttl=54 time=46.7 ms
>     ping: sendmsg: Invalid argument
>     ping: sendmsg: Invalid argument
> 
> Apply this patch, error message will become:
>     64 bytes from  (117.135.169.41): icmp_seq=5 ttl=54 time=47.5 ms
>     ping: sendmsg: Cannot assign requested address
>     ping: sendmsg: Cannot assign requested address
> 
> Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>

Changing the behavior of such a core, and fundamental function, with
hundreds of direct and indirect call sites is extremely dangerous.

I was worried that someone might depend upon the -EINVAL return value,
and indeed I quickly found that the code in net/netfilter/ipvs/ip_vs_xmit.c
does want to see EINVAL in this case.

I refuse to audit the entire call chain of all users of this helper
function, as I said there are hundreds.  But there are potentially
many other call sites, either direct or indirect, that have this
problem as well.

And furthermore, USERSPACE itself might depend upon the error code
being -EINVAL since we've been returning it for several decades.

To be quite honest I think the value of this change is very low and
with the risk factors involved here I really would rather not make
this change at all.

Sorry.