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.

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

same skb is added to the msg's skb_list multiple times in ss_tcp_process_skb() #31

Closed
vdmit11 opened this issue Nov 17, 2014 · 5 comments
Assignees
Labels

Comments

@vdmit11
Copy link
Contributor

vdmit11 commented Nov 17, 2014

The ss_tcp_process_skb()/ss_tcp_process_proto_skb() iterate over sk_buff's fragments and make several upcalls for each fragment.

There are two problems:

  1. put_skb_to_msg() is called for each fragment. The function enqueues the given skb to the msg's skb_list, so the same skb is actually added to the list multiple times (for each fragment of the skb).
  2. On SS_POSTPONE, the postpone_skb() is called after put_skb_to_msg(). Both functions add the skb to the msg's skb_list, so the same skb is added twice when the message is postponed.
@vdmit11 vdmit11 changed the title same skb is added to the msgskb_list multiple times in same skb is added to the msg's skb_list multiple times in case of SS_POSTPONE Nov 17, 2014
@vdmit11 vdmit11 changed the title same skb is added to the msg's skb_list multiple times in case of SS_POSTPONE same skb is added to the msg's skb_list multiple times in ss_tcp_process_skb() Nov 17, 2014
@vdmit11 vdmit11 added the bug label Nov 17, 2014
@vdmit11 vdmit11 assigned vdmit11 and krizhanovsky and unassigned vdmit11 Nov 17, 2014
@keshonok keshonok assigned keshonok and unassigned krizhanovsky Feb 11, 2015
@keshonok
Copy link
Contributor

Eventually, ss_skb_queue_tail() is called in all cases, and it has protection from a double insertion. If an SKB in on the list already, then it's not put there again.

This sort of design might be questionable, but that's a totally different topic. The bug does not exist as described.

@krizhanovsky
Copy link
Contributor

This is poor design of put_skb_to_msg(), must be fixed. There is no sense to call the hook many times just to see that the skb is already in the list.

@keshonok
Copy link
Contributor

It appears that the fix is not complete. When the message is postponed - and that the usual course of action as an skb is processed in chunks - tfw_connection_postpone_skb() attempts to add the skb to the message's skb_list again. Always.

@keshonok keshonok reopened this Mar 15, 2015
@krizhanovsky
Copy link
Contributor

Ok, please do the fix.

@keshonok
Copy link
Contributor

The rest of this issue is moved to #80.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants