| Message ID | 1349342067-27586-1-git-send-email-erik.hugne@ericsson.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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
> 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
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
> 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
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
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);