Re: rwlock issue in secmodel

[Lars Heidieker <lars%heidieker.de@localhost>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Thu, 30 Oct 2014 18:20:01 +0100
Maxime Villard <max%M00nBSD.net@localhost> wrote:

> Le 27/10/2014 21:21, Lars Heidieker a écrit :
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On Mon, 27 Oct 2014 16:42:15 +0100
> > Maxime Villard <max%M00nBSD.net@localhost> wrote:
> > 
> >> Hi,
> >> I think there's a rwlock issue in secmodel/secmodel.c:
> >>
> >> 174	if (sm == NULL) {
> >> 175		error = EFAULT;
> >> 176		goto out;
> >> 177	}
> >> 178
> >> 179	/* Check if the secmodel is already present. */
> >> 180	rw_enter(&secmodels_lock, RW_WRITER);
> >> ...
> >>   out:
> >> 194	/* Unlock the secmodels list. */
> >> 195	rw_exit(&secmodels_lock);
> >> 196
> >> 197	return error;
> >>
> >> This goto out will release secmodels_lock while it is not yet held,
> >> right?
> >>
> >> The same in secmodel_unplug().
> >>
> > 
> > Yes, seems that way to me.
> > secmodel_plug is only called from secmodel_register, where it would
> > crash anyway if sm is NULL so it should be an assert, right?
> 
> yes, but in case it gets used in the future...
> 
> > (and for unplug the check should be in secmodel_deregister)
> > 
> > Lars
> > 
> 
> Index: secmodel.c
> ===================================================================
> RCS file: /cvsroot/src/sys/secmodel/secmodel.c,v
> retrieving revision 1.1
> diff -u -r1.1 secmodel.c
> --- secmodel.c	4 Dec 2011 19:24:59 -0000	1.1
> +++ secmodel.c	30 Oct 2014 17:17:36 -0000
> @@ -171,10 +171,8 @@
>  	secmodel_t tsm;
>  	int error = 0;
>  
> -	if (sm == NULL) {
> -		error = EFAULT;
> -		goto out;
> -	}
> +	if (sm == NULL)
> +		return EFAULT;
>  
>  	/* Check if the secmodel is already present. */
>  	rw_enter(&secmodels_lock, RW_WRITER);
> @@ -203,10 +201,8 @@
>  	secmodel_t tsm;
>  	int error = 0;
>  
> -	if (sm == NULL) {
> -		error = EFAULT;
> -		goto out;
> -	}
> +	if (sm == NULL)
> +		return EFAULT;
>  
>  	/* Make sure the secmodel is present. */
>  	rw_enter(&secmodels_lock, RW_WRITER);
> 
> 
> Ok?
> 

looks good to me.

lars
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlRYtzsACgkQcxuYqjT7GRZRogCfbFcVHO9Fcxb1uM7UY8MX0RuW
O5AAoNFDw5kQdfTHgatRWQksRO32wvxQ
=MX72
-----END PGP SIGNATURE-----