kern/159355: unp_gc in 8.2 is once again being overly agressive in recycling port rights

[Spencer Minear]

>Number:         159355
>Category:       kern
>Synopsis:       unp_gc in 8.2 is once again being overly agressive in recycling port rights
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Mon Aug 01 13:50:09 UTC 2011
>Closed-Date:
>Last-Modified:
>Originator:     Spencer Minear
>Release:        8.2-Release
>Organization:
McAfee Inc.
>Environment:
We found it in our SecureOS variant of FreeBSD 8.2.  But can see it on installation of the 8.2 release code. FreeBSD freebe1.scur.com 8.2-RELEASE FreeBSD 8.2-RELEASE
>Description:
It appears that changes in the lock processing related to the unpcb list and socket receive buffers opened up a timing window in which the unp_gc processing will see unpcb's in the unpcb list that look like they should be recycled but are currently in the process of being received or have been received.

>From an operational point what we could see is that a client sent one end of a socket pair to a server, closed its end.  We saw some data exchanged between the server and the client and then unp_gc would get around to finishing up its processing which included marking the socket as one that cannot receive more data.  The client would try to send some data and get a broken pipe error code.
>How-To-Repeat:
Run the test program provided with PR 112554.  It will report the problem again and you can also see it by monitoring the net.local.recycled sysctl value.
>Fix:
We fixed the problem by adding a check in the unp_gc code to make sure that the socket still looks like one that should be reclaimed before before it is added to the unref array.  We also added some extra counters to make sure that if there is a mix of recycles and false recycles to make sure that later loops over the unref array act on the correct number of entries.

A diff in our code base is as follows.  The line numbers do not match your code files but it does includes the FreeBSD original code along with compiler flag that we use to mark our changes from the base code.  This should make it clear what we did to fix the problem.

Index: uipc_usrreq.c
===================================================================
RCS file: /projects/fusion/cvs/src/freebsd/sys/kern/uipc_usrreq.c,v
retrieving revision 1.24
retrieving revision 1.25
diff -c -r1.24 -r1.25
*** uipc_usrreq.c       23 May 2011 16:04:40 -0000      1.24
--- uipc_usrreq.c       26 Jul 2011 21:34:10 -0000      1.25
***************
*** 2106,2111 ****
--- 2106,2118 ----
  SYSCTL_INT(_net_local, OID_AUTO, recycled, CTLFLAG_RD, &unp_recycled, 0, 
      "Number of unreachable sockets claimed by the garbage collector.");
  
+ #ifdef SCC_KERNEL
+ static int unp_recovered;
+ SYSCTL_INT(_net_local, OID_AUTO, recovered, CTLFLAG_RD,
+          &unp_recovered, 0,
+          "Number of times gc declared a socket dead but it was just externalized.");
+ #endif /* SCC_KERNEL */
+ 
  static int unp_taskcount;
  SYSCTL_INT(_net_local, OID_AUTO, taskcount, CTLFLAG_RD, &unp_taskcount, 0, 
      "Number of times the garbage collector has run.");
***************
*** 2118,2123 ****
--- 2125,2133 ----
        struct file **unref;
        struct unpcb *unp;
        int i;
+ #ifdef SCC_KERNEL
+       int dead_cnt;
+ #endif /* SCC_KERNEL */ 
  
        unp_taskcount++;
        UNP_LIST_LOCK();
***************
*** 2156,2170 ****
--- 2166,2202 ----
         * as as unreachable and store them locally.
         */
        UNP_LIST_LOCK();
+ #ifndef SCC_KERNEL
        for (i = 0, head = heads; *head != NULL; head++)
+ #else
+       for (dead_cnt = 0, head = heads; *head != NULL; head++)
+ #endif /* !SCC_KERNEL */
                LIST_FOREACH(unp, *head, unp_link)
                        if (unp->unp_gcflag & UNPGC_DEAD) {
+ #ifdef SCC_KERNEL
+                           if (unp->unp_msgcount == 0) {
+                               /* This was probably externalized 
+                                * between when it was marked dead
+                                * by an early unp_scan and the unp
+                                * that carried it could be scanned */
+                               unp_recovered++;
+                           } else {
+                               unref[dead_cnt++] = unp->unp_file;
+ #else
                                unref[i++] = unp->unp_file;
+ #endif /* SCC_KERNEL */
                                fhold(unp->unp_file);
                                KASSERT(unp->unp_file != NULL,
                                    ("unp_gc: Invalid unpcb."));
+ #ifndef SCC_KERNEL
                                KASSERT(i <= unp_unreachable,
                                    ("unp_gc: incorrect unreachable count."));
+ #else
+                               KASSERT(dead_cnt <= unp_unreachable,
+                                   ("unp_gc: incorrect unreachable count."));
+ 
+                           }
+ #endif /* !SCC_KERNEL */ 
                        }
        UNP_LIST_UNLOCK();
  
***************
*** 2173,2187 ****
--- 2205,2231 ----
         * struct files associated with these sockets but leave each socket
         * with one remaining ref.
         */
+ #ifndef SCC_KERNEL
        for (i = 0; i < unp_unreachable; i++)
+ #else
+       for (i = 0; i < dead_cnt; i++)
+ #endif /* !SCC_KERNEL */
                sorflush(unref[i]->f_data);
  
        /*
         * And finally release the sockets so they can be reclaimed.
         */
+ #ifndef SCC_KERNEL
        for (i = 0; i < unp_unreachable; i++)
+ #else
+       for (i = 0; i < dead_cnt; i++)
+ #endif /* !SCC_KERNEL */
                fdrop(unref[i], NULL);
+ #ifndef SCC_KERNEL
        unp_recycled += unp_unreachable;
+ #else
+       unp_recycled += dead_cnt;
+ #endif /* !SCC_KERNEL */
        free(unref, M_TEMP);
  }
  


>Release-Note:
>Audit-Trail:
>Unformatted: