tls_device_decrypted() reports the TCP sequence of the record it is classifying as tcp_sk(sk)->copied_seq - rxm->full_len. The original strparser read every record out of the TCP receive queue before the TLS layer looked at it, so copied_seq had already moved past the record. The TLS-specific strparser leaves the record in the TCP receive queue in its normal mode and only calls tcp_read_done() from tls_strp_msg_consume(), after decryption. When the tracepoint fires, copied_seq still points at the first byte of the record, and the reported tcp_seq is one record length too low. In copy mode (e.g. when the receive queue is short under memory pressure or with a collapsed receive window, or the record spans overlapping or mixed decrypted/undecrypted skbs), tcp_read_sock() has already advanced copied_seq past the record. It can also consume a FIN that ends the record's last skb, which adds one byte of sequence space absent from the record length, so subtracting the length reports a start one byte too high. copied_seq cannot tell whether a FIN was consumed, so the start must be saved before the read. Record the TCP sequence in the strparser whenever it starts parsing a new record, before anything can switch it to copy mode, and report that via a new tls_strp_msg_seq() helper. The value is then valid in both modes without having to track which path entered copy mode. No functional change beyond the tracepoint argument. Found by code inspection while working on TLS 1.3 hardware offload support. Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser") Signed-off-by: Rishikesh Jethwani --- include/net/tls.h | 2 ++ net/tls/tls.h | 6 ++++++ net/tls/tls_device.c | 2 +- net/tls/tls_strp.c | 3 +++ 4 files changed, 12 insertions(+), 1 deletion(-) diff --git a/include/net/tls.h b/include/net/tls.h index e57bef58851e..e1e48a25cafa 100644 --- a/include/net/tls.h +++ b/include/net/tls.h @@ -123,6 +123,8 @@ struct tls_strparser { bool msg_ready; struct strp_msg stm; + /* TCP sequence of the first byte of the current record. */ + u32 seq; struct sk_buff *anchor; struct work_struct work; diff --git a/net/tls/tls.h b/net/tls/tls.h index 60a37bdaaa25..a9f7fd79c69e 100644 --- a/net/tls/tls.h +++ b/net/tls/tls.h @@ -227,6 +227,12 @@ static inline bool tls_strp_msg_mixed_decrypted(struct tls_sw_context_rx *ctx) return ctx->strp.mixed_decrypted; } +static inline u32 tls_strp_msg_seq(struct tls_sw_context_rx *ctx) +{ + DEBUG_NET_WARN_ON_ONCE(!ctx->strp.msg_ready); + return ctx->strp.seq; +} + #ifdef CONFIG_TLS_DEVICE int tls_device_init(void); void tls_device_cleanup(void); diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index f11d0528fc43..fbfe6ec3df04 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -990,7 +990,7 @@ int tls_device_decrypted(struct sock *sk, struct tls_context *tls_ctx) is_encrypted = 0; } - trace_tls_device_decrypted(sk, tcp_sk(sk)->copied_seq - rxm->full_len, + trace_tls_device_decrypted(sk, tls_strp_msg_seq(sw_ctx), tls_ctx->rx.rec_seq, rxm->full_len, is_encrypted, is_decrypted); diff --git a/net/tls/tls_strp.c b/net/tls/tls_strp.c index 6cc222008d95..5d5e8c3fd5ad 100644 --- a/net/tls/tls_strp.c +++ b/net/tls/tls_strp.c @@ -526,6 +526,9 @@ static int tls_strp_read_sock(struct tls_strparser *strp) tls_strp_load_anchor_with_queue(strp, inq); if (!strp->stm.full_len) { + /* Save the record start before copy mode advances copied_seq. */ + strp->seq = tcp_sk(strp->sk)->copied_seq; + if (inq < TLS_HEADER_SIZE) return tls_strp_read_copy(strp, true); if (!tls_strp_check_queue_ok(strp, TLS_HEADER_SIZE)) -- 2.50.1 tls_device_core_ctrl_rx_resync() asks the device to resync at the next record, passing rec_seq + 1 together with tcp_sk(sk)->copied_seq as that record's TCP sequence. With the original strparser the current record had already been read out of the TCP receive queue, so copied_seq was the start of the next record. The TLS-specific strparser leaves the record queued in its normal mode until tls_strp_msg_consume(), so copied_seq is still the start of the current record. The resync branch is taken when tcp_inq() does not exceed the record length, i.e. typically when the current record is the only data queued, and the device is told that record rec_seq + 1 begins where record rec_seq begins. This can prevent hardware resynchronization and keep subsequent records on the software decryption path. Further resync attempts are made with backoff. In copy mode copied_seq is already past the record, so the sequence is correct there; the only exception is a FIN consumed along with the record, after which no further record can follow. Use the record start from tls_strp_msg_seq() plus the record length instead. That is the start of the next record in both modes, so the reported sequence no longer depends on which mode the strparser is in. Only drivers using TLS_OFFLOAD_SYNC_TYPE_CORE_NEXT_HINT are affected. Found by code inspection while working on TLS 1.3 hardware offload support. Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser") Signed-off-by: Rishikesh Jethwani --- net/tls/tls_device.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index fbfe6ec3df04..a6c4d0cc54c7 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -839,6 +839,7 @@ static void tls_device_core_ctrl_rx_resync(struct tls_context *tls_ctx, struct tls_offload_context_rx *ctx, struct sock *sk, struct sk_buff *skb) { + struct tls_sw_context_rx *sw_ctx = tls_sw_ctx_rx(tls_ctx); struct strp_msg *rxm; /* device will request resyncs by itself based on stream scan */ @@ -877,8 +878,9 @@ static void tls_device_core_ctrl_rx_resync(struct tls_context *tls_ctx, memcpy(rcd_sn, tls_ctx->rx.rec_seq, prot->rec_seq_size); tls_bigint_increment(rcd_sn, prot->rec_seq_size); - tls_device_resync_rx(tls_ctx, sk, tcp_sk(sk)->copied_seq, - rcd_sn); + tls_device_resync_rx(tls_ctx, sk, + tls_strp_msg_seq(sw_ctx) + + rxm->full_len, rcd_sn); } } -- 2.50.1 tls_rx_msg_size() tells the device layer where each new record starts, via tls_device_rx_resync_new_rec(), as TCP_SKB_CB(skb)->seq + strp->stm.offset. skb is the strparser anchor, whose cb is copied from the first queued skb by tls_strp_load_anchor_with_queue(), and stm.offset is where the record starts within that skb. When the header cannot be parsed in place, tls_strp_read_copy() moves the strparser to copy mode and resets stm.offset to 0, but the anchor keeps the first skb's cb. The header is then parsed from the copied data, and the reported start is the first byte of that skb rather than of the record. If the record does not begin at an skb boundary, the sequence is too low by the record's offset into the skb. Every resync type compares or advances that sequence. With TLS_OFFLOAD_SYNC_TYPE_DRIVER_REQ and DRIVER_REQ_ASYNC the device's request no longer matches and the resync is missed until a later record. With TLS_OFFLOAD_SYNC_TYPE_CORE_NEXT_HINT the device is given a next-record boundary that is off by the same amount. The header is parsed in copy mode when fewer bytes than the TLS header plus explicit IV, where applicable, are queued under memory pressure or with a collapsed receive window, or when the header straddles skbs that overlap or differ in decrypted status. The latter is most likely right around a loss of device sync, when resync is needed. Commit 9466ef3ec972 ("tls: fix RX desync on overlapping skbs") added that queue-consistency check before the header is parsed; before it only a short queue led here. The strparser now records the start of each record before it can enter copy mode, so report strp->seq instead. Found by code inspection while working on TLS 1.3 hardware offload support. Fixes: 84c61fe1a75b ("tls: rx: do not use the standard strparser") Signed-off-by: Rishikesh Jethwani --- net/tls/tls_sw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index d1ad31986cf2..5db4a6897b29 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -2232,7 +2232,7 @@ int tls_rx_msg_size(struct tls_strparser *strp, struct sk_buff *skb) } tls_device_rx_resync_new_rec(strp->sk, data_len + TLS_HEADER_SIZE, - TCP_SKB_CB(skb)->seq + strp->stm.offset); + strp->seq); return data_len + TLS_HEADER_SIZE; read_failure: -- 2.50.1 TLS_OFFLOAD_SYNC_TYPE_CORE_NEXT_HINT resync tells the device where the next record starts. That is only useful if the device has not yet seen the next record's header, so both places that send the hint first check whether any of the next record is already queued on the socket, using tcp_inq(). tcp_inq() counts from copied_seq, so the checks are only right if they know how much of the current record has been consumed. tls_device_core_ctrl_rx_resync() runs when a record is entirely encrypted and checks tcp_inq() > rxm->full_len. With the original strparser the record had already been read out, so this asked for more than a whole record beyond the current one instead of any data at all, contrary to the intent stated in the commit that added it. Since the TLS-specific strparser the check is correct in its normal mode, where the record is still queued, but copy mode has consumed the record and keeps the original error. The hint can then be sent immediately for a boundary the device has already passed. tls_device_rx_resync_new_rec() runs when a new record header is parsed and checks tcp_inq() > rcd_len, relying on the queue still holding the record being parsed. That does not hold once part of the record has been consumed: in copy mode, and with the original strparser, which also consumed incomplete headers before parsing them. The check then underestimates what is queued and can send the hint after the next header has arrived, too late for the device to use it. In both cases the attempt is wasted and the next try is made only after backoff, keeping the stream on software decryption longer. Count from the record start instead of copied_seq. With the record start now tracked by the strparser in both modes, rcv_nxt minus that start is the number of bytes received for this record and beyond, regardless of how much has been consumed. The tls_device_rx_resync_nh_delay tracepoint now reports received sequence space from the record start, including data already consumed in copy mode. A FIN right after the record now counts as queued data, which only defers a hint that could not be used, since no record can follow it. Only drivers using TLS_OFFLOAD_SYNC_TYPE_CORE_NEXT_HINT are affected. Found by code inspection while working on TLS 1.3 hardware offload support. Fixes: f953d33ba122 ("net/tls: add kernel-driven TLS RX resync") Signed-off-by: Rishikesh Jethwani --- net/tls/tls_device.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index a6c4d0cc54c7..8181564600c1 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -805,10 +805,10 @@ void tls_device_rx_resync_new_rec(struct sock *sk, u32 rcd_len, u32 seq) if (likely(!rx_ctx->resync_nh_do_now)) return; - /* head of next rec is already in, note that the sock_inq will - * include the currently parsed message when called from parser + /* head of next rec is already in, count from the record start + * since copy mode may have consumed part of it already */ - sock_data = tcp_inq(sk); + sock_data = READ_ONCE(tcp_sk(sk)->rcv_nxt) - seq; if (sock_data > rcd_len) { trace_tls_device_rx_resync_nh_delay(sk, sock_data, rcd_len); @@ -841,6 +841,7 @@ static void tls_device_core_ctrl_rx_resync(struct tls_context *tls_ctx, { struct tls_sw_context_rx *sw_ctx = tls_sw_ctx_rx(tls_ctx); struct strp_msg *rxm; + u32 next_seq; /* device will request resyncs by itself based on stream scan */ if (ctx->resync_type != TLS_OFFLOAD_SYNC_TYPE_CORE_NEXT_HINT) @@ -866,9 +867,10 @@ static void tls_device_core_ctrl_rx_resync(struct tls_context *tls_ctx, ctx->resync_nh.decrypted_tgt += TLS_DEVICE_RESYNC_NH_MAX_IVAL; rxm = strp_msg(skb); + next_seq = tls_strp_msg_seq(sw_ctx) + rxm->full_len; /* head of next rec is already in, parser will sync for us */ - if (tcp_inq(sk) > rxm->full_len) { + if (after(READ_ONCE(tcp_sk(sk)->rcv_nxt), next_seq)) { trace_tls_device_rx_resync_nh_schedule(sk); ctx->resync_nh_do_now = 1; } else { @@ -878,9 +880,7 @@ static void tls_device_core_ctrl_rx_resync(struct tls_context *tls_ctx, memcpy(rcd_sn, tls_ctx->rx.rec_seq, prot->rec_seq_size); tls_bigint_increment(rcd_sn, prot->rec_seq_size); - tls_device_resync_rx(tls_ctx, sk, - tls_strp_msg_seq(sw_ctx) + - rxm->full_len, rcd_sn); + tls_device_resync_rx(tls_ctx, sk, next_seq, rcd_sn); } } -- 2.50.1