RE: TCP socket shutdown race condition

From: Ed Maste (emaste_at_sandvine.com)
Date: 08/21/03

  • Next message: Eugene Grosbein: "PnP VendorID list" 
    To: 'Mike Silbersack' <silby@silby.com>
Date: Wed, 20 Aug 2003 18:41:49 -0400

    >Well, I guess the spl() fix is probably going to be the quickest here
    >then, please send it to me once you've pounded on it, Ed.

    So my spl() fix seems to eliminate the problem for me but while I'm looking
    at this stuff I want to make sure there aren't any related cases left in.
    My current patch is at the end of this message.

    One potential problem is a crfree() interrupting code that's incrementing a
    ucred reference. For example, uipc_socket.c:socreate():

            so->so_cred = p->p_ucred;
            crhold(so->so_cred);

    However, I don't think a crfree() interrupting between these should cause a
    problem. The refcount would always be at least 2 to begin with, so the
    ucred wouldn't be free()d.

    Another possible problem is a crfree() interrupting a crfree() for the same
    ucred. This would result in a double free, leading to who knows what
    corruption. I did try the test against a kernel with invariants on, but
    nothing happened; presumably the timing changed enough that the race
    condition wouldn't occur.

    I protected the if (--cr->cr_ref == 0) in crfree() with splhigh() but left
    the actuall free() out of splhigh.

    The cr->cr_ref++ in crhold() assembles to an incl (%eax) which will be
    atomic on a single processor. However I don't think anything guarantees gcc
    will emit that code for that source instruction, so I put a splhigh around
    it too. Probably atomic_add_int would be better -- I'll try that out too.

    Our stress test is running against the patch below, and I'll report any
    findings. If you have any comments on this, please let me know.

    Index: kern_prot.c
    ===================================================================
    RCS file: /cvs/src/sys/kern/kern_prot.c,v
    retrieving revision 1.53.2.9
    diff -c -3 -r1.53.2.9 kern_prot.c
    *** kern_prot.c 9 Mar 2002 05:20:26 -0000 1.53.2.9
    --- kern_prot.c 20 Aug 2003 22:12:29 -0000
    ***************
    *** 997,1003 ****
    --- 997,1005 ----
      crhold(cr)
              struct ucred *cr;
      {
    + int s = splhigh();
            cr->cr_ref++;
    + splx(s);
      }
      
      /*
    ***************
    *** 1008,1017 ****
    --- 1010,1021 ----
      crfree(cr)
            struct ucred *cr;
      {
    + int s = splhigh();
            if (cr->cr_ref == 0)
                    panic("Freeing already free credential! %p", cr);
            
            if (--cr->cr_ref == 0) {
    + splx(s);
                    /*
                     * Some callers of crget(), such as nfs_statfs(),
                     * allocate a temporary credential, but don't
    ***************
    *** 1020,1026 ****
    --- 1024,1032 ----
                    if (cr->cr_uidinfo != NULL)
                            uifree(cr->cr_uidinfo);
                    FREE((caddr_t)cr, M_CRED);
    + return;
            }
    + splx(s);
      }
      
      /*
    _______________________________________________
    freebsd-net@freebsd.org mailing list
    http://lists.freebsd.org/mailman/listinfo/freebsd-net
    To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"

  • Next message: Eugene Grosbein: "PnP VendorID list"