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: <063D6719AE5E284EB5DD2968C1650D6DB00DFD69@AcuExch.aculab.com>
Date:	Tue, 16 Aug 2016 09:16:52 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Xin Long' <lucien.xin@...il.com>,
	David Miller <davem@...emloft.net>
CC:	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

From: Xin Long
> Sent: 13 August 2016 08:48
> >
> > 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.

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.

Also after one error is it actually worth trying to send anything else
at all? ISTM that the code should either:
1) wait for resources and retry.
2) discard the entire queue (freeing resource) and hope the protocol
   timers will recover.

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

	David