Patchwork tipc: flow control should not account for sk_rcvbuf

login
register
mail settings
Submitter erik.hugne@ericsson.com
Date Oct. 4, 2012, 9:14 a.m.
Message ID <1349342067-27586-1-git-send-email-erik.hugne@ericsson.com>
Download mbox | patch
Permalink /patch/189071/
State Superseded
Delegated to: David Miller
Headers show

Comments

erik.hugne@ericsson.com - Oct. 4, 2012, 9:14 a.m.
From: Erik Hugne <erik.hugne@ericsson.com>

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 <erik.hugne@ericsson.com>
---
 net/tipc/socket.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
Eric Dumazet - Oct. 4, 2012, 9:21 a.m.
On Thu, 2012-10-04 at 11:14 +0200, erik.hugne@ericsson.com wrote:
> From: Erik Hugne <erik.hugne@ericsson.com>
> 
> 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 <erik.hugne@ericsson.com>
> ---
>  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);
>  


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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
erik.hugne@ericsson.com - Oct. 4, 2012, 9:59 a.m.
> 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Oct. 4, 2012, 10:26 a.m.
On Thu, 2012-10-04 at 11:59 +0200, Erik Hugne 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.
> >
> 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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
erik.hugne@ericsson.com - Oct. 4, 2012, 12:12 p.m.
> 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?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Oct. 4, 2012, 1:55 p.m.
On Thu, 2012-10-04 at 14:12 +0200, Erik Hugne wrote:
> > 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)



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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);