All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] ipv4: Return -ENETUNREACH if we can't create route but saddr is valid
@ 2019-10-10 15:51 Stefano Brivio
  2019-10-16 18:21 ` David Miller
  0 siblings, 1 reply; 3+ messages in thread
From: Stefano Brivio @ 2019-10-10 15:51 UTC (permalink / raw)
  To: David Miller
  Cc: Stefan Walter, Benjamin Coddington, Gonzalo Siero,
	Nikola Forró,
	Eric Dumazet, netdev

...instead of -EINVAL. An issue was found with older kernel versions
while unplugging a NFS client with pending RPCs, and the wrong error
code here prevented it from recovering once link is back up with a
configured address.

Incidentally, this is not an issue anymore since commit 4f8943f80883
("SUNRPC: Replace direct task wakeups from softirq context"), included
in 5.2-rc7, had the effect of decoupling the forwarding of this error
by using SO_ERROR in xs_wake_error(), as pointed out by Benjamin
Coddington.

To the best of my knowledge, this isn't currently causing any further
issue, but the error code doesn't look appropriate anyway, and we
might hit this in other paths as well. So I'm addressing this for
net-next, but it might be worth to queue this for stable < 5.2.

In detail, as analysed by Gonzalo Siero, once the route is deleted
because the interface is down, and can't be resolved and we return
-EINVAL here, this ends up, courtesy of inet_sk_rebuild_header(),
as the socket error seen by tcp_write_err(), called by
tcp_retransmit_timer().

In turn, tcp_write_err() indirectly calls xs_error_report(), which
wakes up the RPC pending tasks with a status of -EINVAL. This is then
seen by call_status() in the SUN RPC implementation, which aborts the
RPC call calling rpc_exit(), instead of handling this as a
potentially temporary condition, i.e. as a timeout.

Return -EINVAL only if the input parameters passed to
ip_route_output_key_hash_rcu() are actually invalid (this is the case
if the specified source address is multicast, limited broadcast or
all zeroes), but return -ENETUNREACH in all cases where, at the given
moment, the given source address doesn't allow resolving the route.

While at it, drop the initialisation of err to -ENETUNREACH, which
was added to __ip_route_output_key() back then by commit
0315e3827048 ("net: Fix behaviour of unreachable, blackhole and
prohibit routes"), but actually had no effect, as it was, and is,
overwritten by the fib_lookup() return code assignment, and anyway
ignored in all other branches, including the if (fl4->saddr) one:
I find this rather confusing, as it would look like -ENETUNREACH is
the "default" error, while that statement has no effect.

Also note that after commit fc75fc8339e7 ("ipv4: dont create routes
on down devices"), we would get -ENETUNREACH if the device is down,
but -EINVAL if the source address is specified and we can't resolve
the route, and this appears to be rather inconsistent.

Reported-by: Stefan Walter <walteste@inf.ethz.ch>
Analysed-by: Benjamin Coddington <bcodding@redhat.com>
Analysed-by: Gonzalo Siero <gsierohu@redhat.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
I think this should be considered for -stable, < 5.2

 net/ipv4/route.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 14654876127e..5bc172abd143 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2470,14 +2470,17 @@ struct rtable *ip_route_output_key_hash_rcu(struct net *net, struct flowi4 *fl4,
 	int orig_oif = fl4->flowi4_oif;
 	unsigned int flags = 0;
 	struct rtable *rth;
-	int err = -ENETUNREACH;
+	int err;
 
 	if (fl4->saddr) {
-		rth = ERR_PTR(-EINVAL);
 		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;
+		}
+
+		rth = ERR_PTR(-ENETUNREACH);
 
 		/* I removed check for oif == dev_out->oif here.
 		   It was wrong for two reasons:
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH net-next] ipv4: Return -ENETUNREACH if we can't create route but saddr is valid
  2019-10-10 15:51 [PATCH net-next] ipv4: Return -ENETUNREACH if we can't create route but saddr is valid Stefano Brivio
@ 2019-10-16 18:21 ` David Miller
  2019-10-16 18:51   ` Stefano Brivio
  0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2019-10-16 18:21 UTC (permalink / raw)
  To: sbrivio; +Cc: walteste, bcodding, gsierohu, nforro, edumazet, netdev

From: Stefano Brivio <sbrivio@redhat.com>
Date: Thu, 10 Oct 2019 17:51:50 +0200

> I think this should be considered for -stable, < 5.2

Changes meant for -stable should not target net-next, but rather net.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH net-next] ipv4: Return -ENETUNREACH if we can't create route but saddr is valid
  2019-10-16 18:21 ` David Miller
@ 2019-10-16 18:51   ` Stefano Brivio
  0 siblings, 0 replies; 3+ messages in thread
From: Stefano Brivio @ 2019-10-16 18:51 UTC (permalink / raw)
  To: David Miller; +Cc: walteste, bcodding, gsierohu, nforro, edumazet, netdev

On Wed, 16 Oct 2019 14:21:59 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Stefano Brivio <sbrivio@redhat.com>
> Date: Thu, 10 Oct 2019 17:51:50 +0200
> 
> > I think this should be considered for -stable, < 5.2  
> 
> Changes meant for -stable should not target net-next, but rather net.

Oh, sorry for that. I thought this would be the best way for patches
that are not strictly (or proven) fixes for net, but still make sense
for stable.

I generalised David Ahern's hint from a different case, I thought you
agreed (<20190618.092512.1610110055396742434.davem@davemloft.net>).

Resending for net now.

-- 
Stefano

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-10-16 18:51 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-10 15:51 [PATCH net-next] ipv4: Return -ENETUNREACH if we can't create route but saddr is valid Stefano Brivio
2019-10-16 18:21 ` David Miller
2019-10-16 18:51   ` Stefano Brivio

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.