Re: [PATCH net 2/3] udp: check encap socket in __udp_lib_err

On Mon, Jul 12, 2021 at 9:37 AM Paolo Abeni [off-list ref] wrote:

On Mon, 2021-07-12 at 13:45 +0100, Vadim Fedorenko wrote:

quoted

quoted

After this patch, the above chunk will not clear 'sk' for packets
targeting ESP in UDP sockets, but AFAICS we will still enter the
following conditional, preserving the current behavior - no ICMP
processing.

We will not enter following conditional for ESP in UDP case because
there is no more check for encap_type or encap_enabled.

I see. You have a bug in the ipv6 code-path. With your patch applied:

---
        sk = __udp6_lib_lookup(net, daddr, uh->dest, saddr, uh->source,
                               inet6_iif(skb), inet6_sdif(skb), udptable, NULL);
        if (sk && udp_sk(sk)->encap_enabled) {
                //...
        }

        if (!sk || udp_sk(sk)->encap_enabled) {
        // can still enter here...
---

quoted

I maybe missing something but d26796ae5894 doesn't actually explain
which particular situation should be avoided by this additional check
and no tests were added to simply reproduce the problem. If you can
explain it a bit more it would greatly help me to improve the fix.

Xin knows better, but AFAICS it used to cover the situation you
explicitly tests in patch 3/3 - incoming packet with src-port == dst-
port == tunnel port - for e.g. vxlan tunnels.

Thanks Paolo and sorry for late.

Right, __udp4/6_lib_err_encap() was introduced to process the ICMP error
packets for UDP tunnels. But it will only work when there's no socket
found with src + dst port, as when the src == dst port a socket might
be found(if the bind addr is ANY) and the code will be called.

quoted

quoted

Why can't you use something alike the following instead?

---

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index c0f9f3260051..96a3b640e4da 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c

@@ -707,7 +707,7 @@ int __udp4_lib_err(struct sk_buff *skb, u32 info, struct udp_table *udptable)
         sk = __udp4_lib_lookup(net, iph->daddr, uh->dest,
                                iph->saddr, uh->source, skb->dev->ifindex,
                                inet_sdif(skb), udptable, NULL);
-       if (!sk || udp_sk(sk)->encap_type) {
+       if (!sk || READ_ONCE(udp_sk(sk)->encap_err_lookup)) {
                 /* No socket for error: try tunnels before discarding */
                 sk = ERR_PTR(-ENOENT);
                 if (static_branch_unlikely(&udp_encap_needed_key)) {

---

Could you please have a look at the above ?

If not all udp tunnels want to do further validation for ICMP error packet,
This looks good to me.

Thanks!

/P