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

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]

Message-ID: <CADvbK_fsH9RAc_r6SaTKYTUJusGcZzPuCXX3zntH7UhFFWq-Cg@mail.gmail.com>
Date:	Tue, 16 Aug 2016 19:34:18 +0800
From:	Xin Long <lucien.xin@...il.com>
To:	David Laight <David.Laight@...lab.com>
Cc:	David Miller <davem@...emloft.net>,
	network dev <netdev@...r.kernel.org>,
	"linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	Vladislav Yasevich <vyasevich@...il.com>,
	"daniel@...earbox.net" <daniel@...earbox.net>
Subject: Re: [PATCH net] sctp: fix a success return may hide an error

>>
>> [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.