On Mon, Jan 15, 2018 at 05:01:36PM +0800, Xin Long wrote:
> After commit cea0cc80a677 ("sctp: use the right sk after waking up from
> wait_buf sleep"), it may change to lock another sk if the asoc has been
> peeled off in sctp_wait_for_sndbuf.
>
> However, the asoc's new sk could be already closed elsewhere, as it's in
> the sendmsg context of the old sk that can't avoid the new sk's closing.
> If the sk's last one refcnt is held by this asoc, later on after putting
> this asoc, the new sk will be freed, while under it's own lock.
>
> This patch is to revert that commit, but fix the old issue by returning
> error under the old sk's lock.
>
> Fixes: cea0cc80a677 ("sctp: use the right sk after waking up from wait_buf
> sleep")
> Reported-by: syzbot+ac6ea7baa4432811e...@syzkaller.appspotmail.com
> Signed-off-by: Xin Long <lucien....@gmail.com>
> ---
> net/sctp/socket.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 15ae018..feb2ca6 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -85,7 +85,7 @@
> static int sctp_writeable(struct sock *sk);
> static void sctp_wfree(struct sk_buff *skb);
> static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
> - size_t msg_len, struct sock **orig_sk);
> + size_t msg_len);
> static int sctp_wait_for_packet(struct sock *sk, int *err, long *timeo_p);
> static int sctp_wait_for_connect(struct sctp_association *, long *timeo_p);
> static int sctp_wait_for_accept(struct sock *sk, long timeo);
> @@ -1977,7 +1977,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr
> *msg, size_t msg_len)
> timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
> if (!sctp_wspace(asoc)) {
> /* sk can be changed by peel off when waiting for buf. */
> - err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len, &sk);
> + err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len);
> if (err) {
> if (err == -ESRCH) {
> /* asoc is already dead. */
> @@ -8022,12 +8022,12 @@ void sctp_sock_rfree(struct sk_buff *skb)
>
> /* Helper function to wait for space in the sndbuf. */
> static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
> - size_t msg_len, struct sock **orig_sk)
> + size_t msg_len)
> {
> struct sock *sk = asoc->base.sk;
> - int err = 0;
> long current_timeo = *timeo_p;
> DEFINE_WAIT(wait);
> + int err = 0;
>
> pr_debug("%s: asoc:%p, timeo:%ld, msg_len:%zu\n", __func__, asoc,
> *timeo_p, msg_len);
> @@ -8056,17 +8056,13 @@ static int sctp_wait_for_sndbuf(struct
> sctp_association *asoc, long *timeo_p,
> release_sock(sk);
> current_timeo = schedule_timeout(current_timeo);
> lock_sock(sk);
> - if (sk != asoc->base.sk) {
> - release_sock(sk);
> - sk = asoc->base.sk;
> - lock_sock(sk);
> - }
> + if (sk != asoc->base.sk)
> + goto do_error;
Is this a safe comparison to make (thinking in terms both of non-cache coherent
arches, or, more likely, of cases where the sock slab reuses an object leading
to the same pointer). Would it be better to have a single point of freeing the
sock and use the SOCK_DEAD flag here?
Neil
>
> *timeo_p = current_timeo;
> }
>
> out:
> - *orig_sk = sk;
> finish_wait(&asoc->wait, &wait);
>
> /* Release the association's refcnt. */
> --
> 2.1.0
>
>
