Project

General

Profile

Actions

Bug #785

open

Develop model 32_8 that takes into account mutex_lock_nested, etc.

Added by Alexey Khoroshilov over 11 years ago. Updated over 11 years ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
-
Start date:
02/03/2011
Due date:
% Done:

0%

Estimated time:
Detected in build:
master
Platform:
Published in build:

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.


Related issues 1 (0 open1 closed)

Related to Linux Kernel Safety RuleDB - Feature #3324: 32: Locking a mutex twice or unlocking without prior lockingClosedEvgeny Novikov08/03/2012

Actions
Actions #1

Updated by Vadim Mutilin over 11 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)

Actions #3

Updated by Vadim Mutilin over 11 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!

Actions #4

Updated by Pavel Shved over 11 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)

Actions #5

Updated by Vadim Mutilin over 11 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

Actions #6

Updated by Alexey Khoroshilov over 11 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

Actions #7

Updated by Pavel Shved over 11 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.

Actions #8

Updated by Alexey Khoroshilov over 11 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.
Actions

Also available in: Atom PDF