| Message ID | 20201030180225.11089-8-rohitm@chelsio.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | cxgb4/ch_ktls: Fixes in nic tls code | expand |
On Fri, 30 Oct 2020 23:32:22 +0530 Rohit Maheshwari wrote: > There could be a case where ACK for tls exchanges prior to start > marker is missed out, and by the time tls is offloaded. This pkt > should not be discarded and handled carefully. It could be > plaintext alone or plaintext + finish as well. By plaintext + finish you mean the start of offload falls in the middle of a TCP skb? That should never happen. We force EOR when we turn on TLS, so you should never see a TCP skb that needs to be half-encrypted.
On 04/11/20 2:21 AM, Jakub Kicinski wrote: > On Fri, 30 Oct 2020 23:32:22 +0530 Rohit Maheshwari wrote: >> There could be a case where ACK for tls exchanges prior to start >> marker is missed out, and by the time tls is offloaded. This pkt >> should not be discarded and handled carefully. It could be >> plaintext alone or plaintext + finish as well. > By plaintext + finish you mean the start of offload falls in the middle > of a TCP skb? That should never happen. We force EOR when we turn on > TLS, so you should never see a TCP skb that needs to be half-encrypted. This happens when re-transmission is issued on a high load system. First time CCS is and finished message comes to driver one by one. Problem is, if ACK is not received for both these packets, while sending for re-transmission, stack sends both these together. Now the start sequence number will be before the start marker record, but it also holds data for encryption. This is handled in this patch. Are you saying this should not happen?
On Wed, 4 Nov 2020 22:48:22 +0530 rohit maheshwari wrote: > On 04/11/20 2:21 AM, Jakub Kicinski wrote: > > On Fri, 30 Oct 2020 23:32:22 +0530 Rohit Maheshwari wrote: > >> There could be a case where ACK for tls exchanges prior to start > >> marker is missed out, and by the time tls is offloaded. This pkt > >> should not be discarded and handled carefully. It could be > >> plaintext alone or plaintext + finish as well. > > By plaintext + finish you mean the start of offload falls in the middle > > of a TCP skb? That should never happen. We force EOR when we turn on > > TLS, so you should never see a TCP skb that needs to be half-encrypted. > This happens when re-transmission is issued on a high load system. > First time CCS is and finished message comes to driver one by one. > Problem is, if ACK is not received for both these packets, while > sending for re-transmission, stack sends both these together. Now > the start sequence number will be before the start marker record, > but it also holds data for encryption. This is handled in this > patch. > Are you saying this should not happen? Maybe Eric could help us out - Rohit says that the TCP stack is generating skbs which straddle MSG_EOR on re-transmit. Can this happen / is it correct?
diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c index 2c92ded79b49..06a22813f466 100644 --- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c +++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c @@ -1828,12 +1828,6 @@ static int chcr_ktls_xmit(struct sk_buff *skb, struct net_device *dev) goto out; } - if (unlikely(tls_record_is_start_marker(record))) { - spin_unlock_irqrestore(&tx_ctx->base.lock, flags); - atomic64_inc(&port_stats->ktls_tx_skip_no_sync_data); - goto out; - } - tls_end_offset = record->end_seq - tcp_seq; pr_debug("seq %#x, start %#x end %#x prev %#x, datalen %d offset %d\n", @@ -1877,6 +1871,36 @@ static int chcr_ktls_xmit(struct sk_buff *skb, struct net_device *dev) skb_get(skb); } + if (unlikely(tls_record_is_start_marker(record))) { + atomic64_inc(&port_stats->ktls_tx_skip_no_sync_data); + /* If tls_end_offset < data_len, means there is some + * data after start marker, which needs encryption, send + * plaintext first and take skb refcount. else send out + * complete pkt as plaintext. + */ + if (tls_end_offset < data_len) + skb_get(skb); + else + tls_end_offset = data_len; + + ret = chcr_ktls_tx_plaintxt(tx_info, skb, tcp_seq, mss, + (!th->fin && th->psh), q, + tls_end_offset, skb_offset); + if (ret) { + /* free the refcount taken earlier */ + if (tls_end_offset < data_len) + dev_kfree_skb_any(skb); + spin_unlock_irqrestore(&tx_ctx->base.lock, + flags); + goto out; + } + + data_len -= tls_end_offset; + tcp_seq = record->end_seq; + skb_offset += tls_end_offset; + continue; + } + /* if a tls record is finishing in this SKB */ if (tls_end_offset <= data_len) { ret = chcr_end_part_handler(tx_info, skb, record,
There could be a case where ACK for tls exchanges prior to start marker is missed out, and by the time tls is offloaded. This pkt should not be discarded and handled carefully. It could be plaintext alone or plaintext + finish as well. Fixes: 5a4b9fe7fece ("cxgb4/chcr: complete record tx handling") Signed-off-by: Rohit Maheshwari <rohitm@chelsio.com> --- .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-)