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

On Sat, Sep 10, 2016 at 12:03:53AM +0800, Xin Long wrote:

quoted

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.

ok, :-)

quoted

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.

Sorry, I'm not sure if I got it.

You mean "during which changes q->asoc->state", right ?

This patch removes the check of q->asoc->state in sctp_outq_tail().

sctp_outq_tail() is called for data only in:
sctp_primitive_SEND -> sctp_do_sm -> sctp_cmd_send_msg ->
sctp_cmd_interpreter -> sctp_cmd_send_msg() -> sctp_outq_tail()

before calling sctp_primitive_SEND, hold sock lock first.
then sctp_primitive_SEND choose FUNC according:

#define TYPE_SCTP_PRIMITIVE_SEND  {
....

if asoc->state is unavailable, FUNC can't be sctp_cmd_send_msg,
but sctp_sf_error_closed/sctp_sf_error_shutdown,  sctp_outq_tail
can't be called, either.
I mean sctp_primitive_SEND do the same check for asoc->state
already actually.

so the code in sctp_outq_tail is redundant actually.

quoted

Neil

Ok, you've convinced me, thanks for taking the time to go through it

Acked-by: Neil Horman <nhorman@tuxdriver.com>