netdev - RE: [PATCH net] sctp: change to save MSG

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]

Date:   Fri, 3 Mar 2017 12:49:48 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Xin Long' <lucien.xin@...il.com>,
        network dev <netdev@...r.kernel.org>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Neil Horman <nhorman@...driver.com>,
        "Vlad Yasevich" <vyasevich@...il.com>
Subject: RE: [PATCH net] sctp: change to save MSG_MORE flag into assoc

From: Xin Long
> Sent: 03 March 2017 06:24
> David Laight noticed the support for MSG_MORE with datamsg->force_day
> didn't really work as we expected, as the first msg with MSG_MORE set
> would always block the following chunks' dequeuing.
> 
> This Patch is to rewrite it by saving the MSG_MORE flag into assoc as
> Divid Laight suggested.
   ^ typo

> asoc->force_delay is used to save MSG_MORE flag before a msg is sent.
> Once this msg is queued, asoc->force_delay is set back to 0, so that
> it will not affect other places flushing out queue.

That doesn't seem right nor make sense.

> asoc->force_delay works as a 'local param' here as the msg sending is
> under protection of sock lock.  It would make sctp's MSG_MORE work as
> tcp's.

It is much more important to get MSG_MORE working 'properly' for SCTP
than for TCP. For TCP an application can always use a long send.

...
> @@ -1982,6 +1982,7 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr *msg, size_t msg_len)
>  	 * breaks.
>  	 */
>  	err = sctp_primitive_SEND(net, asoc, datamsg);
> +	asoc->force_delay = 0;
>  	/* Did the lower layer accept the chunk? */
>  	if (err) {
>  		sctp_datamsg_free(datamsg);

I don't think this is right - or needed.
You only get to the above if some test has decided to send data chunks.
So it just means that the NEXT time someone tries to send data all the
queued data gets sent.
I'm guessing that the whole thing gets called in a loop (definitely needed
for very long data chunks, or after the window is opened).
Now if an application sends a lot of (say) 100 byte chunks with MSG_MORE
set it would expect to see a lot of full ethernet frames be sent.
With the above a frame will be sent (containing all but 1 chunk) when the
amount of queued data becomes too large for an ethernet frame, and immediately
followed by a second ethernet frame with 1 chunk in it.

Now it might be that the flag needs clearing when retransmissions are queued.
OTOH they might get sent for other reasons.

	David