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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 13 Aug 2016 15:47:46 +0800
From:	Xin Long <lucien.xin@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	Vladislav Yasevich <vyasevich@...il.com>, daniel@...earbox.net
Subject: Re: [PATCH net] sctp: fix a success return may hide an error

>
> This style of error handling is dangerous.  The first error can be
> lost.
>
> For example, if sctp_outq_flush_rtx() earlier in this function returns
> an error, it will be lost if any invocation of the function
> sctp_packet_transmit() at the end function signals an error.
>
> I think you should always preserve the first error that is recorded
> into 'error'.
>
> I also wonder about why sctp_outq_flush_rtx() errors are completely
> ignored and don't influence the control flow here in any way.

Yes, the first error can be lost.
Here we just keep the last error. We don't really have to return the
first error or return it on the first failure.

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

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