Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vadim Fedorenko <vfedorenko@novek.ru>
To: Paolo Abeni <pabeni@redhat.com>, David Ahern <dsahern@kernel.org>,
	Willem de Bruijn <willemb@google.com>,
	Xin Long <lucien.xin@gmail.com>
Cc: Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org
Subject: Re: [PATCH net 2/3] udp: check encap socket in __udp_lib_err
Date: Mon, 12 Jul 2021 15:05:51 +0100	[thread overview]
Message-ID: <74c28a85-ad6d-0b33-b5be-90b1bff7ca52@novek.ru> (raw)
In-Reply-To: <cb9830bd8ef1edc3b5a5f11546618cd50ed82f21.camel@redhat.com>

On 12.07.2021 14:37, Paolo Abeni wrote:
> On Mon, 2021-07-12 at 13:45 +0100, Vadim Fedorenko wrote:
>>
>>> 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...
> ---	
> 

Oh, my bad, thanks for catching this!

>> 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.
>

Ok, so my assumption was like yours, that's good.

>>> 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 ?
> 
Sure. The main problem I see here is that udp4_lib_lookup in udp_lib_err_encap
could return different socket because of different source and destination port
and in this case we will never check for correctness of originally found socket,
i.e. encap_err_lookup will never be called and the ICMP notification will never
be applied to that socket even if it passes checks.
My point is that it's simplier to explicitly check socket that was found than
rely on the result of udp4_lib_lookup with different inputs and leave the case
of no socket as it was before d26796ae5894.

If it's ok, I will unify the code for check as Willem suggested and resend v2.


  reply	other threads:[~2021-07-12 14:05 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-12  0:55 [PATCH net 0/3] Fix PMTU for ESP-in-UDP encapsulation Vadim Fedorenko
2021-07-12  0:55 ` [PATCH net 1/3] udp: check for encap using encap_enable Vadim Fedorenko
2021-07-12  8:37   ` Paolo Abeni
2021-07-12 12:32     ` Vadim Fedorenko
2021-07-12 14:05       ` Paolo Abeni
2021-07-12 14:13         ` Vadim Fedorenko
2021-07-12 14:33           ` Paolo Abeni
2021-07-12 16:27             ` Vadim Fedorenko
2021-07-12  0:55 ` [PATCH net 2/3] udp: check encap socket in __udp_lib_err Vadim Fedorenko
2021-07-12  7:59   ` Willem de Bruijn
2021-07-12 12:09     ` Vadim Fedorenko
2021-07-12  9:07   ` Paolo Abeni
2021-07-12 12:45     ` Vadim Fedorenko
2021-07-12 13:37       ` Paolo Abeni
2021-07-12 14:05         ` Vadim Fedorenko [this message]
2021-07-12 14:09           ` Paolo Abeni
2021-07-16 17:50         ` Xin Long
2021-07-12  0:55 ` [PATCH net 3/3] selftests: net: add ESP-in-UDP PMTU test Vadim Fedorenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=74c28a85-ad6d-0b33-b5be-90b1bff7ca52@novek.ru \
    --to=vfedorenko@novek.ru \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=kuba@kernel.org \
    --cc=lucien.xin@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=willemb@google.com \
    --subject='Re: [PATCH net 2/3] udp: check encap socket in __udp_lib_err' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).