Re: [PATCH] sctp: check transport existence before processing a send primitive

[Xin Long]

On Fri, Apr 4, 2025 at 6:05 AM Ricardo Cañuelo Navarro <rcn@xxxxxxxxxx> wrote:
>
> Thanks for the suggestion!
>
> On Thu, Apr 03 2025 at 14:44:18, Xin Long <lucien.xin@xxxxxxxxx> wrote:
>
> > @@ -9234,7 +9236,7 @@ static int sctp_wait_for_sndbuf(struct
> > sctp_association *asoc, long *timeo_p,
> >                                           TASK_INTERRUPTIBLE);
> >                 if (asoc->base.dead)
> >                         goto do_dead;
> > -               if (!*timeo_p)
> > +               if (!*timeo_p || (t && t->dead))
> >                         goto do_nonblock;
> >                 if (sk->sk_err || asoc->state >= SCTP_STATE_SHUTDOWN_PENDING)
> >                         goto do_error;
>
> I suppose checking t->dead should be done after locking the socket
> again, where sctp_assoc_rm_peer() may have had a chance to run, rather
> than here?
>
It shouldn't matter, as long as it's protected by the socket lock.
The logic would be similar to checking asoc->base.dead.

> Something like this:
>
> @@ -9225,7 +9227,9 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>         pr_debug("%s: asoc:%p, timeo:%ld, msg_len:%zu\n", __func__, asoc,
>                  *timeo_p, msg_len);
>
> -       /* Increment the association's refcnt.  */
> +       /* Increment the transport and association's refcnt. */
> +       if (transport)
> +               sctp_transport_hold(transport);
>         sctp_association_hold(asoc);
>
>         /* Wait on the association specific sndbuf space. */
> @@ -9252,6 +9256,8 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>                 lock_sock(sk);
>                 if (sk != asoc->base.sk)
>                         goto do_error;
> +               if (transport && transport->dead)
> +                       goto do_nonblock;
>
>                 *timeo_p = current_timeo;
>         }
> @@ -9259,7 +9265,9 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>  out:
>         finish_wait(&asoc->wait, &wait);
>
> -       /* Release the association's refcnt.  */
> +       /* Release the transport and association's refcnt. */
> +       if (transport)
> +               sctp_transport_put(transport);
>         sctp_association_put(asoc);
>
>         return err;
>
>
> So by the time the sending thread re-claims the socket lock it can tell
> whether someone else removed the transport by checking transport->dead
> (set in sctp_transport_free()) and there's a guarantee that the
> transport hasn't been freed yet because we hold a reference to it.
>
> If the whole receive path through sctp_assoc_rm_peer() is protected by
> the same socket lock, as you said, this should be safe. The tests I ran
> seem to work fine. If you're ok with it I'll send another patch to
> supersede this one.
>
LGTM.

>
> > You will need to reintroduce the dead bit in struct sctp_transport and
> > set it in sctp_transport_free(). Note this field was previously removed in:
> >
> > commit 47faa1e4c50ec26e6e75dcd1ce53f064bd45f729
> > Author: Xin Long <lucien.xin@xxxxxxxxx>
> > Date:   Fri Jan 22 01:49:09 2016 +0800
> >
> >     sctp: remove the dead field of sctp_transport
>
> I understand that none of the transport->dead checks from that commit
> are necessary anymore, since they were replaced by refcnt checks, and
> that we'll only bring the bit back for this particular check we're doing
> now, correct?
Correct, only the 'dead' bit and set it in sctp_transport_free().

Thanks.