MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi Michael,

Thanks for the careful review of v1 -- you were right that v1 was
not ready to merge. Let me walk through the issues and the fixes
in this v2.

1. Patch format corruption
   -----------------------
   v1's diff ended up with "4321006" in the file mode line instead
   of "100644", so `git am` could not apply it. v2 is generated by
   `git diff` against a fresh checkout of net/tap-solaris.c from
   master; `git apply --check` and `patch -p1` both pass cleanly.

2. ip_fd was being closed while still in use
   -----------------------------------------
   In v1 I had `close(if_fd); close(ip_fd);` sitting next to each
   other right before the SIOCSLIFMUXID ioctl() block. But the
   next ioctl() uses ip_fd, so closing it there meant we were
   issuing ioctl() on a stale fd. v2 removes that close entirely:
   ip_fd is now closed only on the failure paths, and the SIOCSLIFMUXID
   block uses a fully valid ip_fd.

3. The label scheme was double-releasing muxids
   ---------------------------------------------
   v1 had separate `fail_arp_muxid` and `fail_arp_fd` labels, and
   the SIOCSLIFMUXID failure path did
       I_PUNLINK arp_muxid;
       I_PUNLINK ip_muxid;
       error_report(...);
       goto fail_arp_muxid;
   then fall-through into fail_arp_muxid which I_PUNLINK'd them
   *again*. v2 fixes this by keeping the inline I_PUNLINK pair
   in the SIOCSLIFMUXID failure path (those muxids were both
   successfully established at that point) and `goto fail_if_fd`
   to skip past the redundant muxid cleanup. The result is each
   muxid and each fd is closed exactly once on every error path.

The new cleanup chain
---------------------

  fail_ip_muxid:  I_PUNLINK ip_muxid
  fail_arp_fd:    close(arp_fd)
  fail_if_fd:     close(if_fd)
  fail_tap_fd:    close(tap_fd); close(ip_fd) if it was opened; ip_fd = 0

ip_fd is a function-static variable, so resetting it to 0 after
close is required: the next tap_alloc() invocation starts with
`if (ip_fd) close(ip_fd);`, and we must not close a fd that some
other code now owns.

I considered folding the SIOCSLIFMUXID failure path into the goto
chain as well, so that *every* cleanup is centralised, but that
would have to either (a) duplicate the I_PUNLINK pair across two
labels, or (b) reorder the chain so muxid cleanup runs before
SIOCSLIFMUXID -- which doesn't make sense because those muxids
don't exist before SIOCSLIFMUXID validates them. I think the
current "inline I_PUNLINK at SIOCSLIFMUXID failure + goto
fail_if_fd" is the cleanest decomposition, but if you prefer
fully-centralised cleanup I'm happy to refactor it that way.

Testing
-------
I do not have access to a Solaris 11 host here, so this patch
has not been tested on a real Solaris system. The change is
purely a refactor of the existing control flow, and the goto-
cleanup pattern mirrors the one used in net/tap-linux.c (which
is tested regularly), so I am fairly confident -- but please
flag anything that looks off and I will follow up.

Could you please take another look?

Thanks,
Weimin

---
Xiong Weimin (1):
  net/tap-solaris: Fix resource leaks on error paths

 net/tap-solaris.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

On Sat, Sep 05, 2026 at 03:04:47PM +0800, Xiong Weimin wrote:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Hi Michael,
> 
> Thanks for the careful review of v1 -- you were right that v1 was
> not ready to merge. Let me walk through the issues and the fixes
> in this v2.
> 
> 1. Patch format corruption
>    -----------------------
>    v1's diff ended up with "4321006" in the file mode line instead
>    of "100644", so `git am` could not apply it. v2 is generated by
>    `git diff` against a fresh checkout of net/tap-solaris.c from
>    master; `git apply --check` and `patch -p1` both pass cleanly.
> 
> 2. ip_fd was being closed while still in use
>    -----------------------------------------
>    In v1 I had `close(if_fd); close(ip_fd);` sitting next to each
>    other right before the SIOCSLIFMUXID ioctl() block. But the
>    next ioctl() uses ip_fd, so closing it there meant we were
>    issuing ioctl() on a stale fd. v2 removes that close entirely:
>    ip_fd is now closed only on the failure paths, and the SIOCSLIFMUXID
>    block uses a fully valid ip_fd.
> 
> 3. The label scheme was double-releasing muxids
>    ---------------------------------------------
>    v1 had separate `fail_arp_muxid` and `fail_arp_fd` labels, and
>    the SIOCSLIFMUXID failure path did
>        I_PUNLINK arp_muxid;
>        I_PUNLINK ip_muxid;
>        error_report(...);
>        goto fail_arp_muxid;
>    then fall-through into fail_arp_muxid which I_PUNLINK'd them
>    *again*. v2 fixes this by keeping the inline I_PUNLINK pair
>    in the SIOCSLIFMUXID failure path (those muxids were both
>    successfully established at that point) and `goto fail_if_fd`
>    to skip past the redundant muxid cleanup. The result is each
>    muxid and each fd is closed exactly once on every error path.
> 
> The new cleanup chain
> ---------------------
> 
>   fail_ip_muxid:  I_PUNLINK ip_muxid
>   fail_arp_fd:    close(arp_fd)
>   fail_if_fd:     close(if_fd)
>   fail_tap_fd:    close(tap_fd); close(ip_fd) if it was opened; ip_fd = 0
> 
> ip_fd is a function-static variable, so resetting it to 0 after
> close is required: the next tap_alloc() invocation starts with
> `if (ip_fd) close(ip_fd);`, and we must not close a fd that some
> other code now owns.
> 
> I considered folding the SIOCSLIFMUXID failure path into the goto
> chain as well, so that *every* cleanup is centralised, but that
> would have to either (a) duplicate the I_PUNLINK pair across two
> labels, or (b) reorder the chain so muxid cleanup runs before
> SIOCSLIFMUXID -- which doesn't make sense because those muxids
> don't exist before SIOCSLIFMUXID validates them. I think the
> current "inline I_PUNLINK at SIOCSLIFMUXID failure + goto
> fail_if_fd" is the cleanest decomposition, but if you prefer
> fully-centralised cleanup I'm happy to refactor it that way.
> 
> Testing
> -------
> I do not have access to a Solaris 11 host here, so this patch
> has not been tested on a real Solaris system. The change is
> purely a refactor of the existing control flow, and the goto-
> cleanup pattern mirrors the one used in net/tap-linux.c (which
> is tested regularly), so I am fairly confident -- but please
> flag anything that looks off and I will follow up.
> 
> Could you please take another look?

Sorry, as long as it's not tested - not really interested.

> Thanks,
> Weimin
> 
> ---
> Xiong Weimin (1):
>   net/tap-solaris: Fix resource leaks on error paths
> 
>  net/tap-solaris.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)