diff --git a/ChangeLog b/ChangeLog index e8c812bca10..6b8bf1cb95b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6123,3 +6123,12 @@ src/stm32f429i-disco-internal.h, up_nsh.c, and up_spi.h: Add a configuration and board support for an external SST25 FLASH. From Ken Pettit (2013-11-28). + * fs/fs_inode.c: The inode semaphore must be re-entrant. Here is the + re-entering path that I found: (1) USB host connects to FLASH drive + and creates /dev/sda, (2) /dev/sda is mounted, (3) FLASH drive is + removed but /dev/sda is not destroyed because there is still a\ + reference on the device because of the mount, (4) umount() is called, + taking the inode semaphore, now the driver tries to destroy the block + driver by calling unregister_blockdriver(). But (5) + unregister_blockdriver() also takes the inode semaphore causing a + deadlock if the inode semaphore is not re-entry. (2013-11-28). diff --git a/fs/fs_inode.c b/fs/fs_inode.c index 14da54ceab3..0fe8689c7f9 100644 --- a/fs/fs_inode.c +++ b/fs/fs_inode.c @@ -52,11 +52,31 @@ * Pre-processor Definitions ****************************************************************************/ +#define NO_HOLDER (pid_t)-1; + +/**************************************************************************** + * Private Types + ****************************************************************************/ +/* Implements a re-entrant mutex for inode access. This must be re-entrant + * because there can be cycles. For example, it may be necessary to destroy + * a block driver inode on umount() after a removable block device has been + * removed. In that case umount() hold the inode semaphore, but the block + * driver may callback to unregister_blockdriver() after the un-mount, + * requiring the seamphore again. + */ + +struct inode_sem_s +{ + sem_t sem; /* The semaphore */ + pid_t holder; /* The current holder of the semaphore */ + int16_t count; /* Number of counts held */ +}; + /**************************************************************************** * Private Variables ****************************************************************************/ -static sem_t tree_sem; +static struct inode_sem_s g_inode_sem; /**************************************************************************** * Public Variables @@ -164,8 +184,10 @@ void fs_initialize(void) * a-time access to the inode tree). */ - (void)sem_init(&tree_sem, 0, 1); - + (void)sem_init(&g_inode_sem.sem, 0, 1); + g_inode_sem.holder = NO_HOLDER; + g_inode_sem.count = 0; + /* Initialize files array (if it is used) */ #ifdef CONFIG_HAVE_WEAKFUNCTIONS @@ -180,21 +202,42 @@ void fs_initialize(void) * Name: inode_semtake * * Description: - * Get exclusive access to the in-memory inode tree (tree_sem). + * Get exclusive access to the in-memory inode tree (g_inode_sem). * ****************************************************************************/ void inode_semtake(void) { + pid_t me; + + /* Do we already hold the semaphore? */ + + me = getpid(); + if (me == g_inode_sem.holder) + { + /* Yes... just increment the count */ + + g_inode_sem.count++; + DEBUGASSERT(g_inode_sem.count > 0); + } + /* Take the semaphore (perhaps waiting) */ - while (sem_wait(&tree_sem) != 0) + else { - /* The only case that an error should occr here is if - * the wait was awakened by a signal. - */ + while (sem_wait(&g_inode_sem.sem) != 0) + { + /* The only case that an error should occr here is if + * the wait was awakened by a signal. + */ - ASSERT(get_errno() == EINTR); + ASSERT(get_errno() == EINTR); + } + + /* No we hold the semaphore */ + + g_inode_sem.holder = me; + g_inode_sem.count = 1; } } @@ -202,13 +245,31 @@ void inode_semtake(void) * Name: inode_semgive * * Description: - * Relinquish exclusive access to the in-memory inode tree (tree_sem). + * Relinquish exclusive access to the in-memory inode tree (g_inode_sem). * ****************************************************************************/ void inode_semgive(void) { - sem_post(&tree_sem); + DEBUGASSERT(g_inode_sem.holder == getpid()); + + /* Is this our last count on the semaphore? */ + + if (g_inode_sem.count > 1) + { + /* No.. just decrement the count */ + + g_inode_sem.count--; + } + + /* Yes.. then we can really release the semaphore */ + + else + { + g_inode_sem.holder = NO_HOLDER; + g_inode_sem.count = 0; + sem_post(&g_inode_sem.sem); + } } /**************************************************************************** @@ -219,7 +280,7 @@ void inode_semgive(void) * and references to its companion nodes. * * Assumptions: - * The caller holds the tree_sem + * The caller holds the g_inode_sem semaphore * ****************************************************************************/