RE: [SSI-devel] semctl problem

[SAMPATHKUMAR K. K. <sam...@in...>]

Hello Laura,
  
Laura Ramirez wrote ...
  
+ Hi Kishore,
+ 
+ I looked at your patch.  Everything looks good, however for the
+ freeundos(), the change seems to be redundant since it only gets
+ called when the list is not NULL.  
  
I initially introduced this _redundant_ code to ensure that we do
not by mistake invoke freeundos() without checking for the validity
of the undo_list. However, as per what you have written, I now feel
that amount of over-caution is not necessary.
  
  
+ Also the freeundos() need to do the decr and test check before
+ actually doing the frees, since they structure may be shared by
+ clones.
  
Incorporated your patch changes and checked-in.
  
  
+ Also, from looking at the code the comparison of tgid and pid in your
+ patch should really be epid and pid.
  
I think your patch did not address this. I have incorporated this, and
have checked in.
  
With this, I believe I have looked at all the instances related to the
comparison of tgid and pid and have changed them where appropriate. Can
you look once over and let me know if the checked-in code addressses
all instances where the change is warranted?
  
  
+ Attach is patch for freeundos() changes that your patch needs.
+ 
+ laura
  
Thanks and Regards,
- Kishore
  
+ 
+ >  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.