Re: [SSI-devel] semctl problem

[Bharata B R. <bha...@hp...>]

Kishore,

While your fix solves the problem I witnessed, I hit upon another
problem in the following code. This manifests as a hang.

ipc/sem.c: alloc_semundo()

1224  sma = sem_lock(semid);
1225  if (!sma) {
1226  	kfree(un);
1227  	return -EINVAL;
1228  }
1229  error = sem_revalidate(semid, sma, nsems, alter ? S_IWUGO :
S_IRUGO);
1230  if(error) {
1231  	kfree(un);
1232  	sem_unlock(sma);
1233  	return error;
1234  }

Here after doing sem_lock(), sem_revalidate() is called which again
tries sem_lock() on the same semid. This causes hang.

Also if sem_revalidate() fails, it never returns with sem_lock held, so
it shoudn't be doing sem_unlock().

I think the above piece of code should look like this:

	sma = sem_lock(semid);
	if (!sma) {
		kfree(un);
		return -EINVAL;
	}
	sem_unlock(sma);

	error = sem_revalidate(semid, sma, nsems, alter ? S_IWUGO : S_IRUGO);
	if(error) {
		kfree(un);
		return error;
	}

Regards,
Bharata.

On Mon, 2005-01-24 at 16:09, SAMPATHKUMAR KISHORE KANIYAR wrote:
>  I have fixed the code in error which caused the panic that you reported.
>  
>  I have also fixed the other instance which can cause panic in
>  sys_semctl() for the client-site invocation.
>  
>  I have added checks in freeundos() as well so that it does not scan
>  through the undolist without checking if the list is NULL.
>  
>  The patch below contains those fixes. Bharata, can you verify if
>  this fixes your problem? Laura, can you review my fix? I plan to
>  check-in tomorrow.
>  
>  Regards,
>  - Kishore
>  
> > I observed a NULL pointer dereference in semctl code.
> > 
> > Call Trace:
> >  [<c010a6df>] show_stack+0x7f/0xa0
> >  [<c010a88e>] show_registers+0x15e/0x210
> >  [<c010abb3>] die+0x93/0x120
> >  [<c0121336>] do_page_fault+0x2e6/0x688
> >  [<c010a341>] error_code+0x2d/0x38
> >  [<c01c0bfa>] sys_semctl+0x1da/0x350
> >  [<c0111d16>] sys_ipc+0xd6/0x340
> >  [<c0109819>] sysenter_past_esp+0x52/0x71
> > 
> > This particular problem is due to accessing undo_list without checking
> > its validity (ipc/sem.c:semctl_down(), line no.1055)
> > 
> > There are at least a couple of other places in the same file where the
> > semaphore undo structures list is accessed w/o checking for its validity
> > first.
> > 
> > Should we correct all references to undo_list to check for NULL before
> > accessing the list ?
> > 
> > Regards,
> > Bharata.
> > 
> > 
> > 
> > -------------------------------------------------------
> > This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
> > Tool for open source databases. Create drag-&-drop reports. Save time
> > by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
> > Download a FREE copy at http://www.intelliview.com/go/osdn_nl
> > _______________________________________________
> > ssic-linux-devel mailing list
> > ssi...@li...
> > https://lists.sourceforge.net/lists/listinfo/ssic-linux-devel
> > 
>