i386/123731: [patch][fix][sysvsem] System V semaphores cause unexpected hanging

[Ivan Shcheklein]

>Number:         123731
>Category:       i386
>Synopsis:       [patch][fix][sysvsem] System V semaphores cause unexpected hanging
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    freebsd-i386
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Fri May 16 11:30:02 UTC 2008
>Closed-Date:
>Last-Modified:
>Originator:     Ivan Shcheklein
>Release:        FREEBSD-6.2
>Organization:
ISPRAS
>Environment:
FreeBSD bs. 6.2-RELEASE FreeBSD 6.2-RELEASE #0: Fri Dec 14 20:38:20 MSK 2007     root at bsd.:/usr/obj/usr/src/sys/GENERIC  i386
>Description:
It seems that my previous PR (http://www.freebsd.org/cgi/query-pr.cgi?pr=118153) was too hard to understand. There is no any feedback for a long time and no one was assigned to consider the problem. Moreover it was tagged with [sysvshm] while [sysvsem] is quite better here.

I want to try once again since I suppose it is really critical bug.

The bug itself is the following:

in src/sys/kern/sysv_sem.c,v 1.78 2005/06/07 we have:

1. in line 1066 we get semptr through sem_base
  
    semptr = &semakptr->u.sem_base[sopptr->sem_num];  

2. in lines 1129-1333 we adjust counters using this semptr:

    if (sopptr->sem_op == 0)
        semptr->semzcnt++;
    else
        semptr->semncnt++;

3. in line 1135 we age going to sleep:

    error = msleep(semakptr, sema_mtxp, (PZERO - 4) | PCATCH, "semwait", 0);

4. in lines 1153-1156 after wake up we again adjust counters using the same semptr:

    if (sopptr->sem_op == 0)
         semptr->semzcnt--;
    else
         semptr->semncnt--;

During "msleep" in step 3 "sem_base" of the semaphore can be moved (if other process drops semaphore which was allocated before this one). Therefore, I believe, in step 4 "semptr" value can be invalid and MUST be renewed before use. 

As a circumstantial proof you can also consider the same Darwin's code:

----------------------------------------------------
		/*
		 * IMPORTANT: while we were asleep, the semaphore array might
		 * have been reallocated somewhere else (see grow_sema_array()).
		 * When we wake up, we have to re-lookup the semaphore 
		 * structures and re-validate them.
		 */

		suptr = NULL;	/* sem_undo may have been reallocated */
	 	semaptr = &sema[semid];	   /* sema may have been reallocated */
----------------------------------------------------

This bug can cause unexpected processes hanging in the multiprocess environment which extensively use System V semaphores.

What else do you need to consider this bug?


>How-To-Repeat:

>Fix:
Patch is attached which renew sempth after wakeup:

*** sysv_sem_old.c	Fri Dec 14 18:40:09 2007
--- sysv_sem.c	Fri Dec 14 18:23:46 2007
***************
*** 1145,1154 ****
--- 1145,1159 ----
  			error = EIDRM;
  			goto done2;
  		}
  
  		/*
+ 		 * Renew the semaphore's pointer after wakeup
+ 		 */
+ 		semptr = &semakptr->u.sem_base[sopptr->sem_num];
+ 
+ 		/*
  		 * The semaphore is still alive.  Readjust the count of
  		 * waiting processes.
  		 */
  		if (sopptr->sem_op == 0)
  			semptr->semzcnt--;

Note! Possibly some other strucutures should be also re-validates like in Darwin.

Patch attached with submission follows:

*** sysv_sem_old.c	Fri Dec 14 18:40:09 2007
--- sysv_sem.c	Fri Dec 14 18:23:46 2007
***************
*** 1145,1154 ****
--- 1145,1159 ----
  			error = EIDRM;
  			goto done2;
  		}
  
  		/*
+ 		 * Renew the semaphore's pointer after wakeup
+ 		 */
+ 		semptr = &semakptr->u.sem_base[sopptr->sem_num];
+ 
+ 		/*
  		 * The semaphore is still alive.  Readjust the count of
  		 * waiting processes.
  		 */
  		if (sopptr->sem_op == 0)
  			semptr->semzcnt--;


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