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_dfc7kmnCP0XgThF9oHX-2tVFNjN1Q-kobqT8WMRv7PqQ@mail.gmail.com>
Date:	Wed, 17 Aug 2016 19:42:37 +0800
From:	Xin Long <lucien.xin@...il.com>
To:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:	David Laight <David.Laight@...lab.com>,
	David Miller <davem@...emloft.net>,
	network dev <netdev@...r.kernel.org>,
	"linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
	Vladislav Yasevich <vyasevich@...il.com>,
	"daniel@...earbox.net" <daniel@...earbox.net>
Subject: Re: [PATCH net] sctp: fix a success return may hide an error

>> > If  letting the application see ENOMEM errors, and sctp has to drop this
>> > chunk, instead of retransmiting the ENOMEM chunk, but the ENOMEM
>> > chunk may not be the chunk from current msg, as it flush all the queue.
>> > even if users get an ENOMEM error, they may re-send a chunk that is same
>> > with the one that is still in retransmit queue.
>>
>> Yep, one more reason to handle those internally when safe.
I just checked tcp_sendmsg, it doesn't return any transmit error to user,
*NOT ONLY* ENOMEM.  you can check __tcp_push_pending_frames
and tcp_push, their return type is even void. although it may get
err from sk->sk_err:
    err = sk_stream_error(sk, flags, err);
But I didn't see it put any err into sk->sk_err in the main transmit
path.

yes, tcp_write_xmit has return value, as well as tcp_transmit_skb and
err = icsk->icsk_af_ops->queue_xmit(sk, skb, &inet->cork.fl). but
all of them are just used for internal, never return to userspace

In tcp_write_xmit, it even uses "unlikely':
if (unlikely(tcp_transmit_skb(sk, skb, 1, gfp)))
       break;


>
> Xin, maybe you can squash this patch and this ENOMEM handling? I'm
> thinking that handling ENOMEM may result in similar situations in other
> places, so we have a common reasoning on them.
>
So this reason does really matter, and not only for ENOMEM in transmit
path.