Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0 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). - [Medium] Missing `Fixes:` tag and stable Cc on a real bug fix. Pre-existing issues: - [High] The identical leak the patch closes for hv_sock remains in the sibling transport that shares the same `vsk->close_work` /… -- vsock/hv_sock: fix socket/table leak when rescind races the delayed close This changes hvs_close_timeout() so that, when SOCK_DONE is already set but vsk->close_work_scheduled is still true, the running work drops the scheduling reference and calls vsock_remove_sock() itself. That covers the case where hvs_close_connection() could not cancel the already-dequeued work. A couple of questions below. > Make the running timeout complete the cleanup the rescind path could not: > when SOCK_DONE is set but the work is still marked scheduled, drop the > scheduling reference and remove the socket. The two cleanup sites are > mutually exclusive and guarded by close_work_scheduled, so the reference > is dropped exactly once. (cancel_delayed_work_sync() cannot be used from > hvs_do_close_lock_held(): it runs under the same lock_sock() the work > takes.) > > Signed-off-by: Bartłomiej Dmitruk <[email protected]> [Severity: Medium] Should this carry a Fixes: tag and a stable Cc? git blame on the pre-image points both halves of the defective logic at commit a9eeb998c28d ("hv_sock: Add support for delayed close"): the cancel_delayed_work()-conditional reference drop in hvs_do_close_lock_held(), and the unconditional vsk->close_work_scheduled = false in hvs_close_timeout(). The triggering event is a host-initiated channel rescind, so it is not under guest control. Would the following trailers be appropriate, along with a [PATCH net] subject prefix? Fixes: a9eeb998c28d ("hv_sock: Add support for delayed close") Cc: [email protected] > diff --git a/net/vmw_vsock/hyperv_transport.c > b/net/vmw_vsock/hyperv_transport.c > index 855b15971c359..2779961fc18ca 100644 > --- a/net/vmw_vsock/hyperv_transport.c > +++ b/net/vmw_vsock/hyperv_transport.c > @@ -499,10 +499,18 @@ static void hvs_close_timeout(struct work_struct *work) > > sock_hold(sk); > lock_sock(sk); > - if (!sock_flag(sk, SOCK_DONE)) > + if (!sock_flag(sk, SOCK_DONE)) { > hvs_do_close_lock_held(vsk, false); > - > - vsk->close_work_scheduled = false; > + } else if (vsk->close_work_scheduled) { > + /* A concurrent rescind (hvs_close_connection) set SOCK_DONE but > + * could not cancel this already-running work, so it left the > + * scheduling reference and vsock_remove_sock() to us. Finish > + * the cleanup to avoid leaking the socket and its table entry. > + */ > + vsk->close_work_scheduled = false; > + vsock_remove_sock(vsk); > + sock_put(sk); > + } > release_sock(sk); > sock_put(sk); > } [Severity: High] This is a pre-existing issue and not introduced by this patch, but does the same struct sock leak still exist in virtio_transport_common.c, which uses the same vsk->close_work and vsk->close_work_scheduled fields from include/net/af_vsock.h? virtio_transport_close_timeout() still has the pattern this hunk replaces: net/vmw_vsock/virtio_transport_common.c:virtio_transport_close_timeout() { sock_hold(sk); lock_sock(sk); if (!sock_flag(sk, SOCK_DONE)) { (void)virtio_transport_reset(vsk, NULL); virtio_transport_do_close(vsk, false); } vsk->close_work_scheduled = false; release_sock(sk); sock_put(sk); } and the cancel-failure path is the same one described in the changelog: net/vmw_vsock/virtio_transport_common.c:virtio_transport_cancel_close_work() { if (vsk->close_work_scheduled && (!cancel_timeout || cancel_delayed_work(&vsk->close_work))) { vsk->close_work_scheduled = false; virtio_transport_remove_sock(vsk); /* Release refcnt obtained when we scheduled the timeout */ sock_put(sk); } } The racing event there is a peer packet rather than a host rescind: virtio_transport_recv_pkt() lock_sock() virtio_transport_recv_connected() /* VIRTIO_VSOCK_OP_RST */ virtio_transport_do_close(vsk, true) virtio_transport_cancel_close_work(vsk, true) If virtio_transport_close_timeout() has already been dequeued and is blocked on lock_sock(), cancel_delayed_work() returns false, so close_work_scheduled stays true, the scheduling sock_put() is skipped and virtio_transport_remove_sock() is not called, while SOCK_DONE is set. The work then resumes, skips the !sock_flag(sk, SOCK_DONE) branch, clears close_work_scheduled and drops only its own local reference. Does that leak the struct sock and its struct virtio_vsock_sock, leave the socket in the bound/connected tables, and leave vvs->rx_queue unpurged? The recovery in virtio_transport_destruct() -> virtio_transport_cancel_close_ work(vsk, true) would not run because sk_refcnt never reaches zero, and would be a no-op anyway once the flag has been cleared. Since both transports share close_work and close_work_scheduled, would it make sense to put this cleanup rule in code common to both rather than only in hyperv_transport.c, so the two users do not implement different ownership protocols for the same field? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917220213.56102-1-bartlomiej.dmitruk%40isec.pl
