Re: [PATCH net 1/1] net/handshake: Protect request hash lookups

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/handshake: Protect request hash lookups

This patch adds a kref and an rcu_head to struct handshake_req.
handshake_req_hash_lookup() and handshake_req_next() now return a
counted reference, and callers release it with handshake_req_put(). The
final put frees the request via kfree_rcu().

Closes: https://sashiko.dev/#/patchset/20260521-handshake-file-pin-v2-0-b9dadc472040@oracle.com
Closes: https://sashiko.dev/#/patchset/20260525-handshake-file-pin-v3-0-66c616906ead@oracle.com

> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 3fd4fef9bab1a..da5d41e139a78 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c

[ ... ]

> @@ -127,6 +128,7 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>  		handshake_complete(req, -EIO, NULL);
>  out_status:
>  	trace_handshake_cmd_accept_err(net, req, NULL, err);
> +	handshake_req_put(req);
>  	return err;
>  }

[Severity: High]
The commit message says:

  If FD_PREPARE() or hp_accept() fails after a concurrent cancellation
  or socket close has released the submit file pin, dropping the file pin
  in handshake_nl_accept_doit() can trigger socket destruction
  immediately, freeing the request before handshake_complete() or
  trace_handshake_cmd_accept_err() completes.

The new kref keeps req alive during this window. Is req->hr_sk still
protected, though?

Both error paths in handshake_nl_accept_doit() drop the accept side file
reference before they reach handshake_complete():

    FD_PREPARE(fdf, O_CLOEXEC, req->hr_file);
    if (fdf.err) {
        fput(req->hr_file); /* drop ref from handshake_req_next() */
        err = fdf.err;
        goto out_complete;
    }

    err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
    if (err)
        goto out_complete; /* Automatic cleanup handles fput */

handshake_req_next() now takes a kref on the request, but nothing takes
a reference on the sock. handshake_complete() then dereferences the sock
before it checks HANDSHAKE_F_REQ_COMPLETED:

net/handshake/request.c:handshake_complete() {
    struct sock *sk = req->hr_sk;
    struct net *net = sock_net(sk);

    if (!test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
    ...
}

Can the following interleaving cause a use-after-free of the sock?

tlshd (accept)                     consumer (e.g. nvme-tcp)
handshake_req_next()
  get_file(), kref_get()
                                   tls_handshake_cancel()
                                     handshake_req_cancel()
                                       test_and_set_bit(COMPLETED)
                                       fput(req->hr_file)
FD_PREPARE() fails (EMFILE)
  fput(req->hr_file)
  (or hp_accept() fails and the
   FD_PREPARE cleanup does fput)
                                   nvme_tcp_alloc_queue() err_sock:
                                     __fput_sync(queue->sock->file)
                                       sock_close()->tcp_close()->
                                       ...->handshake_sk_destruct()
                                         (req survives on accept kref)
                                         sk_destruct(sk)  /* sock freed */
handshake_complete(req, -EIO, NULL)
  sk = req->hr_sk;
  sock_net(sk)   /* reads freed sock */

The final fput runs synchronously on the consumer's thread. Deferring
tlshd's own fput through task_work would therefore not close this window.

One option is to keep the accept side file reference until after
handshake_complete(). Another is to take sock_hold(req->hr_sk) in
handshake_req_next() and drop it in handshake_nl_accept_doit(). A third
is to move the hr_sk and sock_net() dereference inside the
test_and_set_bit() branch of handshake_complete(). Would any of these
work here?