On Sun, Jan 29, 2012 at 10:29:24PM +0100, Jesper Juhl wrote: > The Coverity checker noticed a path through nfsd4_lock() where we call > release_lockowner(lock_sop); (at the 'out:' label) where 'lock_sop' is > NULL. > That goes bad since release_lockowner() calls unhash_lockowner() which > dereferences its argument. > release_lockowner() also calls nfs4_free_lockowner(), but that's not a > problem since that function just calls kfree() and kmem_cache_free(), > both of which are safe to call with NULL as argument. > > There are several ways to fix the bug. > - rework nfsd4_lock() so the call to release_lockowner(NULL) will never happen. > - let release_lockowner() test for NULL and return if given one. > - let unhash_lockowner() test for NULL and return if given one (which makes both it and release_lockowner() safe). > > I chose the last option for this patch. > > For information, the path Coverity spotted (in defect report 201504) is this: > ... > [...] > 4098out: > At conditional (12): "status" taking the true branch. > At conditional (13): "new_state" taking the true branch. > 4099 if (status && new_state) new_state is initialized false, and referenced otherwise only once, when a reference ot is is passed here: > 4010 status = lookup_or_create_lock_state(cstate, open_stp, lock, > 4011 &lock_stp, &new_state); so if new_state is true, then lookup_or_create_lock_state() set it to true. But it sets that only in one spot, at the end: *new = true; return nfs_ok; Note nfs_ok is zero. Therefore: > At conditional (11): "status" taking the true branch. > 4012 if (status) > 4013 goto out; this could not have happened. So it looks like a Coverity bug. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
