Sierra QMI drivers S2.11N2.18 serial->disc_mutex warning

IoT Modules MC/EM Series
1

As a word of warning to those looking to use the S2.11N2.18 recently released, I believe that there is a bug in the locking of disc_mutex in the GobiSerial driver whereby a race condition is introduced with a second thread that is holding the mutex. Use this version with extreme caution.

Specifically, this is the code segment that has me concerned:

$ vim GobiSerial/GobiSerial.c
...
 346    /* only send down the usb control message if enabled */
 347    if (serial->dev && flow_control) {
 348       int already_locked = mutex_is_locked(&serial->disc_mutex);     <<<
 349       if (!already_locked)                                           
 350           mutex_lock(&serial->disc_mutex);                           
...

The problem is that other threads can lock serial->disc_mutex prior to the Gobi_dtr_rts() function being called. This results in a race condition where Gobi_dtr_rts() can improperly ignore the existing lock on serial->disc_mutex.

I welcome any data or analysis that proves this concern false, but a grep of “disc_mutex” in the drivers directory of a vanilla kernel’s source shows that it is locked and unlocked several different places in the usbserial hierarchy:

$ grep -R disc_mutex kernel-3.4.42/drivers/usb/serial/
drivers/usb/serial/usb-serial.c: * disconnected, return with its disc_mutex held and its refcount
drivers/usb/serial/usb-serial.c:		mutex_lock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:			mutex_unlock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_unlock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_unlock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_lock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_unlock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_lock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_unlock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:		mutex_unlock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_init(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_lock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_unlock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_lock(&serial->disc_mutex);
drivers/usb/serial/usb-serial.c:	mutex_unlock(&serial->disc_mutex);

Chris

2

Hi,

Your analysis is correct. The mentioned code only checks for the proper lock. It cannot check for improper lock like abruptly closing minicom.

Thanks