svn commit: r291652 - stable/10/sys/netinet

[Marcelo Araujo]

Hello,

I'm sorry, but that 'even' smell buggy for me! Could you explain what that
'even' really mean?

Best,
On Dec 3, 2015 1:26 AM, "Fabien Thomas" <fabient at freebsd.org> wrote:

> Author: fabient
> Date: Wed Dec  2 17:26:37 2015
> New Revision: 291652
> URL: https://svnweb.freebsd.org/changeset/base/291652
>
> Log:
>   MFC r291301:
>
>   The r241129 description was wrong that the scenario is possible
>    only for read locks on pcbs. The same race can happen with write
>    lock semantics as well.
>
>    The race scenario:
>
>    - Two threads (1 and 2) locate pcb with writer semantics
> (INPLOOKUP_WLOCKPCB)
>     and do in_pcbref() on it.
>    - 1 and 2 both drop the inp hash lock.
>    - Another thread (3) grabs the inp hash lock. Then it runs in_pcbfree(),
>     which wlocks the pcb. They must happen faster than 1 or 2 come
> INP_WLOCK()!
>    - 1 and 2 congest in INP_WLOCK().
>    - 3 does in_pcbremlists(), drops hash lock, and runs
> in_pcbrele_wlocked(),
>     which doesn't free the pcb due to two references on it.
>     Then it unlocks the pcb.
>    - 1 (or 2) gets wlock on the pcb, runs in_pcbrele_wlocked(), which
> doesn't
>     report inp as freed, due to 2 (or 1) still helding extra reference on
> it.
>     The thread tries to do smth with a disconnected pcb and crashes.
>
>    Submitted by:        emeric.poupon at stormshield.eu
>    Reviewed by: glebius@
>    Sponsored by: Stormshield
>    Tested by: Cassiano Peixoto, Stormshield
>
> Modified:
>   stable/10/sys/netinet/in_pcb.c
> Directory Properties:
>   stable/10/   (props changed)
>
> Modified: stable/10/sys/netinet/in_pcb.c
>
> ==============================================================================
> --- stable/10/sys/netinet/in_pcb.c      Wed Dec  2 16:29:36 2015
> (r291651)
> +++ stable/10/sys/netinet/in_pcb.c      Wed Dec  2 17:26:37 2015
> (r291652)
> @@ -1148,8 +1148,17 @@ in_pcbrele_wlocked(struct inpcb *inp)
>
>         INP_WLOCK_ASSERT(inp);
>
> -       if (refcount_release(&inp->inp_refcount) == 0)
> +       if (refcount_release(&inp->inp_refcount) == 0) {
> +               /*
> +                * If the inpcb has been freed, let the caller know, even
> if
> +                * this isn't the last reference.
> +                */
> +               if (inp->inp_flags2 & INP_FREED) {
> +                       INP_WUNLOCK(inp);
> +                       return (1);
> +               }
>                 return (0);
> +       }
>
>         KASSERT(inp->inp_socket == NULL, ("%s: inp_socket != NULL",
> __func__));
>
>
>