Skip to content
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

Jump to bottom

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 2 commits into from
+18 −5

Conversation

kingluo
Copy link
Contributor

@kingluo kingluo commented Jun 26, 2024 •
edited
Loading

close #2117
close #2149

The leak only occurs when si_wq is full and continues to process the current skb (which may contain the remaining SSL record), sk->sk_receive_queue, and possibly skbs that come in later. The leak is never triggered when we reset the connection and stop processing data immediately. Such a fix would be reasonable even without the leak since it is unlikely that si_wq will become full without flooding.

@const-t
Copy link
Contributor

const-t commented Jul 5, 2024

In general I don't have any corrections, however I have few suggestions:

  1. I would like to suggest to do connection reset on failed tfw_connection_send() for each protocol, not only for http2.
  2. Let's replace DBG message with warning in case of failed pushing to si_wq. It's pretty important event, where we disconnect the client and it's would be great to know about that. Maybe it worth to add statistics counter for this event.
  3. Maybe we should find exact place where we leaks on si_wq overflow, just to know, maybe it can be reproduced in another way that not known at this moment.

@krizhanovsky Please see this comment, we need to know your opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@const-t const-t Awaiting requested review from const-t

@krizhanovsky krizhanovsky Awaiting requested review from krizhanovsky

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memory leak found in ping flood Tls errors under ping flood
2 participants
@kingluo @const-t