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

Set proper tcp_gso_segs for skb with TLS TAG. #2372

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EvgeniiMekhanik
Copy link
Contributor

Previosly we don't set tcp_gso_segs for skb with new added TLS TAG, because we assume that it will be set during tso,tcp}_fragment() from tcp_write_xmit. It is not correct, because skb with new added TLS TAG can be already produced during one of previous call of tso,tcp}_fragment(). In this case this skb has already set tcp_gso_segs. Later in the loop in tcp_write_xmit tcp_init_tso_segs skips such skbs if tcp_gso_segs > 1, but mss is same. Limit will be also greater then mss, because if tcp_gso_segs > 1, limit is recalculated. In this case we never set proper tcp_gso_segs (with taking into account new added TLS TAG), that leads to kernel BUG during call tcp_shifted_skb.
This BUG was hardly reproduced, because:

  • it is not a usual case when TLS TAG is added to the skb, which was produced during call tso,tcp}_fragment().
  • tcp_shifted_skb is called very rarely from tcp_shift_skb_data. This two conditions were reproduced only on mtu 80 under heavy load.

@EvgeniiMekhanik EvgeniiMekhanik requested review from const-t and krizhanovsky and removed request for const-t March 24, 2025 04:33
@krizhanovsky krizhanovsky removed their request for review March 24, 2025 16:45
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.

Seems it can happens only on loopback interface, am I right? Also I would like to ask, maybe we can move tcp_set_skb_tso_segs(skb, mss_now); from tcp_tfw_sk_write_xmit() to tfw_tls_encrypt() to do this in one place? Looks like this is only for cases where we have only one skb to encrypt, for other cases we do tcp_set_skb_tso_segs() in tfw_tls_encrypt().

@EvgeniiMekhanik
Copy link
Contributor Author

Not only for loopback interface. According to our private discussion tcp_write_xmit->tcp_event_new_data_sent add
skb to the tcp_rtx_queue. Then when ack received tcp_sacktag_walk is called and we iterate over all skbs in tcp_rtx_queue (skb_rbtree_walk_from(skb))

Previosly we don't set `tcp_gso_segs` for skb with new
added TLS TAG, because we assume that it will be set
during `tso,tcp}_fragment()` from `tcp_write_xmit`.
It is not correct, because skb with new added TLS TAG
can be already produced during one of previous call of
`tso,tcp}_fragment()`. In this case this skb has already
set `tcp_gso_segs`. Later in the loop in `tcp_write_xmit`
`tcp_init_tso_segs` skips such skbs if `tcp_gso_segs` > 1,
but mss is same. Limit will be also greater then mss,
because if `tcp_gso_segs` > 1, limit is recalculated. In
this case we never set proper `tcp_gso_segs` (with taking
into account new added TLS TAG), that leads to kernel BUG
during call `tcp_shifted_skb`.
This BUG was hardly reproduced, because:
- it is not a usual case when TLS TAG is added to the
  skb, which was produced during call `tso,tcp}_fragment()`.
- `tcp_shifted_skb` is called very rarely from
  `tcp_shift_skb_data`.
This two conditions were reproduced only on mtu 80 under heavy
load.
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the MekhanikEvgenii/fix-2059 branch from bb4d001 to e4c7ca1 Compare March 29, 2025 02:19
@EvgeniiMekhanik
Copy link
Contributor Author

No this is a case when tail_skb != skb and there is no one skb for encryption, because if there is only one skb for encryption we call tcp_set_skb_tso_segs for it in tcp_tfw_sk_write_xmit

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

Successfully merging this pull request may close these issues.

2 participants