Fix memory under ping flood #2257
Conversation
EvgeniiMekhanik
requested review from
const-t and
krizhanovsky
and removed request for
krizhanovsky
EvgeniiMekhanik
force-pushed
the
MekhanikEvgenii/fix-2149
branch
from
0c01528 to
9ba0c85
Compare
EvgeniiMekhanik
force-pushed
the
MekhanikEvgenii/fix-2149
branch
from
9ba0c85 to
bb894ad
Compare
const-t
left a comment
const-t
left a comment
There was a problem hiding this comment.
Logic looks correct, but I must notice TFW_CONN_PROCESS_RESULT is confusing and does implicit return. Maybe we would rewrite it?
|
It seems the only test we have for this is tempesta-tech/tempesta-test#615 , which tests other HTTP/2 floods. So it seems we're going to merge this with no any tests. @RomanBelozerov @EvgeniiMekhanik can we enable at least part of the test PR to test this memory leak? @EvgeniiMekhanik could you please also check if the issue #1940 (comment) still exists and update the #1940 status for this fix. |
krizhanovsky
left a comment
krizhanovsky
left a comment
There was a problem hiding this comment.
In general, looks good, but I ask @ai-tmpst to review this as well since the code isn't trivial and still could have non-obvious problems
fw/connection.h
Outdated
| * responses to client. We should continue to process \ | ||
| * WINDOW_UPDATE frames so, we should decrypt all skbs, \ | ||
| * not drop them. \ | ||
| */ \ |
There was a problem hiding this comment.
the comment is misaligned
fw/connection.h
Outdated
| if (extra_cond_to_out) { \ | ||
| kfree_skb(nskb); \ | ||
| goto out; \ | ||
| } \ |
There was a problem hiding this comment.
This condition is used only in http_frame.c
fw/http_frame.c
Outdated
| * and can immediately close connection. | ||
| */ | ||
| TFW_CONN_PROCESS_RESULT(r, was_stopped, | ||
| !(TFW_CONN_TYPE(c) & Conn_Stop), nskb); |
There was a problem hiding this comment.
The tfw_http_msg_process_generic() and TFW_CONN_PROCESS_RESULT() are identical, so instead of the macro we can just introduce a function to call tfw_http_msg_process_generic() and process the result
fw/connection.h
Outdated
| } else if (save_err_code != T_OK) { \ | ||
| r = save_err_code; \ | ||
| } \ | ||
| } while(0) |
There was a problem hiding this comment.
The macro isn't really generic and treats result of different function in different way. We can reduce copy & paste in http_frame.c and I'm not sure about keeping such a macro just for 2, still bit different, code places. Personally I'd prefer just to reference the each other code in comment.
EvgeniiMekhanik
force-pushed
the
MekhanikEvgenii/fix-2149
branch
3 times, most recently
from
0651a08 to
8c9d48a
Compare
| return (TFW_CONN_TYPE(conn) & Conn_Stop) && | ||
| (!((TFW_CONN_TYPE(conn) & Conn_H2Clnt) == Conn_H2Clnt)); | ||
| return (tfw_connection_was_stopped(conn) | ||
| && (!((TFW_CONN_TYPE(conn) & Conn_H2Clnt) == Conn_H2Clnt))); |
There was a problem hiding this comment.
!((TFW_CONN_TYPE(conn) & Conn_H2Clnt) == Conn_H2Clnt)
Couldn't it be reduced to !((TFW_CONN_TYPE(conn) & Conn_H2Clnt)?
There was a problem hiding this comment.
No because Conn_H2Clnt = 2 | TFW_FSM_HTTPS, so !((TFW_CONN_TYPE(conn) & Conn_H2Clnt) is false for H2 connections but it also false for https connection. But we should not process anything if error occurs in https connection. For http2 connection we should do it because we should process window_update frames from the client to have ability to send error response with body
There was a problem hiding this comment.
Oh, I see, it's not a one-bit flag. Sorry, I should look at it by myself.
EvgeniiMekhanik
force-pushed
the
MekhanikEvgenii/fix-2149
branch
from
8c9d48a to
f393ea3
Compare
There was a memory leak if `tfw_connection_recv` fails with T_BAD in `tfw_tls_connection_recv - because we continue to process skbs in this case but don't call `tfw_connection_recv` and do not free skbs. But if error occurs we should decrypt skbs and pass it to `tfw_connection_recv` to process WINDOW_UPDATE frames (it is necessary to send custom error responses with long body). Also fix some replated problems: - we should check Conn_Shutdown bit in all places where we check Conn_Stop bit to prevent processing any other frames. - we should continue to process frames if `tfw_http_msg_process_generic` fails same as we do it in tls.c. - add ALLOW_ERROR_INJECTION marso to check fixes. Closes #2149 #2117
- Split `TFW_CONN_PROCESS_RESULT` into two part first part is used in tls.c. Second part is a new macro `TFW_H2_CONN_PROCESS_RESULT` is defined in `tfw_h2_frame_process` and used only in this function. - Add WARN_ON_ONCE to check that `next` pointer is always zero for HTTP2 requests after calling `tfw_http_msg_process_generic` (see also existing appropriate warning in `tfw_http_req_process`. - Align `streams_num_exceeded` statistic.
EvgeniiMekhanik
force-pushed
the
MekhanikEvgenii/fix-2149
branch
from
f393ea3 to
717cbd0
Compare
There was a memory leak if
tfw_connection_recvfails with T_BAD in
tfw_tls_connection_recv - because we continue to process skbs in this case but don't calltfw_connection_recvand do not free skbs. But if error occurs we should decrypt skbs and pass it totfw_connection_recv` to process WINDOW_UPDATE frames (it is necessary to sendcustom error responses with long body).
Also fix some replated problems:
tfw_http_msg_process_genericfails same as we do it in tls.c.Closes #2149 #2117