Bug #785
openDevelop model 32_8 that takes into account mutex_lock_nested, etc.
0%
Description
Absence of mutex_lock_nested leads to false positive at fc_lport.ko.
We should look at mutex interface (see include/linux/mutex.h and Documentation/mutex-design.txt) and implement it in the model:
mutex_lock_killable_nested
mutex_lock_interruptible_nested
etc.
Updated by Vadim Mutilin almost 14 years ago
Nested interface is described in Documentation/lockdep-design.txt.
The purpose of _nested functions is to give tips to the linux kernel Validator which searches for deadlocks. The validator distinguishes the locks by its class (type of the structure). Like our rerouting does! The validator uses the nested tip in case if it can not tell two different objects with the same class (in addition the correct lock order is specified)
Updated by Alexey Khoroshilov almost 14 years ago
Updated by Vadim Mutilin almost 14 years ago
Thanks, trace is really required, because file drivers/scsi/libfc/fc_lport.c does not use mutex_lock_nested directly, there is no call to it.
From the name fc_lport.ko the other files can hardly be determined.
The nested calls were found in the source np_piv.c:
158 list_for_each_entry(vn_port, &n_port->vports, list) { 159 mutex_lock_nested(&vn_port->lp_mutex, LPORT_MUTEX_VN_PORT); 160 __fc_vport_setlink(n_port, vn_port); 161 mutex_unlock(&vn_port->lp_mutex); 162 }
But the BLAST trace does not contain nested calls, because
we set CONFIG_DEBUG_LOCK_ALLOC=false and all nested are replaced by normal calls
mutex.h:
extern void mutex_lock(struct mutex *lock); extern int __must_check mutex_lock_interruptible(struct mutex *lock); extern int __must_check mutex_lock_killable(struct mutex *lock); # define mutex_lock_nested(lock, subclass) mutex_lock(lock) # define mutex_lock_interruptible_nested(lock, subclass) mutex_lock_interruptible(lock) # define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock)
The trace is incorrect due to bug #680, the goto removed
LDV_ERROR: goto LDV_ERROR;
But If it were not cut we have a false unsafe
lock2: different objects with the same field name
mutex_lock(&lport->lp_mutex); mutex_lock_nested(&vn_port->lp_mutex, LPORT_MUTEX_VN_PORT);
But as you can see, the driver developer give us a tip using special nested function with a parameter LPORT_MUTEX_VN_PORT saying that it is the different lock, not the same as &lport->lp_mutex. We definitely should use this tip!
Updated by Pavel Shved almost 14 years ago
A quick fix that comes to my mind is just to reroute nested
calls separately from the "usual" locks. Rerouter should be fixed, I guess, as it supports only unary functions (if I recall correctly)
Updated by Vadim Mutilin almost 14 years ago
The problems with using the tips provided by nested functions:
1. The second parameter subclass may not be a constant. (for now it is not a problem because all 25 drivers use constants)
2. The function mutex_unlock does not have nested version and has no subclass parameter.
A proposed solution:¶
state
add previous lock subclass state variable: prev_subclass_TEMPLATE = 0
mutex_lock_nested
1. increment ldv_lock counter if subclass does not equal to prev_subclass, otherwise fail
2. check that subclass is greater then prev_subclass (they should be acquired in the subclass order)
3. update prev_subclass
mutex_unlock
1. decrement ldv_lock counter if possible, otherwise fail
characteristics of the solution:
+ remove false unsafes when nested functions are used
- do not check the order of unlocks, may miss true unsafes
Updated by Alexey Khoroshilov almost 14 years ago
Вадим Мутилин wrote:
A proposed solution:¶
state
add previous lock subclass state variable: prev_subclass_TEMPLATE = 0
add previous lock subclass state variable: prev_subclass_TEMPLATE = -1
mutex_lock_nested
1. increment ldv_lock counter if subclass does not equal to prev_subclass, otherwise fail
2. check that subclass is greater then prev_subclass (they should be acquired in the subclass order)
3. update prev_subclass
mutex_lock = mutex_lock_nested(lock,0)
mutex_unlock
1. decrement ldv_lock counter if possible, otherwise fail
2. update prev_subclass to counter of nested locks-1
characteristics of the solution:
+ remove false unsafes when nested functions are used
- do not check the order of unlocks, may miss true unsafes
Updated by Pavel Shved over 13 years ago
- Priority changed from High to Normal
Decided to make it normal. Besides, it won't make us make a quality boost for this model.
Updated by Alexey Khoroshilov over 13 years ago
- Subject changed from Complete 32_7 model: mutex_lock_nested, etc. to Develop model 32_8 that takes into account mutex_lock_nested, etc.