[PATCH] tipc: flow control should not account for sk_rcvbuf

From: Erik Hugne <redacted>

The TIPC flow control is design around message count, and it should not
account for the sk_rcvbuf when enqueueing messages to the socket
receive queue.

This fixes a problem when the sk_add_backlog fails due to this check
and TIPC_ERR_OVERLOAD is reported back to the sender.
The sender would then drop it's side of the connection only, leaving
a stale connection on the other end.

Signed-off-by: Erik Hugne <redacted>
---
 net/tipc/socket.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 09dc5b9..02fed90 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c

@@ -1269,10 +1269,8 @@ static u32 dispatch(struct tipc_port *tport, struct sk_buff *buf)
 	if (!sock_owned_by_user(sk)) {
 		res = filter_rcv(sk, buf);
 	} else {
-		if (sk_add_backlog(sk, buf, sk->sk_rcvbuf))
-			res = TIPC_ERR_OVERLOAD;
-		else
-			res = TIPC_OK;
+		__sk_add_backlog(sk, buf);
+		res = TIPC_OK;
 	}
 	bh_unlock_sock(sk);
 

-- 
1.7.5.4

On Thu, 2012-10-04 at 11:14 +0200, erik.hugne@ericsson.com wrote:

What guarantee do we have this cannot use all kernel memory ?

If sk->sk_rcvbuf is not an acceptable limit here, you must use a
different limit, but not infinity.

What guarantee do we have this cannot use all kernel memory ?

If sk->sk_rcvbuf is not an acceptable limit here, you must use a
different limit, but not infinity.

There is an implicit limit on how much data that can be buffered on each socket, controlled by TIPC_FLOW_CONTROL_WIN.

This limit is:
TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE

//E

On Thu, 2012-10-04 at 11:59 +0200, Erik Hugne wrote:

quoted

What guarantee do we have this cannot use all kernel memory ?

If sk->sk_rcvbuf is not an acceptable limit here, you must use a
different limit, but not infinity.

There is an implicit limit on how much data that can be buffered on each 
socket, controlled by TIPC_FLOW_CONTROL_WIN.

This limit is:
TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE

And this limit is tested _before_ queueing to backlog if socket is owned
by the user ?

You'll have to demonstrate this in the changelog.

Again, I dont think this patch is safe, we need an explicit limit.

And this limit is tested _before_ queueing to backlog if socket is owned
by the user ?

You'll have to demonstrate this in the changelog.

Again, I dont think this patch is safe, we need an explicit limit.

You're right Eric..

Another way of solving it is to increase the default sk_rcvbuf size to
(TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE)
at socket creation.

Do you think that would be acceptable?

On Thu, 2012-10-04 at 14:12 +0200, Erik Hugne wrote:

quoted

And this limit is tested _before_ queueing to backlog if socket is owned
by the user ?

You'll have to demonstrate this in the changelog.

Again, I dont think this patch is safe, we need an explicit limit.

You're right Eric..

Another way of solving it is to increase the default sk_rcvbuf size to
(TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE)
at socket creation.

Do you think that would be acceptable?

If its a tipc constant, you also could use

if (sk_add_backlog(sk, buf,
                   TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE))

no ?

But yes, a protocol is allowed to change sk_rcvbuf value (its done for
TCP for example, with a limit to tcp_rmem[2] (between 4 and 6 Mbytes)

From: Erik Hugne <redacted>

When large buffers are sent over connected TIPC sockets, it
is likely that the sk_backlog will be filled up on the
receiver side, but the TIPC flow control mechanism is happily
unaware of this since that is based on message count.

The sender will receive a TIPC_ERR_OVERLOAD message when this occurs
and drop it's side of the connection, leaving it stale on
the receiver end.

By increasing the sk_rcvbuf to a 'worst case' value, we avoid the
overload caused by a full backlog queue and the flow control
will work properly.

This worst case value is the max TIPC message size times
the flow control window, multiplied by two because a sender
will transmit up to double the window size before a port is marked
congested.
We multiply this by 2 to account for the sk_buff and other overheads.

Signed-off-by: Erik Hugne <redacted>
---
 net/tipc/socket.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 09dc5b9..fd5f042 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c

@@ -220,6 +220,7 @@ static int tipc_create(struct net *net, struct socket *sock, int protocol,
 
 	sock_init_data(sock, sk);
 	sk->sk_backlog_rcv = backlog_rcv;
+	sk->sk_rcvbuf = TIPC_FLOW_CONTROL_WIN * 2 * TIPC_MAX_USER_MSG_SIZE * 2;
 	tipc_sk(sk)->p = tp_ptr;
 	tipc_sk(sk)->conn_timeout = CONN_TIMEOUT_DEFAULT;
 

-- 
1.7.5.4

From: <redacted>
Date: Thu, 4 Oct 2012 17:00:43 +0200

From: Erik Hugne <redacted>

When large buffers are sent over connected TIPC sockets, it
is likely that the sk_backlog will be filled up on the
receiver side, but the TIPC flow control mechanism is happily
unaware of this since that is based on message count.

The sender will receive a TIPC_ERR_OVERLOAD message when this occurs
and drop it's side of the connection, leaving it stale on
the receiver end.

By increasing the sk_rcvbuf to a 'worst case' value, we avoid the
overload caused by a full backlog queue and the flow control
will work properly.

This worst case value is the max TIPC message size times
the flow control window, multiplied by two because a sender
will transmit up to double the window size before a port is marked
congested.
We multiply this by 2 to account for the sk_buff and other overheads.

Signed-off-by: Erik Hugne <redacted>

Applied.