Re: [PATCH net] sctp: fix a success return may hide an error

[Xin Long <lucien.xin@gmail.com>]

>>
>> [1]
>> Both sctp_outq_flush_rtx and sctp_packet_transmit can ONLY
>> return one error (-ENOMEM), as sctp_outq_flush_rtx also calls
>> sctp_packet_transmit.
>
> What is the effect of the error?
> If it is 'just' equivalent to a lost ethernet packet (and the skb (etc)
> is freed) then the protocol will recover.
> If it is anything else then the error path is probably wrong.
This err returns back to sctp_sendmsg, there sctp will abort asoc.
in this function, sctp tries to do 3 things:
1. flush rtx queue
2. transmit the packet of current transport
3. flush all the transports.
Now sctp would do them one by one, even if one of them returns err.

>
> Also after one error is it actually worth trying to send anything else
> at all? ISTM that the code should either:
yeah, that's the problem.
the "sctp_flush_out:" code tries to force clear all the transport before
returning even if there're errors already.

> 1) wait for resources and retry.
> 2) discard the entire queue (freeing resource) and hope the protocol
>    timers will recover.
It's a different process, will think about it.

>
>> [2]
>> It's the original codes that it doesn't return immediately when
>> sctp_outq_flush_rtx returns error. I guess it just doesn't want
>> to stop flushing out transport_list only because it fail to flush
>> rtx.
>> even sctp_packet_transmit_chunk in sctp_outq_flush also just
>> put the error into sk->sk_err, instread of returning immediately.
>>
>> So we cannot return the err at the first failure as [2], the error
>> here is always -ENOMEM as [1].
>> I think to return the last error here is ok, at least  not dangerous,
>> can also fix the issue "a success return may hide an error" with
>> clear codes. :)
>
> Which code looks at sk->sk_err?
> It doesn't look right to be setting an error code on the socket due
> a transmit packet discard.
I guess sctp_packet_transmit_chunk's return value is used for
'status' (like PMTU_FULL,RWND_FUL... ), that's why err was
put into sk->sk_err.   This err is supposed to be checked in
sctp_sendmsg, but there sctp_error check sk->sk_err only when
err == -EPIPE.
yes, we need to fix this, thanks.