cvs commit: src/sys/i386/i386 sys_machdep.c

[Kostik Belousov]

On Fri, Sep 12, 2008 at 10:22:35AM -0400, John Baldwin wrote:
> On Friday 12 September 2008 05:51:11 am Konstantin Belousov wrote:
> > kib         2008-09-12 09:51:11 UTC
> > 
> >   FreeBSD src repository
> > 
> >   Modified files:
> >     sys/i386/i386        sys_machdep.c 
> >   Log:
> >   SVN rev 182960 on 2008-09-12 09:51:11Z by kib
> >   
> >   The user_ldt_alloc() function shall return with dt_lock locked.
> >   The user_ldt_free() function shall return with dt_lock unlocked.
> >   Error handling code in both functions do not handle this, fix it by
> >   doing necessary lock/unlock.
> >   
> >   While there, fix minor style nits.
> 
> Hmm, I had actually thought it was intentional for user_ldt_alloc() to only 
> return with the lock held on success and depend on a later call to another 
> method to drop the lock in the success case (so the locking isn't visible to 
> consumers of the API in theory).  For example, i386_ldt_grow() depended on 
> this feature and is now broken (it leaks a lock on failure).  I missed this 
> when looking at this yesterday.

I probably miss something there.

On failure of user_ldt_alloc(), i386_ldt_grow() does return (ENOMEM),
without changing lock state for dt_lock.

There are three call locations for the i386_ldt_grow(), all of
them in i386_set_ldt(). On failure, each call location does
mtx_unlock_spin(&dt_lock) immediately after call. So I assumed that
protocol for i386_ldt_grow() is to always return with dt_lock locked.

Two other callers of the user_ldt_alloc() in cpu_fork() do panic()
immediately after the failed call to user_ldt_alloc().

Could you, please, point me to exact place where the lock would leak ?

> 
> Other notes:
> 
> - Since user_ldt_free() handles the case of there not being an LDT, the code
>   in exec_setregs() on i386 can be simplified to just always call
>   user_ldt_free().
> - cpu_exit() could possibly do the same.  I wonder if exec_setregs() needs the
>   same fixup to %gs that cpu_exit() does.  If so, that could possibly be moved
>   into user_ldt_free().  Ah, exec_setregs() does it unconditionally.  I think
>   you could make cpu_exit() just do it unconditionally as well before calling
>   user_ldt_free().
> 
> -- 
> John Baldwin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-src/attachments/20080912/3d8b1adc/attachment.pgp