Re: CVS commit: src/sys/net

[Andrew Doran <ad%netbsd.org@localhost>]

On Tue, Nov 25, 2008 at 02:40:36AM +0000, Quentin Garnier wrote:

> Module Name:  src
> Committed By: cube
> Date:         Tue Nov 25 02:40:36 UTC 2008
> 
> Modified Files:
>       src/sys/net: bsd-comp.c if_ppp.c if_ppp.h ppp-comp.h ppp-deflate.c
> 
> Log Message:
> Rework the way PPP compmressors are handled and allow them to be
> automatically loaded when needed.

Thanks, however there are problems with this:

- The code using module_hold() has race conditions. It would be better to
  reference count the compressors individually.

- It appears to leak module references on error.

- ppp_register_compressor/deregister_compressor check for existance and then
  insert/remove which involves dropping and re-acquring the lock. The result
  of the check is invalid once the lock is dropped. TOCTOU.

- ppp_deflate_modcmd() can unregister the first compressor but fail to
  unregister the second during unload.

- ppp_deflate_modcmd() can fail to register the second compressor, by which
  time the first is already registerd and could have become busy. I suggest
  fixing these two by registering groups of compressors instead of
  registering them individually.

- There is no explicit locking in pppioctl(). Calls made when manipluating
  compressors can sleep and lead to race conditions, as kernel_lock is
  dropped automatically when sleeping.

Andrew