netdev - Re: [PATCH net 1/6] sctp: remove the unnecessary state check in sctp_outq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Sep 2016 10:28:41 -0400
From:   Neil Horman <nhorman@...driver.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem <davem@...emloft.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Vlad Yasevich <vyasevich@...il.com>, daniel@...earbox.net
Subject: Re: [PATCH net 1/6] sctp: remove the unnecessary state check in
 sctp_outq_tail

On Fri, Sep 09, 2016 at 03:11:57PM +0800, Xin Long wrote:
> >>
> > Its not enough to just look at the paths where outq_tail is called, because
> > the outq_tail function is checking a state variable that can update
> > asynchronously in the side effect processing queue.  Look at any one of the
> > timer functions.  Those all fire asynchronous to the outq list, but they all
> 
> Fortunately, sctp_do_sm is protected by lock_sock.
> I just checked all the sctp_do_sm calling in  timer functions.
> 
> ------------
>         bh_lock_sock(sk);
>         if (sock_owned_by_user(sk)) {    <----------- [1]
>                 pr_debug("%s: sock is busy\n", __func__);
>                 /* Try again later.  */
>                 mod_timer()
>                 goto out_unlock;
>         }
>         error = sctp_do_sm(net, SCTP_EVENT_T_TIMEOUT,
> out_unlock:
>         bh_unlock_sock(sk);
> 
> 
> which means sctp_do_sm is *non-reenterable* because of
> sock lock's protection. even if it's interrupted by something
> like timers, they have to delay their real process until sock
> lock is released.
> 
> So It's still safe here. pls double-check.
> 
I don't know, I still don't feel safe about it.  I agree the socket lock keeps
the state from changing during a single transmission, which makes the use case
you are focused on correct.

That said, have you considered the retransmit case?  That is to say, if you
queue and flush the outq, and some packets fail delivery, and in the time
between the intial send and the expiration of the RTX timer (during which the
socket lock will have been released), an event may occur which changes the
transport state, which will then be ignored with your patch.

Neil