Re: [PATCH net-next v2 1/8] tls: bound consecutive no-data records in tls_sw_read_sock()

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new limit can deadlock a CPU through recursion.
- [Low] The new call loads sk->sk_data_ready with a plain read and calls
  it, while another thread can rewrite the pointer.

--

tls: bound consecutive no-data records in tls_sw_read_sock()

This commit stops tls_sw_read_sock() once it has consumed
TLS_RX_NODATA_LIMIT (16) consecutive zero-length application_data
records in one call. A record that delivers data resets the count. When
the limit is reached, the loop breaks and calls sk->sk_data_ready() so
the records still queued are noticed.

[ ... ]

[ ... ]

[Severity: High]
Can this direct call to sk->sk_data_ready() re-enter tls_sw_read_sock()
and deadlock on sk_lock.slock?

When this line runs, ctx->reader_present is still 1 and the caller
still holds the socket lock.

Suppose KCM is attached to the TCP socket after TLS_RX has been set up.
kcm_attach() does not reject ULP sockets, and it installs
psock_data_ready() as csk->sk_data_ready. The KCM psock sits in
sk_user_data without SK_USER_DATA_PSOCK, so the sk_psock_get() check at
the top of tls_sw_read_sock() does not catch it.

In softirq the chain would look like this:

tcp_v4_rcv()
  bh_lock_sock(sk)
  tcp_data_ready()
    psock_data_ready()
      strp_data_ready()            /* socket not owned by user */
        strp_read_sock()
          tls_sw_read_sock()       /* reader_present = 1 */
            /* 16 empty records consumed */
            sk->sk_data_ready(sk)  /* psock_data_ready() */
              strp_data_ready()
                strp_read_sock()
                  tls_sw_read_sock()
                    tls_rx_reader_acquire()
                      sk_wait_event()
                        release_sock()
                          spin_lock_bh(&sk->sk_lock.slock)

The nested tls_rx_reader_acquire() sees reader_present == 1 and calls
sk_wait_event(). That calls release_sock() before the zero timeout is
checked. release_sock() then spins on sk_lock.slock, which this CPU
already holds through bh_lock_sock().

A remote peer could trigger this with 16 empty application_data records.
In TLS 1.3 that is about 352 bytes, which fits in one segment.

Before this patch, with TLS set up first and KCM second, the only
notification from inside the reader was tls_rx_reader_release() ->
tls_rx_msg_maybe_announce() -> ctx->saved_data_ready. That is the
callback that was installed before TLS (sock_def_readable here), so the
consumer's callback was never called re-entrantly. If KCM attaches
first, saved_data_ready is already psock_data_ready, so the TLS-then-KCM
ordering is the new exposure.

Would it work to defer the notification until the loop has exited and
reader_present has been cleared, or to queue it as work?

[Severity: Low]
Should this be READ_ONCE(sk->sk_data_ready)(sk), as the TCP call sites
use?

nvme_tcp_restore_sock_ops() does a plain store to sk->sk_data_ready
while holding only sk_callback_lock. __nvme_tcp_stop_queue() calls it
before cancel_work_sync(&queue->io_work). That means io_work can be
inside nvme_tcp_try_recv() -> tls_sw_read_sock() and load the pointer
at the same moment.

The result is an unannotated data race on the callback pointer, which
KCSAN would report.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-tls-follow-on-v2-0-2dd1947bb642%40kernel.org