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
pgppaUXGzbhRf.pgp
Description: PGP signature