| Message ID | 20240430140343.389543-1-tparkin@katalix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] l2tp: fix ICMP error handling for UDP-encap sockets | expand |
On 30/04/2024 15:03, Tom Parkin wrote: > Since commit a36e185e8c85 > ("udp: Handle ICMP errors for tunnels with same destination port on both endpoints") > UDP's handling of ICMP errors has allowed for UDP-encap tunnels to > determine socket associations in scenarios where the UDP hash lookup > could not. > > Subsequently, commit d26796ae58940 > ("udp: check udp sock encap_type in __udp_lib_err") > subtly tweaked the approach such that UDP ICMP error handling would be > skipped for any UDP socket which has encapsulation enabled. > > In the case of L2TP tunnel sockets using UDP-encap, this latter > modification effectively broke ICMP error reporting for the L2TP > control plane. > > To a degree this isn't catastrophic inasmuch as the L2TP control > protocol defines a reliable transport on top of the underlying packet > switching network which will eventually detect errors and time out. > > However, paying attention to the ICMP error reporting allows for more > timely detection of errors in L2TP userspace, and aids in debugging > connectivity issues. > > Reinstate ICMP error handling for UDP encap L2TP tunnels: > > * implement struct udp_tunnel_sock_cfg .encap_err_rcv in order to allow > the L2TP code to handle ICMP errors; > > * only implement error-handling for tunnels which have a managed > socket: unmanaged tunnels using a kernel socket have no userspace to > report errors back to; > > * flag the error on the socket, which allows for userspace to get an > error such as -ECONNREFUSED back from sendmsg/recvmsg; > > * pass the error into ip[v6]_icmp_error() which allows for userspace to > get extended error information via. MSG_ERRQUEUE. > > Fixes: d26796ae5894 ("udp: check udp sock encap_type in __udp_lib_err") > Signed-off-by: Tom Parkin <tparkin@katalix.com> Reviewed-by: James Chapman <jchapman@katalix.com>
On Tue, 30 Apr 2024 15:03:43 +0100 Tom Parkin wrote: > Subject: [PATCH net-next] l2tp: fix ICMP error handling for UDP-encap sockets Seems like we should target it at net? Description indicates it's a clear regression. > +static void l2tp_udp_encap_err_recv(struct sock *sk, struct sk_buff *skb, int err, > + __be16 port, u32 info, u8 *payload) > +{ > + struct l2tp_tunnel *tunnel = l2tp_sk_to_tunnel(sk); > + > + if (!tunnel || tunnel->fd < 0) > + return; not: the !tunnel can't happen, right? > + sk->sk_err = err; > + sk_error_report(sk); > + > + if (ip_hdr(skb)->version == IPVERSION) { > + if (inet_test_bit(RECVERR, sk)) > + return ip_icmp_error(sk, skb, err, port, info, payload); > + } > +#if IS_ENABLED(CONFIG_IPV6) > + else > + if (inet6_test_bit(RECVERR6, sk)) > + return ipv6_icmp_error(sk, skb, err, port, info, payload); > +#endif nit: mismatch on the braces here, this would be more usual: + if (ip_hdr(skb)->version == IPVERSION) { + if (inet_test_bit(RECVERR, sk)) + return ip_icmp_error(sk, skb, err, port, info, payload); +#if IS_ENABLED(CONFIG_IPV6) + } else { + if (inet6_test_bit(RECVERR6, sk)) + return ipv6_icmp_error(sk, skb, err, port, info, payload); +#endif + } +}
Thanks Jakub, On Fri, May 03, 2024 at 15:32:14 -0700, Jakub Kicinski wrote: > Seems like we should target it at net? Description indicates it's > a clear regression. Ack, I'll respin for net. > not: the !tunnel can't happen, right? Your question makes me realise that l2tp_udp_encap_err_recv is being called in the same context as l2tp_udp_encap_recv, and so should be using rcu_dereference_sk_user_data to access the tunnel handle rather than l2tp_sk_to_tunnel. I'll fix that in the respin. However I note that l2tp_udp_encap_recv also checks for the tunnel handle being NULL :-| > nit: mismatch on the braces here, this would be more usual: > > + if (ip_hdr(skb)->version == IPVERSION) { > + if (inet_test_bit(RECVERR, sk)) > + return ip_icmp_error(sk, skb, err, port, info, payload); > +#if IS_ENABLED(CONFIG_IPV6) > + } else { > + if (inet6_test_bit(RECVERR6, sk)) > + return ipv6_icmp_error(sk, skb, err, port, info, payload); > +#endif > + } > > +} Thanks, I will fix this.
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 8d21ff25f160..0a23de37d2d7 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -919,6 +919,28 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb) } EXPORT_SYMBOL_GPL(l2tp_udp_encap_recv); +static void l2tp_udp_encap_err_recv(struct sock *sk, struct sk_buff *skb, int err, + __be16 port, u32 info, u8 *payload) +{ + struct l2tp_tunnel *tunnel = l2tp_sk_to_tunnel(sk); + + if (!tunnel || tunnel->fd < 0) + return; + + sk->sk_err = err; + sk_error_report(sk); + + if (ip_hdr(skb)->version == IPVERSION) { + if (inet_test_bit(RECVERR, sk)) + return ip_icmp_error(sk, skb, err, port, info, payload); + } +#if IS_ENABLED(CONFIG_IPV6) + else + if (inet6_test_bit(RECVERR6, sk)) + return ipv6_icmp_error(sk, skb, err, port, info, payload); +#endif +} + /************************************************************************ * Transmit handling ***********************************************************************/ @@ -1493,6 +1515,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, .sk_user_data = tunnel, .encap_type = UDP_ENCAP_L2TPINUDP, .encap_rcv = l2tp_udp_encap_recv, + .encap_err_rcv = l2tp_udp_encap_err_recv, .encap_destroy = l2tp_udp_encap_destroy, };
Since commit a36e185e8c85 ("udp: Handle ICMP errors for tunnels with same destination port on both endpoints") UDP's handling of ICMP errors has allowed for UDP-encap tunnels to determine socket associations in scenarios where the UDP hash lookup could not. Subsequently, commit d26796ae58940 ("udp: check udp sock encap_type in __udp_lib_err") subtly tweaked the approach such that UDP ICMP error handling would be skipped for any UDP socket which has encapsulation enabled. In the case of L2TP tunnel sockets using UDP-encap, this latter modification effectively broke ICMP error reporting for the L2TP control plane. To a degree this isn't catastrophic inasmuch as the L2TP control protocol defines a reliable transport on top of the underlying packet switching network which will eventually detect errors and time out. However, paying attention to the ICMP error reporting allows for more timely detection of errors in L2TP userspace, and aids in debugging connectivity issues. Reinstate ICMP error handling for UDP encap L2TP tunnels: * implement struct udp_tunnel_sock_cfg .encap_err_rcv in order to allow the L2TP code to handle ICMP errors; * only implement error-handling for tunnels which have a managed socket: unmanaged tunnels using a kernel socket have no userspace to report errors back to; * flag the error on the socket, which allows for userspace to get an error such as -ECONNREFUSED back from sendmsg/recvmsg; * pass the error into ip[v6]_icmp_error() which allows for userspace to get extended error information via. MSG_ERRQUEUE. Fixes: d26796ae5894 ("udp: check udp sock encap_type in __udp_lib_err") Signed-off-by: Tom Parkin <tparkin@katalix.com> --- net/l2tp/l2tp_core.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)