Skip to content

fix(2117): if si_wq is full, reset connection in case of flooding #2150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kingluo wants to merge 4 commits into from
Closed

fix(2117): if si_wq is full, reset connection in case of flooding #2150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a215ad0
fix(2117): if si_wq is full, reset connection
kingluo Jun 26, 2024
c9c438d
use `T_WARN` if `-EBUSY`
kingluo Jul 24, 2024
57d025b
Update fw/sync_socket.h
kingluo Aug 15, 2024
71b8a2b
Merge branch 'master' into jinhua/fix-2117
kingluo Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion fw/http_frame.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,8 +324,12 @@ __tfw_h2_send_frame(TfwH2Ctx *ctx, TfwFrameHdr *hdr, TfwStr *data,
tfw_h2_on_tcp_entail_ack;
}

if ((r = tfw_connection_send((TfwConn *)conn, &msg)))
if ((r = tfw_connection_send((TfwConn *)conn, &msg))) {
/* si_wq is full, reset connection in case of flooding */
if (unlikely(r == -EBUSY))
r = T_BLOCK_WITH_RST;
goto err;
}
/*
* We do not close client connection automatically here in case
* of failed sending, the caller must make such decision instead;
Expand Down
8 changes: 6 additions & 2 deletions fw/sock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Synchronous Socket API.
*
* Copyright (C) 2014 NatSys Lab. (info@natsys-lab.com).
* Copyright (C) 2015-2023 Tempesta Technologies, Inc.
* Copyright (C) 2015-2024 Tempesta Technologies, Inc.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -569,7 +569,7 @@ ss_send(struct sock *sk, struct sk_buff **skb_head, int flags)
*/
sock_hold(sk);
if (ss_wq_push(&sw, cpu)) {
T_DBG2("Cannot schedule socket %p for transmission"
T_WARN("Cannot schedule socket %p for transmission"
" (queue size %d)\n", sk,
tfw_wq_size(&per_cpu(si_wq, cpu)));
sock_put(sk);
Expand Down Expand Up @@ -1003,6 +1003,9 @@ ss_tcp_data_ready(struct sock *sk)
return;
}

if (unlikely(SS_CONN_TYPE(sk) & Conn_Reset))
return;
Copy link
Copy Markdown
Contributor

@krizhanovsky krizhanovsky Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introducing Conn_Reset state, used only in one function seems like a workaround. I'd prefer a more clear solution. Seems this state is only needed to not to execute the skb processing while the socket is in the queue for closing, so don't we already have enough information about the socket state at the moment?

Copy link
Copy Markdown
Contributor Author

@kingluo kingluo Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a workaround, but a bugfix: In case of errors, we should exit the call chain and stop handling the involved TLS records, unrolled skb, sk->sk_receive_queue, and future sk_data_ready callbacks from the kernel (that's why we need to reset but not close the socket). Otherwise, it's an undefined behavior. And yes, we didn't cover the RST case (not close) in that function, that's exactly why I made changes here.

Copy link
Copy Markdown
Contributor

@krizhanovsky krizhanovsky Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new socket state used only in one function looks awkward.

Can we handle this with a real socket state? Maybe we can even avoid calling sk_data_ready by setting some socket state and/or doing partial close/reset?


if (skb_queue_empty(&sk->sk_receive_queue)) {
/*
* Check for URG data.
Expand All @@ -1028,6 +1031,7 @@ ss_tcp_data_ready(struct sock *sk)
case SS_BLOCK_WITH_RST:
flags = SS_F_ABORT_FORCE;
action = ss_close;
SS_CONN_TYPE(sk) |= Conn_Reset;
break;
case SS_BAD:
flags = SS_F_SYNC;
Expand Down
7 changes: 6 additions & 1 deletion fw/sync_socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Synchronous Socket API.
*
* Copyright (C) 2014 NatSys Lab. (info@natsys-lab.com).
* Copyright (C) 2015-2023 Tempesta Technologies, Inc.
* Copyright (C) 2015-2024 Tempesta Technologies, Inc.
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by
Expand Down Expand Up @@ -67,6 +67,11 @@ enum {
* and wait until ACK to our FIN is come.
*/
Conn_Closing = (0x3 << __Flag_Bits),
/*
* Connection is in special state: the socket is reset
* and we should not process sk->sk_receive_queue.
*/
Conn_Reset = (0x4 << __Flag_Bits),
};

typedef struct tfw_conn_t TfwConn;
Expand Down
2 changes: 1 addition & 1 deletion tls/ttls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2311,7 +2311,7 @@ ttls_recv(void *tls_data, unsigned char *buf, unsigned int len, unsigned int *re
TTLS_WARN(tls, "TLS cannot decrypt msg on state %s, ret=%d%s\n",
tls_state_to_str(tls->state), r,
r == -EBADMSG ? "(bad ciphertext)" : "");
return r;
return T_BLOCK_WITH_RST;
Copy link
Copy Markdown
Contributor

@krizhanovsky krizhanovsky Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we're going to block the client, not just reset it's connection. As noted in lib/log.h the return code for a security event, not for OOM, which might have different reasons.

Copy link
Copy Markdown
Contributor Author

@kingluo kingluo Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pure misleading in naming, all I want is sending RST, but we have only T_BLOCK_WITH_RST constant. Maybe we should bring in a dedicated constant for normal RST.

Copy link
Copy Markdown
Contributor

@krizhanovsky krizhanovsky Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if we need to reset connections, then we do need designated RST constant and appropriate workflow handling the return codes.

}

if (io->msgtype == TTLS_MSG_ALERT) {
Expand Down