Skip to content

fw/cache: Do not take an additional reference on paged sk_buff fragment while copying http/2 response body #1734

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
Merged
merged 1 commit into from
Nov 17, 2022
Merged

fw/cache: Do not take an additional reference on paged sk_buff fragment while copying http/2 response body #1734

merged 1 commit into from
Nov 17, 2022

Conversation

s0nx
Copy link
Contributor

@s0nx s0nx commented Oct 26, 2022

Signed-off-by: Petr Vyazovik xen@f-m.fm

@s0nx s0nx requested review from krizhanovsky and const-t October 26, 2022 20:34
@s0nx s0nx self-assigned this Oct 26, 2022
@s0nx s0nx linked an issue Oct 26, 2022 that may be closed by this pull request
OOM while running h2load when caching is enabled #1728
Closed
@avbelov23
Copy link
Contributor

Did I understand correctly that we are taking a reference in tfw_h2_append_predefined_body() and this is redundant?

krizhanovsky
krizhanovsky requested changes Nov 9, 2022
View reviewed changes
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

It seems there is an issue with the fix. @const-t review is definitely required, since the skb refcounting is quite a subtle topic.

@s0nx s0nx force-pushed the pv-1728-h2-cache-memleak branch 2 times, most recently from 8b6b784 to f194db6 Compare November 11, 2022 15:22
@s0nx
Copy link
Contributor Author

s0nx commented Nov 11, 2022

Taking additional reference on paged sk_buff fragment is not needed when building http/2 response body because each fragment is represented by newly allocated page with page->_refcount == 1. Extra page reference would result in memory leak during sk_buff freeing because of non-zero page->_refcount, so final __put_page() would not be called.

For http/1 responses we still need an additional page reference due to the fact that the page in question has actually been allocated previously by TDB (see tdb_file_open()). Missing an additional reference here would lead to TDB owned pages freeing by kfree_skb(), which in turn would cause memory corruption.

tfw_h2_append_predefined_body() is obviously called for http/2 responses only, so an additional reference has been deleted.

@s0nx s0nx requested a review from krizhanovsky November 11, 2022 15:30
krizhanovsky
krizhanovsky approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

LGTM

const-t
const-t approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@const-t const-t left a comment

Choose a reason for hiding this comment

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

LGTM

@s0nx s0nx closed this Nov 11, 2022
@s0nx s0nx reopened this Nov 11, 2022
@s0nx s0nx closed this Nov 14, 2022
@s0nx s0nx reopened this Nov 14, 2022
@s0nx s0nx closed this Nov 14, 2022
@s0nx s0nx reopened this Nov 14, 2022
@s0nx s0nx closed this Nov 14, 2022
@s0nx s0nx reopened this Nov 14, 2022
@s0nx
fw/cache: Do not take an additional reference on paged sk_buff fragment
cb4e298 
while copying http/2 response body

Taking additional reference on paged sk_buff fragment is not needed
when building http/2 response body because each fragment is represented
by newly allocated page with page->_refcount == 1. Extra page reference
would result in memory leak during sk_buff freeing because of non-zero
page->_refcount, so final __put_page() would not be called.

For http/1 responses we still need an additional page reference
due to the fact that the page in question has actually been allocated
previously by TDB (see tdb_file_open()). Missing an additional reference
here would lead to TDB owned pages freeing by kfree_skb(), which in turn
would cause memory corruption.

Signed-off-by: Petr Vyazovik <xen@f-m.fm>
@s0nx s0nx force-pushed the pv-1728-h2-cache-memleak branch from f194db6 to cb4e298 Compare November 17, 2022 15:44
@s0nx s0nx merged commit f1d65a5 into master Nov 17, 2022
@krizhanovsky krizhanovsky deleted the pv-1728-h2-cache-memleak branch July 2, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krizhanovsky krizhanovsky krizhanovsky approved these changes

@const-t const-t const-t approved these changes

Assignees

@s0nx s0nx

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OOM while running h2load when caching is enabled
4 participants
@s0nx @avbelov23 @krizhanovsky @const-t