linux-kernel - lockdep: incorrect deadlock warning with two GPIO expanders

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMpxmJUuj5VrzEu4YZUVxr-ZqFU4VGJpjyHimMJug23EC7j=sQ@mail.gmail.com>
Date:   Mon, 12 Sep 2016 13:51:55 +0200
From:   Bartosz Golaszewski <bgolaszewski@...libre.com>
To:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: lockdep: incorrect deadlock warning with two GPIO expanders

I'm trying to figure out a way of getting rid of an incorrect lockdep
deadlock warning, but the issue is not trivial.

In our hardware an I2C multiplexer is controlled by a GPIO provided by
an expander. There's a second expander using the same device driver
(pca953x) on one of the I2C bus segments. The diagram below presents
the setup:

                                               - - - - -
 -------             ---------  Bus segment 1 |         |
|       |           |         |---------------  Devices
|       | SCL/SDA   |         |               |         |
| Linux |-----------| I2C MUX |                - - - - -
|       |    |      |         | Bus segment 2
|       |    |      |         |-------------------
 -------     |       ---------                    |
             |           |                    - - - - -
        ------------     | MUX GPIO          |         |
       |            |    |                     Devices
       |    GPIO    |    |                   |         |
       | Expander 1 |----                     - - - - -
       |            |                             |
        ------------                              | SCL/SDA
                                                  |
                                             ------------
                                            |            |
                                            |    GPIO    |
                                            | Expander 2 |
                                            |            |
                                             ------------

Lockdep prints a deadlock message when trying to set the direction or
get/set the value of any GPIO provided by the second expander.

The reason for the warning is that we take the chip->i2c_lock in
pca953x_gpio_set/get_value() or pca953x_gpio_direction_input/output()
and then come right back to pca953x_gpio_set_value() when the GPIO mux
kicks in. The call path is as follows (starting from a fixed regulator
controlled by a GPIO provided by the second expander):

  regulator_register()
  |_ _regulator_do_enable()
     |_ pca953x_gpio_set_value() <- mutex_lock
        |_ pca953x_write_single()
           |_ i2c_smbus_write_byte_data()
              |_ i2c_smbus_xfer()
                 |_ i2c_transfer()
                    |_ __i2c_transfer()
                       |_ i2c_mux_master_xfer()
                          |_ i2c_mux_gpio_select()
                             |_ i2c_mux_gpio_set()
                                |_ pca953x_gpio_set_value() <- 2nd mutex_lock

 The locks actually protect different expanders, but lockdep doesn't
see this and says:

  Possible unsafe locking scenario:

        CPU0
        ----
   lock(&chip->i2c_lock);
   lock(&chip->i2c_lock);

  *** DEADLOCK ***

  May be due to missing lock nesting notation

Using mutex_lock_nested(&chip->i2c_lock, SINGLE_DEPTH_NESTING) in
pca953x_gpio_get_value() and pca953x_gpio_direction_input/output()
helps for reading the values or setting the direction, but doesn't do
anything if used in pca953x_gpio_set_value() since we still end up
taking the lock of the same subclass again.

It would require some nasty hacks to figure out that a GPIO is being
used by an I2C mux if we wanted to explicitly provide a different
sublass in this case, but that would not fix the culprit, since the
same problem would occur in other gpio drivers under similar
circumstances.

It seems the problem is with the way lockdep works, but I lack the
knowledge to propose any solution.

Any help & ideas are appreciated.

Best regards,
Bartosz Golaszewski