LinuxLists.cc - [PATCH v4 1/3] vt: fix check for system/busy console drivers when unregistering them

[permalink] [raw]

Subject: [PATCH v4 2/3] vt: fix locking around vt_bind/vt_unbind

Currently vt_bind and vt_unbind access at least the con_driver object
and registered_con_driver array without holding the console lock. Fix
this by locking around the whole function in each case.

Signed-off-by: Imre Deak <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
---
drivers/tty/vt/vt.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 9c046fb..5d36c23 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3318,11 +3318,8 @@ static int vt_bind(struct con_driver *con)
if (first == 0 && last == MAX_NR_CONSOLES -1)
deflt = 1;

- if (first != -1) {
- console_lock();
+ if (first != -1)
do_bind_con_driver(csw, first, last, deflt);
- console_unlock();
- }

first = -1;
last = -1;
@@ -3362,9 +3359,7 @@ static int vt_unbind(struct con_driver *con)
deflt = 1;

if (first != -1) {
- console_lock();
ret = do_unbind_con_driver(csw, first, last, deflt);
- console_unlock();
if (ret != 0)
return ret;
}
@@ -3394,11 +3389,15 @@ static ssize_t store_bind(struct device *dev, struct device_attribute *attr,
struct con_driver *con = dev_get_drvdata(dev);
int bind = simple_strtoul(buf, NULL, 0);

+ console_lock();
+
if (bind)
vt_bind(con);
else
vt_unbind(con);

+ console_unlock();
+
return count;
}

--
1.8.4

2014-12-15 22:18:19

[permalink] [raw]

Subject: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

Currently there is a lock order problem between the console lock and the
kernfs s_active lock of the console driver's bind sysfs entry. When
writing to the sysfs entry the lock order is first s_active then console
lock, when unregistering the console driver via
do_unregister_con_driver() the order is the opposite. See the below
bugzilla reference for one instance of a lockdep backtrace.

Fix this by unregistering the console driver from a deferred work, where
we can safely drop the console lock while unregistering the device and
corresponding sysfs entries (which in turn acquire s_active). Note that
we have to keep the console driver slot in the registered_con_driver
array reserved for the driver that's being unregistered until it's fully
removed. Otherwise a concurrent call to do_register_con_driver could
try to reuse the same slot and fail when registering the corresponding
device with a minor index that's still in use.

Note that the referenced bug report contains two dmesg logs with two
distinct lockdep reports: [1] is about a locking scenario involving
s_active, console_lock and the fb_notifier list lock, while [2] is
about a locking scenario involving only s_active and console_lock.
In [1] locking fb_notifier triggers the lockdep warning only because
of its dependence on console_lock, otherwise case [1] is the same
s_active<->console_lock dependency problem fixed by this patch.
Before this change we have the following locking scenarios involving
the 3 locks:

a) via do_unregister_framebuffer()->...->do_unregister_con_driver():
1. console lock 2. fb_notifier lock 3. s_active lock
b) for example via give_up_console()->do_unregister_con_driver():
1. console lock 2. s_active lock
c) via vt_bind()/vt_unbind():
1. s_active lock 2. console lock

Since c) is the console bind sysfs entry's write code path we can't
change the locking order there. We can only fix this issue by removing
s_active's dependence on the other two locks in a) and b). We can do
this only in the vt code which owns the corresponding sysfs entry, so
that after the change we have:

a) 1. console lock 2. fb_notifier lock
b) 1. console lock
c) 1. s_active lock 2. console lock
d) in the new con_driver_unregister_callback():
1. s_active lock

[1] https://bugs.freedesktop.org/attachment.cgi?id=87716
[2] https://bugs.freedesktop.org/attachment.cgi?id=107602

v2:
- get console_lock earlier in con_driver_unregister_callback(), so we
protect the following console driver field assignments there
- add code coment explaining the reason for deferring the sysfs entry
removal
- add a third paragraph to the commit message explaining why there are
two distinct lockdep reports and that this issue is independent of
fb/fbcon. (Peter Hurley)
v3:
- clarify further the third paragraph
v4:
- rebased on v4 of patch 1/3

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=70523
Signed-off-by: Imre Deak <[email protected]>
---
drivers/tty/vt/vt.c | 61 ++++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 51 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 5d36c23..921854e 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -108,6 +108,7 @@
#define CON_DRIVER_FLAG_MODULE 1
#define CON_DRIVER_FLAG_INIT 2
#define CON_DRIVER_FLAG_ATTR 4
+#define CON_DRIVER_FLAG_ZOMBIE 8

struct con_driver {
const struct consw *con;
@@ -153,6 +154,7 @@ static int set_vesa_blanking(char __user *p);
static void set_cursor(struct vc_data *vc);
static void hide_cursor(struct vc_data *vc);
static void console_callback(struct work_struct *ignored);
+static void con_driver_unregister_callback(struct work_struct *ignored);
static void blank_screen_t(unsigned long dummy);
static void set_palette(struct vc_data *vc);

@@ -182,6 +184,7 @@ static int blankinterval = 10*60;
core_param(consoleblank, blankinterval, int, 0444);

static DECLARE_WORK(console_work, console_callback);
+static DECLARE_WORK(con_driver_unregister_work, con_driver_unregister_callback);

/*
* fg_console is the current virtual console,
@@ -3603,7 +3606,8 @@ static int do_register_con_driver(const struct consw *csw, int first, int last)
for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
con_driver = &registered_con_driver[i];

- if (con_driver->con == NULL) {
+ if (con_driver->con == NULL &&
+ !(con_driver->flag & CON_DRIVER_FLAG_ZOMBIE)) {
con_driver->con = csw;
con_driver->desc = desc;
con_driver->node = i;
@@ -3665,16 +3669,20 @@ int do_unregister_con_driver(const struct consw *csw)
struct con_driver *con_driver = &registered_con_driver[i];

if (con_driver->con == csw) {
- vtconsole_deinit_device(con_driver);
- device_destroy(vtconsole_class,
- MKDEV(0, con_driver->node));
+ /*
+ * Defer the removal of the sysfs entries since that
+ * will acquire the kernfs s_active lock and we can't
+ * acquire this lock while holding the console lock:
+ * the unbind sysfs entry imposes already the opposite
+ * order. Reset con already here to prevent any later
+ * lookup to succeed and mark this slot as zombie, so
+ * it won't get reused until we complete the removal
+ * in the deferred work.
+ */
con_driver->con = NULL;
- con_driver->desc = NULL;
- con_driver->dev = NULL;
- con_driver->node = 0;
- con_driver->flag = 0;
- con_driver->first = 0;
- con_driver->last = 0;
+ con_driver->flag = CON_DRIVER_FLAG_ZOMBIE;
+ schedule_work(&con_driver_unregister_work);
+
return 0;
}
}
@@ -3683,6 +3691,39 @@ int do_unregister_con_driver(const struct consw *csw)
}
EXPORT_SYMBOL_GPL(do_unregister_con_driver);

+static void con_driver_unregister_callback(struct work_struct *ignored)
+{
+ int i;
+
+ console_lock();
+
+ for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
+ struct con_driver *con_driver = &registered_con_driver[i];
+
+ if (!(con_driver->flag & CON_DRIVER_FLAG_ZOMBIE))
+ continue;
+
+ console_unlock();
+
+ vtconsole_deinit_device(con_driver);
+ device_destroy(vtconsole_class, MKDEV(0, con_driver->node));
+
+ console_lock();
+
+ if (WARN_ON_ONCE(con_driver->con))
+ con_driver->con = NULL;
+ con_driver->desc = NULL;
+ con_driver->dev = NULL;
+ con_driver->node = 0;
+ WARN_ON_ONCE(con_driver->flag != CON_DRIVER_FLAG_ZOMBIE);
+ con_driver->flag = 0;
+ con_driver->first = 0;
+ con_driver->last = 0;
+ }
+
+ console_unlock();
+}
+
/*
* If we support more console drivers, this function is used
* when a driver wants to take over some existing consoles
--
1.8.4

2014-12-16 07:34:40

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] vt: fix check for system/busy console drivers when unregistering them

On Tue, Dec 16, 2014 at 12:15:59AM +0200, Imre Deak wrote:
> The default console driver (conswitchp) and busy drivers bound to a
> console (as reported by con_is_bound()) shouldn't be unregistered.
> System console drivers (without the CON_DRIVER_FLAG_MODULE flag) can be
> unregistered, provided they are neither default nor busy. The current
> code checks for the CON_DRIVER_FLAG_INIT flag but this doesn't make
> sense: this flag is set for a driver whenever its associated console's
> con_startup() function is called, which first happens when the console
> driver is registered (so before the console gets bound) and gets cleared
> when the console gets unbound. The purpose of this flag is to show if we
> need to call con_startup() on a console before we use it.
>
> Based on the above, do_unregister_con_driver() in its current form will
> allow unregistering a console driver only if it was never bound, but
> will refuse to unregister one that was bound and later unbound.
>
> Fix this by dropping the CON_DRIVER_FLAG_INIT check, allowing
> unregistering of any console driver provided that it's not the default
> one and it's not busy.
>
> v2:
> - reword the third paragraph to clarify how the fix works (Peter Hurley)
> v3:
> - unchanged
> v4:
> - Allow unregistering a system console driver too, needed by i915 to
> unregister vgacon. Update commit description accordingly. (Daniel)
>
> Signed-off-by: Imre Deak <[email protected]>

You've successufully tricked me to again look at vt.c. Argh!!

Reviewed-by: Daniel Vetter <[email protected]>

> ---
> drivers/tty/vt/vt.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index f3fbbbc..9c046fb 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3665,8 +3665,7 @@ int do_unregister_con_driver(const struct consw *csw)
> for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
> struct con_driver *con_driver = &registered_con_driver[i];
>
> - if (con_driver->con == csw &&
> - con_driver->flag & CON_DRIVER_FLAG_INIT) {
> + if (con_driver->con == csw) {
> vtconsole_deinit_device(con_driver);
> device_destroy(vtconsole_class,
> MKDEV(0, con_driver->node));
> --
> 1.8.4
>

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2014-12-16 07:37:25

[permalink] [raw]

Subject: Re: [PATCH v4 2/3] vt: fix locking around vt_bind/vt_unbind

On Tue, Dec 16, 2014 at 12:16:00AM +0200, Imre Deak wrote:
> Currently vt_bind and vt_unbind access at least the con_driver object
> and registered_con_driver array without holding the console lock. Fix
> this by locking around the whole function in each case.
>
> Signed-off-by: Imre Deak <[email protected]>
> Reviewed-by: Peter Hurley <[email protected]>

Well it's not pretty that console_lock is constantly growing and getting
pushed up the lock dep chain the past few years for various reasons, but
this here looks genuine and won't wrap new locks in console_lock.

Reviewed-by: Daniel Vetter <[email protected]>

> ---
> drivers/tty/vt/vt.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 9c046fb..5d36c23 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3318,11 +3318,8 @@ static int vt_bind(struct con_driver *con)
> if (first == 0 && last == MAX_NR_CONSOLES -1)
> deflt = 1;
>
> - if (first != -1) {
> - console_lock();
> + if (first != -1)
> do_bind_con_driver(csw, first, last, deflt);
> - console_unlock();
> - }
>
> first = -1;
> last = -1;
> @@ -3362,9 +3359,7 @@ static int vt_unbind(struct con_driver *con)
> deflt = 1;
>
> if (first != -1) {
> - console_lock();
> ret = do_unbind_con_driver(csw, first, last, deflt);
> - console_unlock();
> if (ret != 0)
> return ret;
> }
> @@ -3394,11 +3389,15 @@ static ssize_t store_bind(struct device *dev, struct device_attribute *attr,
> struct con_driver *con = dev_get_drvdata(dev);
> int bind = simple_strtoul(buf, NULL, 0);
>
> + console_lock();
> +
> if (bind)
> vt_bind(con);
> else
> vt_unbind(con);
>
> + console_unlock();
> +
> return count;
> }
>
> --
> 1.8.4
>

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2014-12-16 07:52:27

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On Tue, Dec 16, 2014 at 12:16:01AM +0200, Imre Deak wrote:
> Currently there is a lock order problem between the console lock and the
> kernfs s_active lock of the console driver's bind sysfs entry. When
> writing to the sysfs entry the lock order is first s_active then console
> lock, when unregistering the console driver via
> do_unregister_con_driver() the order is the opposite. See the below
> bugzilla reference for one instance of a lockdep backtrace.
>
> Fix this by unregistering the console driver from a deferred work, where
> we can safely drop the console lock while unregistering the device and
> corresponding sysfs entries (which in turn acquire s_active). Note that
> we have to keep the console driver slot in the registered_con_driver
> array reserved for the driver that's being unregistered until it's fully
> removed. Otherwise a concurrent call to do_register_con_driver could
> try to reuse the same slot and fail when registering the corresponding
> device with a minor index that's still in use.
>
> Note that the referenced bug report contains two dmesg logs with two
> distinct lockdep reports: [1] is about a locking scenario involving
> s_active, console_lock and the fb_notifier list lock, while [2] is
> about a locking scenario involving only s_active and console_lock.
> In [1] locking fb_notifier triggers the lockdep warning only because
> of its dependence on console_lock, otherwise case [1] is the same
> s_active<->console_lock dependency problem fixed by this patch.
> Before this change we have the following locking scenarios involving
> the 3 locks:
>
> a) via do_unregister_framebuffer()->...->do_unregister_con_driver():
> 1. console lock 2. fb_notifier lock 3. s_active lock
> b) for example via give_up_console()->do_unregister_con_driver():
> 1. console lock 2. s_active lock
> c) via vt_bind()/vt_unbind():
> 1. s_active lock 2. console lock
>
> Since c) is the console bind sysfs entry's write code path we can't
> change the locking order there. We can only fix this issue by removing
> s_active's dependence on the other two locks in a) and b). We can do
> this only in the vt code which owns the corresponding sysfs entry, so
> that after the change we have:
>
> a) 1. console lock 2. fb_notifier lock
> b) 1. console lock
> c) 1. s_active lock 2. console lock
> d) in the new con_driver_unregister_callback():
> 1. s_active lock
>
> [1] https://bugs.freedesktop.org/attachment.cgi?id=87716
> [2] https://bugs.freedesktop.org/attachment.cgi?id=107602
>
> v2:
> - get console_lock earlier in con_driver_unregister_callback(), so we
> protect the following console driver field assignments there
> - add code coment explaining the reason for deferring the sysfs entry
> removal
> - add a third paragraph to the commit message explaining why there are
> two distinct lockdep reports and that this issue is independent of
> fb/fbcon. (Peter Hurley)
> v3:
> - clarify further the third paragraph
> v4:
> - rebased on v4 of patch 1/3
>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> Signed-off-by: Imre Deak <[email protected]>

So the normal/simple solution would be to remove the con driver from the
registration while holding required locks, but destroy sysfs stuff after
the critical section.

The problem is that console unbind/bind/reg/unreg are all done with the
locks held already, and the reason for that is the fbcon/fbdev notifier
chain. You can read up on the story by chasing the commits that added the
various locked do_* functions over the past years.

Short story is that the notifier chain has it's own mutex and any call
mediated through it introces that lock into the chain, which creates a
massive pile of additional depencies. The only solution is to grab _all_
required locks outside of that notifier chain, which means we've spent a
lot of patches growing the scope of console_lock. Which is bad, since
anything holding console_lock can't get dmesg lines out when it dies.

This patch here is another step into the wrong direction imo. It's also
for a feature that mere users should never use (even though it's in
sysfs). Heck it's a regression which has been introduced by pulling
console_lock out&up, before that effort sysfs files worked correctly and
implemented locking as described.

The right solution imo here is to at least cut up the fbdev notifier into
the different uses so that the locking artificial locking inversions go
away. Most of the calls are used to go from fbdev core (fbmem.c) to the
fbcon.c. Apparently someone though it would be great to allow fb.ko and
fbcon.ko to be able to load in any order whatsoever. Then there's various
other calls that go the other direction (e.g. fbcon calling into backlight
logic) and those introduce the locking inversion. So if we split things
into 2 notifier chais, one to exclusively call into fbcon and the other
for everything else we could revert all the patche that move console_lock
out and fix this bug here properly.

So NACK from me for this.
-Daniel

> ---
> drivers/tty/vt/vt.c | 61 ++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 51 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 5d36c23..921854e 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -108,6 +108,7 @@
> #define CON_DRIVER_FLAG_MODULE 1
> #define CON_DRIVER_FLAG_INIT 2
> #define CON_DRIVER_FLAG_ATTR 4
> +#define CON_DRIVER_FLAG_ZOMBIE 8
>
> struct con_driver {
> const struct consw *con;
> @@ -153,6 +154,7 @@ static int set_vesa_blanking(char __user *p);
> static void set_cursor(struct vc_data *vc);
> static void hide_cursor(struct vc_data *vc);
> static void console_callback(struct work_struct *ignored);
> +static void con_driver_unregister_callback(struct work_struct *ignored);
> static void blank_screen_t(unsigned long dummy);
> static void set_palette(struct vc_data *vc);
>
> @@ -182,6 +184,7 @@ static int blankinterval = 10*60;
> core_param(consoleblank, blankinterval, int, 0444);
>
> static DECLARE_WORK(console_work, console_callback);
> +static DECLARE_WORK(con_driver_unregister_work, con_driver_unregister_callback);
>
> /*
> * fg_console is the current virtual console,
> @@ -3603,7 +3606,8 @@ static int do_register_con_driver(const struct consw *csw, int first, int last)
> for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
> con_driver = &registered_con_driver[i];
>
> - if (con_driver->con == NULL) {
> + if (con_driver->con == NULL &&
> + !(con_driver->flag & CON_DRIVER_FLAG_ZOMBIE)) {
> con_driver->con = csw;
> con_driver->desc = desc;
> con_driver->node = i;
> @@ -3665,16 +3669,20 @@ int do_unregister_con_driver(const struct consw *csw)
> struct con_driver *con_driver = &registered_con_driver[i];
>
> if (con_driver->con == csw) {
> - vtconsole_deinit_device(con_driver);
> - device_destroy(vtconsole_class,
> - MKDEV(0, con_driver->node));
> + /*
> + * Defer the removal of the sysfs entries since that
> + * will acquire the kernfs s_active lock and we can't
> + * acquire this lock while holding the console lock:
> + * the unbind sysfs entry imposes already the opposite
> + * order. Reset con already here to prevent any later
> + * lookup to succeed and mark this slot as zombie, so
> + * it won't get reused until we complete the removal
> + * in the deferred work.
> + */
> con_driver->con = NULL;
> - con_driver->desc = NULL;
> - con_driver->dev = NULL;
> - con_driver->node = 0;
> - con_driver->flag = 0;
> - con_driver->first = 0;
> - con_driver->last = 0;
> + con_driver->flag = CON_DRIVER_FLAG_ZOMBIE;
> + schedule_work(&con_driver_unregister_work);
> +
> return 0;
> }
> }
> @@ -3683,6 +3691,39 @@ int do_unregister_con_driver(const struct consw *csw)
> }
> EXPORT_SYMBOL_GPL(do_unregister_con_driver);
>
> +static void con_driver_unregister_callback(struct work_struct *ignored)
> +{
> + int i;
> +
> + console_lock();
> +
> + for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
> + struct con_driver *con_driver = &registered_con_driver[i];
> +
> + if (!(con_driver->flag & CON_DRIVER_FLAG_ZOMBIE))
> + continue;
> +
> + console_unlock();
> +
> + vtconsole_deinit_device(con_driver);
> + device_destroy(vtconsole_class, MKDEV(0, con_driver->node));
> +
> + console_lock();
> +
> + if (WARN_ON_ONCE(con_driver->con))
> + con_driver->con = NULL;
> + con_driver->desc = NULL;
> + con_driver->dev = NULL;
> + con_driver->node = 0;
> + WARN_ON_ONCE(con_driver->flag != CON_DRIVER_FLAG_ZOMBIE);
> + con_driver->flag = 0;
> + con_driver->first = 0;
> + con_driver->last = 0;
> + }
> +
> + console_unlock();
> +}
> +
> /*
> * If we support more console drivers, this function is used
> * when a driver wants to take over some existing consoles
> --
> 1.8.4
>

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2014-12-16 10:23:30

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On Tue, 2014-12-16 at 08:53 +0100, Daniel Vetter wrote:
> On Tue, Dec 16, 2014 at 12:16:01AM +0200, Imre Deak wrote:
> > Currently there is a lock order problem between the console lock and the
> > kernfs s_active lock of the console driver's bind sysfs entry. When
> > writing to the sysfs entry the lock order is first s_active then console
> > lock, when unregistering the console driver via
> > do_unregister_con_driver() the order is the opposite. See the below
> > bugzilla reference for one instance of a lockdep backtrace.
> >
> > Fix this by unregistering the console driver from a deferred work, where
> > we can safely drop the console lock while unregistering the device and
> > corresponding sysfs entries (which in turn acquire s_active). Note that
> > we have to keep the console driver slot in the registered_con_driver
> > array reserved for the driver that's being unregistered until it's fully
> > removed. Otherwise a concurrent call to do_register_con_driver could
> > try to reuse the same slot and fail when registering the corresponding
> > device with a minor index that's still in use.
> >
> > Note that the referenced bug report contains two dmesg logs with two
> > distinct lockdep reports: [1] is about a locking scenario involving
> > s_active, console_lock and the fb_notifier list lock, while [2] is
> > about a locking scenario involving only s_active and console_lock.
> > In [1] locking fb_notifier triggers the lockdep warning only because
> > of its dependence on console_lock, otherwise case [1] is the same
> > s_active<->console_lock dependency problem fixed by this patch.
> > Before this change we have the following locking scenarios involving
> > the 3 locks:
> >
> > a) via do_unregister_framebuffer()->...->do_unregister_con_driver():
> > 1. console lock 2. fb_notifier lock 3. s_active lock
> > b) for example via give_up_console()->do_unregister_con_driver():
> > 1. console lock 2. s_active lock
> > c) via vt_bind()/vt_unbind():
> > 1. s_active lock 2. console lock
> >
> > Since c) is the console bind sysfs entry's write code path we can't
> > change the locking order there. We can only fix this issue by removing
> > s_active's dependence on the other two locks in a) and b). We can do
> > this only in the vt code which owns the corresponding sysfs entry, so
> > that after the change we have:
> >
> > a) 1. console lock 2. fb_notifier lock
> > b) 1. console lock
> > c) 1. s_active lock 2. console lock
> > d) in the new con_driver_unregister_callback():
> > 1. s_active lock
> >
> > [1] https://bugs.freedesktop.org/attachment.cgi?id=87716
> > [2] https://bugs.freedesktop.org/attachment.cgi?id=107602
> >
> > v2:
> > - get console_lock earlier in con_driver_unregister_callback(), so we
> > protect the following console driver field assignments there
> > - add code coment explaining the reason for deferring the sysfs entry
> > removal
> > - add a third paragraph to the commit message explaining why there are
> > two distinct lockdep reports and that this issue is independent of
> > fb/fbcon. (Peter Hurley)
> > v3:
> > - clarify further the third paragraph
> > v4:
> > - rebased on v4 of patch 1/3
> >
> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> > Signed-off-by: Imre Deak <[email protected]>
>
> So the normal/simple solution would be to remove the con driver from the
> registration while holding required locks, but destroy sysfs stuff after
> the critical section.
>
> The problem is that console unbind/bind/reg/unreg are all done with the
> locks held already, and the reason for that is the fbcon/fbdev notifier
> chain. You can read up on the story by chasing the commits that added the
> various locked do_* functions over the past years.
>
> Short story is that the notifier chain has it's own mutex and any call
> mediated through it introces that lock into the chain, which creates a
> massive pile of additional depencies. The only solution is to grab _all_
> required locks outside of that notifier chain, which means we've spent a
> lot of patches growing the scope of console_lock. Which is bad, since
> anything holding console_lock can't get dmesg lines out when it dies.

These are independent issues from what this patch fixes. It doesn't try
to grow the scope of console_lock either, it makes the sysfs s_active
lock independent of console_lock.

> This patch here is another step into the wrong direction imo. It's also
> for a feature that mere users should never use (even though it's in
> sysfs). Heck it's a regression which has been introduced by pulling
> console_lock out&up, before that effort sysfs files worked correctly and
> implemented locking as described.

It doesn't matter where you take console_lock, since it's fixed where
s_active is taken (in the vfs code before the sysfs entry's write hook
is called). So again this issue is not related to the growing scope of
console_lock. This fix makes the s_active lock independent of
console_lock, so I don't see why it's a step in the wrong direction.

> The right solution imo here is to at least cut up the fbdev notifier into
> the different uses so that the locking artificial locking inversions go
> away. Most of the calls are used to go from fbdev core (fbmem.c) to the
> fbcon.c. Apparently someone though it would be great to allow fb.ko and
> fbcon.ko to be able to load in any order whatsoever. Then there's various
> other calls that go the other direction (e.g. fbcon calling into backlight
> logic) and those introduce the locking inversion. So if we split things
> into 2 notifier chais, one to exclusively call into fbcon and the other
> for everything else we could revert all the patche that move console_lock
> out and fix this bug here properly.
>
> So NACK from me for this.

I think I proved it in the commit message that this issue is independent
of fbcon/fbdev, so refactoring these will not solve it. This patch fixes
an issue in vt and while your points may be valid, they are not really
about this issue or how the patch fixes it.

--Imre

> -Daniel
>
> > ---
> > drivers/tty/vt/vt.c | 61 ++++++++++++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 51 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index 5d36c23..921854e 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -108,6 +108,7 @@
> > #define CON_DRIVER_FLAG_MODULE 1
> > #define CON_DRIVER_FLAG_INIT 2
> > #define CON_DRIVER_FLAG_ATTR 4
> > +#define CON_DRIVER_FLAG_ZOMBIE 8
> >
> > struct con_driver {
> > const struct consw *con;
> > @@ -153,6 +154,7 @@ static int set_vesa_blanking(char __user *p);
> > static void set_cursor(struct vc_data *vc);
> > static void hide_cursor(struct vc_data *vc);
> > static void console_callback(struct work_struct *ignored);
> > +static void con_driver_unregister_callback(struct work_struct *ignored);
> > static void blank_screen_t(unsigned long dummy);
> > static void set_palette(struct vc_data *vc);
> >
> > @@ -182,6 +184,7 @@ static int blankinterval = 10*60;
> > core_param(consoleblank, blankinterval, int, 0444);
> >
> > static DECLARE_WORK(console_work, console_callback);
> > +static DECLARE_WORK(con_driver_unregister_work, con_driver_unregister_callback);
> >
> > /*
> > * fg_console is the current virtual console,
> > @@ -3603,7 +3606,8 @@ static int do_register_con_driver(const struct consw *csw, int first, int last)
> > for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
> > con_driver = &registered_con_driver[i];
> >
> > - if (con_driver->con == NULL) {
> > + if (con_driver->con == NULL &&
> > + !(con_driver->flag & CON_DRIVER_FLAG_ZOMBIE)) {
> > con_driver->con = csw;
> > con_driver->desc = desc;
> > con_driver->node = i;
> > @@ -3665,16 +3669,20 @@ int do_unregister_con_driver(const struct consw *csw)
> > struct con_driver *con_driver = &registered_con_driver[i];
> >
> > if (con_driver->con == csw) {
> > - vtconsole_deinit_device(con_driver);
> > - device_destroy(vtconsole_class,
> > - MKDEV(0, con_driver->node));
> > + /*
> > + * Defer the removal of the sysfs entries since that
> > + * will acquire the kernfs s_active lock and we can't
> > + * acquire this lock while holding the console lock:
> > + * the unbind sysfs entry imposes already the opposite
> > + * order. Reset con already here to prevent any later
> > + * lookup to succeed and mark this slot as zombie, so
> > + * it won't get reused until we complete the removal
> > + * in the deferred work.
> > + */
> > con_driver->con = NULL;
> > - con_driver->desc = NULL;
> > - con_driver->dev = NULL;
> > - con_driver->node = 0;
> > - con_driver->flag = 0;
> > - con_driver->first = 0;
> > - con_driver->last = 0;
> > + con_driver->flag = CON_DRIVER_FLAG_ZOMBIE;
> > + schedule_work(&con_driver_unregister_work);
> > +
> > return 0;
> > }
> > }
> > @@ -3683,6 +3691,39 @@ int do_unregister_con_driver(const struct consw *csw)
> > }
> > EXPORT_SYMBOL_GPL(do_unregister_con_driver);
> >
> > +static void con_driver_unregister_callback(struct work_struct *ignored)
> > +{
> > + int i;
> > +
> > + console_lock();
> > +
> > + for (i = 0; i < MAX_NR_CON_DRIVER; i++) {
> > + struct con_driver *con_driver = &registered_con_driver[i];
> > +
> > + if (!(con_driver->flag & CON_DRIVER_FLAG_ZOMBIE))
> > + continue;
> > +
> > + console_unlock();
> > +
> > + vtconsole_deinit_device(con_driver);
> > + device_destroy(vtconsole_class, MKDEV(0, con_driver->node));
> > +
> > + console_lock();
> > +
> > + if (WARN_ON_ONCE(con_driver->con))
> > + con_driver->con = NULL;
> > + con_driver->desc = NULL;
> > + con_driver->dev = NULL;
> > + con_driver->node = 0;
> > + WARN_ON_ONCE(con_driver->flag != CON_DRIVER_FLAG_ZOMBIE);
> > + con_driver->flag = 0;
> > + con_driver->first = 0;
> > + con_driver->last = 0;
> > + }
> > +
> > + console_unlock();
> > +}
> > +
> > /*
> > * If we support more console drivers, this function is used
> > * when a driver wants to take over some existing consoles
> > --
> > 1.8.4
> >
>

2014-12-16 12:56:12

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On 12/16/2014 05:23 AM, Imre Deak wrote:
> On Tue, 2014-12-16 at 08:53 +0100, Daniel Vetter wrote:
>> On Tue, Dec 16, 2014 at 12:16:01AM +0200, Imre Deak wrote:
>>> Currently there is a lock order problem between the console lock and the
>>> kernfs s_active lock of the console driver's bind sysfs entry. When
>>> writing to the sysfs entry the lock order is first s_active then console
>>> lock, when unregistering the console driver via
>>> do_unregister_con_driver() the order is the opposite. See the below
>>> bugzilla reference for one instance of a lockdep backtrace.
>>>
>>> Fix this by unregistering the console driver from a deferred work, where
>>> we can safely drop the console lock while unregistering the device and
>>> corresponding sysfs entries (which in turn acquire s_active). Note that
>>> we have to keep the console driver slot in the registered_con_driver
>>> array reserved for the driver that's being unregistered until it's fully
>>> removed. Otherwise a concurrent call to do_register_con_driver could
>>> try to reuse the same slot and fail when registering the corresponding
>>> device with a minor index that's still in use.
>>>
>>> Note that the referenced bug report contains two dmesg logs with two
>>> distinct lockdep reports: [1] is about a locking scenario involving
>>> s_active, console_lock and the fb_notifier list lock, while [2] is
>>> about a locking scenario involving only s_active and console_lock.
>>> In [1] locking fb_notifier triggers the lockdep warning only because
>>> of its dependence on console_lock, otherwise case [1] is the same
>>> s_active<->console_lock dependency problem fixed by this patch.
>>> Before this change we have the following locking scenarios involving
>>> the 3 locks:
>>>
>>> a) via do_unregister_framebuffer()->...->do_unregister_con_driver():
>>> 1. console lock 2. fb_notifier lock 3. s_active lock
>>> b) for example via give_up_console()->do_unregister_con_driver():
>>> 1. console lock 2. s_active lock
>>> c) via vt_bind()/vt_unbind():
>>> 1. s_active lock 2. console lock
>>>
>>> Since c) is the console bind sysfs entry's write code path we can't
>>> change the locking order there. We can only fix this issue by removing
>>> s_active's dependence on the other two locks in a) and b). We can do
>>> this only in the vt code which owns the corresponding sysfs entry, so
>>> that after the change we have:
>>>
>>> a) 1. console lock 2. fb_notifier lock
>>> b) 1. console lock
>>> c) 1. s_active lock 2. console lock
>>> d) in the new con_driver_unregister_callback():
>>> 1. s_active lock
>>>
>>> [1] https://bugs.freedesktop.org/attachment.cgi?id=87716
>>> [2] https://bugs.freedesktop.org/attachment.cgi?id=107602
>>>
>>> v2:
>>> - get console_lock earlier in con_driver_unregister_callback(), so we
>>> protect the following console driver field assignments there
>>> - add code coment explaining the reason for deferring the sysfs entry
>>> removal
>>> - add a third paragraph to the commit message explaining why there are
>>> two distinct lockdep reports and that this issue is independent of
>>> fb/fbcon. (Peter Hurley)
>>> v3:
>>> - clarify further the third paragraph
>>> v4:
>>> - rebased on v4 of patch 1/3
>>>
>>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=70523
>>> Signed-off-by: Imre Deak <[email protected]>
>>
>> So the normal/simple solution would be to remove the con driver from the
>> registration while holding required locks, but destroy sysfs stuff after
>> the critical section.
>>
>> The problem is that console unbind/bind/reg/unreg are all done with the
>> locks held already, and the reason for that is the fbcon/fbdev notifier
>> chain. You can read up on the story by chasing the commits that added the
>> various locked do_* functions over the past years.
>>
>> Short story is that the notifier chain has it's own mutex and any call
>> mediated through it introces that lock into the chain, which creates a
>> massive pile of additional depencies. The only solution is to grab _all_
>> required locks outside of that notifier chain, which means we've spent a
>> lot of patches growing the scope of console_lock. Which is bad, since
>> anything holding console_lock can't get dmesg lines out when it dies.
>
> These are independent issues from what this patch fixes. It doesn't try
> to grow the scope of console_lock either, it makes the sysfs s_active
> lock independent of console_lock.
>
>> This patch here is another step into the wrong direction imo. It's also
>> for a feature that mere users should never use (even though it's in
>> sysfs). Heck it's a regression which has been introduced by pulling
>> console_lock out&up, before that effort sysfs files worked correctly and
>> implemented locking as described.
>
> It doesn't matter where you take console_lock, since it's fixed where
> s_active is taken (in the vfs code before the sysfs entry's write hook
> is called). So again this issue is not related to the growing scope of
> console_lock. This fix makes the s_active lock independent of
> console_lock, so I don't see why it's a step in the wrong direction.
>
>> The right solution imo here is to at least cut up the fbdev notifier into
>> the different uses so that the locking artificial locking inversions go
>> away. Most of the calls are used to go from fbdev core (fbmem.c) to the
>> fbcon.c. Apparently someone though it would be great to allow fb.ko and
>> fbcon.ko to be able to load in any order whatsoever. Then there's various
>> other calls that go the other direction (e.g. fbcon calling into backlight
>> logic) and those introduce the locking inversion. So if we split things
>> into 2 notifier chais, one to exclusively call into fbcon and the other
>> for everything else we could revert all the patche that move console_lock
>> out and fix this bug here properly.
>>
>> So NACK from me for this.
>
> I think I proved it in the commit message that this issue is independent
> of fbcon/fbdev, so refactoring these will not solve it. This patch fixes
> an issue in vt and while your points may be valid, they are not really
> about this issue or how the patch fixes it.

I think you may have missed Daniel's point. Which is what I was trying to
point out earlier but in an overly terse manner.

If you start by just moving the sysfs teardown outside the console_lock
(but still in the same thread of execution), then the direct lock inversion
between console_lock and the sysfs lock goes away.

However, you'll now realize that you can't move the sysfs teardown outside
the console lock because fbcon is doing teardown from inside its notifier,
which means that there would be a lock inversion between the console lock
and the notifier lock.

Which is why we're pointing out that the real problem here is the
fb notifier call chain lock.

Regards,
Peter Hurley

2014-12-16 13:45:21

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On Tue, Dec 16, 2014 at 1:50 PM, Peter Hurley <[email protected]> wrote:
>>> So NACK from me for this.
>>
>> I think I proved it in the commit message that this issue is independent
>> of fbcon/fbdev, so refactoring these will not solve it. This patch fixes
>> an issue in vt and while your points may be valid, they are not really
>> about this issue or how the patch fixes it.
>
> I think you may have missed Daniel's point. Which is what I was trying to
> point out earlier but in an overly terse manner.
>
> If you start by just moving the sysfs teardown outside the console_lock
> (but still in the same thread of execution), then the direct lock inversion
> between console_lock and the sysfs lock goes away.
>
> However, you'll now realize that you can't move the sysfs teardown outside
> the console lock because fbcon is doing teardown from inside its notifier,
> which means that there would be a lock inversion between the console lock
> and the notifier lock.
>
> Which is why we're pointing out that the real problem here is the
> fb notifier call chain lock.

It's more the overall story. The patch is imo technically ok, but it's
yet another step in the wrong direction. We've unfortunately piled up
lots of those around fbcon/console_lock and eventually we need to
start going a better direction. Applying a hack will just result in
everyone forgetting about this until the next head of the hydra pops
out of the water.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2014-12-16 14:51:09

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On Tue, 2014-12-16 at 07:50 -0500, Peter Hurley wrote:
> On 12/16/2014 05:23 AM, Imre Deak wrote:
> > On Tue, 2014-12-16 at 08:53 +0100, Daniel Vetter wrote:
> >> On Tue, Dec 16, 2014 at 12:16:01AM +0200, Imre Deak wrote:
> >>> Currently there is a lock order problem between the console lock and the
> >>> kernfs s_active lock of the console driver's bind sysfs entry. When
> >>> writing to the sysfs entry the lock order is first s_active then console
> >>> lock, when unregistering the console driver via
> >>> do_unregister_con_driver() the order is the opposite. See the below
> >>> bugzilla reference for one instance of a lockdep backtrace.
> >>>
> >>> Fix this by unregistering the console driver from a deferred work, where
> >>> we can safely drop the console lock while unregistering the device and
> >>> corresponding sysfs entries (which in turn acquire s_active). Note that
> >>> we have to keep the console driver slot in the registered_con_driver
> >>> array reserved for the driver that's being unregistered until it's fully
> >>> removed. Otherwise a concurrent call to do_register_con_driver could
> >>> try to reuse the same slot and fail when registering the corresponding
> >>> device with a minor index that's still in use.
> >>>
> >>> Note that the referenced bug report contains two dmesg logs with two
> >>> distinct lockdep reports: [1] is about a locking scenario involving
> >>> s_active, console_lock and the fb_notifier list lock, while [2] is
> >>> about a locking scenario involving only s_active and console_lock.
> >>> In [1] locking fb_notifier triggers the lockdep warning only because
> >>> of its dependence on console_lock, otherwise case [1] is the same
> >>> s_active<->console_lock dependency problem fixed by this patch.
> >>> Before this change we have the following locking scenarios involving
> >>> the 3 locks:
> >>>
> >>> a) via do_unregister_framebuffer()->...->do_unregister_con_driver():
> >>> 1. console lock 2. fb_notifier lock 3. s_active lock
> >>> b) for example via give_up_console()->do_unregister_con_driver():
> >>> 1. console lock 2. s_active lock
> >>> c) via vt_bind()/vt_unbind():
> >>> 1. s_active lock 2. console lock
> >>>
> >>> Since c) is the console bind sysfs entry's write code path we can't
> >>> change the locking order there. We can only fix this issue by removing
> >>> s_active's dependence on the other two locks in a) and b). We can do
> >>> this only in the vt code which owns the corresponding sysfs entry, so
> >>> that after the change we have:
> >>>
> >>> a) 1. console lock 2. fb_notifier lock
> >>> b) 1. console lock
> >>> c) 1. s_active lock 2. console lock
> >>> d) in the new con_driver_unregister_callback():
> >>> 1. s_active lock
> >>>
> >>> [1] https://bugs.freedesktop.org/attachment.cgi?id=87716
> >>> [2] https://bugs.freedesktop.org/attachment.cgi?id=107602
> >>>
> >>> v2:
> >>> - get console_lock earlier in con_driver_unregister_callback(), so we
> >>> protect the following console driver field assignments there
> >>> - add code coment explaining the reason for deferring the sysfs entry
> >>> removal
> >>> - add a third paragraph to the commit message explaining why there are
> >>> two distinct lockdep reports and that this issue is independent of
> >>> fb/fbcon. (Peter Hurley)
> >>> v3:
> >>> - clarify further the third paragraph
> >>> v4:
> >>> - rebased on v4 of patch 1/3
> >>>
> >>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> >>> Signed-off-by: Imre Deak <[email protected]>
> >>
> >> So the normal/simple solution would be to remove the con driver from the
> >> registration while holding required locks, but destroy sysfs stuff after
> >> the critical section.
> >>
> >> The problem is that console unbind/bind/reg/unreg are all done with the
> >> locks held already, and the reason for that is the fbcon/fbdev notifier
> >> chain. You can read up on the story by chasing the commits that added the
> >> various locked do_* functions over the past years.
> >>
> >> Short story is that the notifier chain has it's own mutex and any call
> >> mediated through it introces that lock into the chain, which creates a
> >> massive pile of additional depencies. The only solution is to grab _all_
> >> required locks outside of that notifier chain, which means we've spent a
> >> lot of patches growing the scope of console_lock. Which is bad, since
> >> anything holding console_lock can't get dmesg lines out when it dies.
> >
> > These are independent issues from what this patch fixes. It doesn't try
> > to grow the scope of console_lock either, it makes the sysfs s_active
> > lock independent of console_lock.
> >
> >> This patch here is another step into the wrong direction imo. It's also
> >> for a feature that mere users should never use (even though it's in
> >> sysfs). Heck it's a regression which has been introduced by pulling
> >> console_lock out&up, before that effort sysfs files worked correctly and
> >> implemented locking as described.
> >
> > It doesn't matter where you take console_lock, since it's fixed where
> > s_active is taken (in the vfs code before the sysfs entry's write hook
> > is called). So again this issue is not related to the growing scope of
> > console_lock. This fix makes the s_active lock independent of
> > console_lock, so I don't see why it's a step in the wrong direction.
> >
> >> The right solution imo here is to at least cut up the fbdev notifier into
> >> the different uses so that the locking artificial locking inversions go
> >> away. Most of the calls are used to go from fbdev core (fbmem.c) to the
> >> fbcon.c. Apparently someone though it would be great to allow fb.ko and
> >> fbcon.ko to be able to load in any order whatsoever. Then there's various
> >> other calls that go the other direction (e.g. fbcon calling into backlight
> >> logic) and those introduce the locking inversion. So if we split things
> >> into 2 notifier chais, one to exclusively call into fbcon and the other
> >> for everything else we could revert all the patche that move console_lock
> >> out and fix this bug here properly.
> >>
> >> So NACK from me for this.
> >
> > I think I proved it in the commit message that this issue is independent
> > of fbcon/fbdev, so refactoring these will not solve it. This patch fixes
> > an issue in vt and while your points may be valid, they are not really
> > about this issue or how the patch fixes it.
>
> I think you may have missed Daniel's point. Which is what I was trying to
> point out earlier but in an overly terse manner.

I did get what Daniel said and they are valid points. They require more
work though and since things are broken in the vt code right now we
should try to fix it there, instead of waiting for those planned changes
elsewhere to take place.

The fix will be anyway the same in principal even after Daniel's planned
rework for fbcon/fbdev: not holding the console_lock while freeing the
sysfs entries. The only difference is that this happens in a deferred
work in my patch vs. the same thread after the planned refactoring.

> If you start by just moving the sysfs teardown outside the console_lock
> (but still in the same thread of execution), then the direct lock inversion
> between console_lock and the sysfs lock goes away.
>
> However, you'll now realize that you can't move the sysfs teardown outside
> the console lock because fbcon is doing teardown from inside its notifier,
> which means that there would be a lock inversion between the console lock
> and the notifier lock.
>
> Which is why we're pointing out that the real problem here is the
> fb notifier call chain lock.

Right, I agree that the ideal solution would be to not have a deferred
work for this, but I don't know how much refactoring that requires
elsewhere. The fix is technically correct as Daniel pointed out and imo
it's simple enough to apply it while the bigger refactoring takes place.
That way we can have a working way to reload modules, which is broken
atm.

--Imre

2014-12-16 15:00:51

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On 12/16/2014 09:38 AM, Imre Deak wrote:
> On Tue, 2014-12-16 at 07:50 -0500, Peter Hurley wrote:
>> On 12/16/2014 05:23 AM, Imre Deak wrote:
>>> On Tue, 2014-12-16 at 08:53 +0100, Daniel Vetter wrote:
>>>> On Tue, Dec 16, 2014 at 12:16:01AM +0200, Imre Deak wrote:
>>>>> Currently there is a lock order problem between the console lock and the
>>>>> kernfs s_active lock of the console driver's bind sysfs entry. When
>>>>> writing to the sysfs entry the lock order is first s_active then console
>>>>> lock, when unregistering the console driver via
>>>>> do_unregister_con_driver() the order is the opposite. See the below
>>>>> bugzilla reference for one instance of a lockdep backtrace.
>>>>>
>>>>> Fix this by unregistering the console driver from a deferred work, where
>>>>> we can safely drop the console lock while unregistering the device and
>>>>> corresponding sysfs entries (which in turn acquire s_active). Note that
>>>>> we have to keep the console driver slot in the registered_con_driver
>>>>> array reserved for the driver that's being unregistered until it's fully
>>>>> removed. Otherwise a concurrent call to do_register_con_driver could
>>>>> try to reuse the same slot and fail when registering the corresponding
>>>>> device with a minor index that's still in use.
>>>>>
>>>>> Note that the referenced bug report contains two dmesg logs with two
>>>>> distinct lockdep reports: [1] is about a locking scenario involving
>>>>> s_active, console_lock and the fb_notifier list lock, while [2] is
>>>>> about a locking scenario involving only s_active and console_lock.
>>>>> In [1] locking fb_notifier triggers the lockdep warning only because
>>>>> of its dependence on console_lock, otherwise case [1] is the same
>>>>> s_active<->console_lock dependency problem fixed by this patch.
>>>>> Before this change we have the following locking scenarios involving
>>>>> the 3 locks:
>>>>>
>>>>> a) via do_unregister_framebuffer()->...->do_unregister_con_driver():
>>>>> 1. console lock 2. fb_notifier lock 3. s_active lock
>>>>> b) for example via give_up_console()->do_unregister_con_driver():
>>>>> 1. console lock 2. s_active lock
>>>>> c) via vt_bind()/vt_unbind():
>>>>> 1. s_active lock 2. console lock
>>>>>
>>>>> Since c) is the console bind sysfs entry's write code path we can't
>>>>> change the locking order there. We can only fix this issue by removing
>>>>> s_active's dependence on the other two locks in a) and b). We can do
>>>>> this only in the vt code which owns the corresponding sysfs entry, so
>>>>> that after the change we have:
>>>>>
>>>>> a) 1. console lock 2. fb_notifier lock
>>>>> b) 1. console lock
>>>>> c) 1. s_active lock 2. console lock
>>>>> d) in the new con_driver_unregister_callback():
>>>>> 1. s_active lock
>>>>>
>>>>> [1] https://bugs.freedesktop.org/attachment.cgi?id=87716
>>>>> [2] https://bugs.freedesktop.org/attachment.cgi?id=107602
>>>>>
>>>>> v2:
>>>>> - get console_lock earlier in con_driver_unregister_callback(), so we
>>>>> protect the following console driver field assignments there
>>>>> - add code coment explaining the reason for deferring the sysfs entry
>>>>> removal
>>>>> - add a third paragraph to the commit message explaining why there are
>>>>> two distinct lockdep reports and that this issue is independent of
>>>>> fb/fbcon. (Peter Hurley)
>>>>> v3:
>>>>> - clarify further the third paragraph
>>>>> v4:
>>>>> - rebased on v4 of patch 1/3
>>>>>
>>>>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=70523
>>>>> Signed-off-by: Imre Deak <[email protected]>
>>>>
>>>> So the normal/simple solution would be to remove the con driver from the
>>>> registration while holding required locks, but destroy sysfs stuff after
>>>> the critical section.
>>>>
>>>> The problem is that console unbind/bind/reg/unreg are all done with the
>>>> locks held already, and the reason for that is the fbcon/fbdev notifier
>>>> chain. You can read up on the story by chasing the commits that added the
>>>> various locked do_* functions over the past years.
>>>>
>>>> Short story is that the notifier chain has it's own mutex and any call
>>>> mediated through it introces that lock into the chain, which creates a
>>>> massive pile of additional depencies. The only solution is to grab _all_
>>>> required locks outside of that notifier chain, which means we've spent a
>>>> lot of patches growing the scope of console_lock. Which is bad, since
>>>> anything holding console_lock can't get dmesg lines out when it dies.
>>>
>>> These are independent issues from what this patch fixes. It doesn't try
>>> to grow the scope of console_lock either, it makes the sysfs s_active
>>> lock independent of console_lock.
>>>
>>>> This patch here is another step into the wrong direction imo. It's also
>>>> for a feature that mere users should never use (even though it's in
>>>> sysfs). Heck it's a regression which has been introduced by pulling
>>>> console_lock out&up, before that effort sysfs files worked correctly and
>>>> implemented locking as described.
>>>
>>> It doesn't matter where you take console_lock, since it's fixed where
>>> s_active is taken (in the vfs code before the sysfs entry's write hook
>>> is called). So again this issue is not related to the growing scope of
>>> console_lock. This fix makes the s_active lock independent of
>>> console_lock, so I don't see why it's a step in the wrong direction.
>>>
>>>> The right solution imo here is to at least cut up the fbdev notifier into
>>>> the different uses so that the locking artificial locking inversions go
>>>> away. Most of the calls are used to go from fbdev core (fbmem.c) to the
>>>> fbcon.c. Apparently someone though it would be great to allow fb.ko and
>>>> fbcon.ko to be able to load in any order whatsoever. Then there's various
>>>> other calls that go the other direction (e.g. fbcon calling into backlight
>>>> logic) and those introduce the locking inversion. So if we split things
>>>> into 2 notifier chais, one to exclusively call into fbcon and the other
>>>> for everything else we could revert all the patche that move console_lock
>>>> out and fix this bug here properly.
>>>>
>>>> So NACK from me for this.
>>>
>>> I think I proved it in the commit message that this issue is independent
>>> of fbcon/fbdev, so refactoring these will not solve it. This patch fixes
>>> an issue in vt and while your points may be valid, they are not really
>>> about this issue or how the patch fixes it.
>>
>> I think you may have missed Daniel's point. Which is what I was trying to
>> point out earlier but in an overly terse manner.
>
> I did get what Daniel said and they are valid points. They require more
> work though and since things are broken in the vt code right now we
> should try to fix it there, instead of waiting for those planned changes
> elsewhere to take place.
>
> The fix will be anyway the same in principal even after Daniel's planned
> rework for fbcon/fbdev: not holding the console_lock while freeing the
> sysfs entries.

Oh, I didn't know Daniel was planning to rework fbcon/fbdev.

And these are not 'the same in principal'. Deferring to a worker thread
is not a magic bullet; it allows for race conditions that don't otherwise
exist.

> The only difference is that this happens in a deferred
> work in my patch vs. the same thread after the planned refactoring.
>
>> If you start by just moving the sysfs teardown outside the console_lock
>> (but still in the same thread of execution), then the direct lock inversion
>> between console_lock and the sysfs lock goes away.
>>
>> However, you'll now realize that you can't move the sysfs teardown outside
>> the console lock because fbcon is doing teardown from inside its notifier,
>> which means that there would be a lock inversion between the console lock
>> and the notifier lock.
>>
>> Which is why we're pointing out that the real problem here is the
>> fb notifier call chain lock.
>
> Right, I agree that the ideal solution would be to not have a deferred
> work for this, but I don't know how much refactoring that requires
> elsewhere. The fix is technically correct as Daniel pointed out and imo
> it's simple enough to apply it while the bigger refactoring takes place.
> That way we can have a working way to reload modules, which is broken
> atm.

Fine. Just another expedient fix piled on top of other expedient fixes
that go back past 3.9 with no end in sight.

Regards,
Peter Hurley

2014-12-16 15:10:15

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On Tue, Dec 16, 2014 at 4:00 PM, Peter Hurley <[email protected]> wrote:
>> The fix will be anyway the same in principal even after Daniel's planned
>> rework for fbcon/fbdev: not holding the console_lock while freeing the
>> sysfs entries.
>
> Oh, I didn't know Daniel was planning to rework fbcon/fbdev.

I don't. I've tried, cried and failed, so maybe in my next live ;-)
But what I've tried was "let's fix fbcon" which was probably a bit too
ambigitous. Splitting the notifier chains should be a lot more doable
(but still lots of work I guess). The problem is it's not just the
notifier chain that's broken with fbcon, but a lot more things:
- initial modeset is done while holding the console lock. Would be
true even after we fix this, since it's done as part of the con setup
done by con_register. We'd need to do the modeset upfront, before
registering fbcon.
- panic handling is a joke with drm drivers and fbdev emulation plus
fbcon - there's way too many sleeps and way too much code in the
fbcon+drm panic path for this to ever work reliably as is. Would need
massive rework. One of the most glaring things is that we have about 3
things that tried to set up fbcon on panic (drm fbdev emulation panic
handler, console unblanking on panic and fbcon panic handling).

It's imo unfixable overall, so my long term plan is to get rid of
fbdev emulation, fbcon and all console_lock paths from kms drivers (we
have that now with I915_FBDEV=n). And then provide consoles with
userspace deamons (kmscon) and a very minimalistic crash-dump tooling
for panic handling on bare-bones kms drivers (not yet there, but
patches from David Herrmann are floating around).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2014-12-16 15:48:47

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On 12/16/2014 10:10 AM, Daniel Vetter wrote:
> On Tue, Dec 16, 2014 at 4:00 PM, Peter Hurley <[email protected]> wrote:
>>> The fix will be anyway the same in principal even after Daniel's planned
>>> rework for fbcon/fbdev: not holding the console_lock while freeing the
>>> sysfs entries.
>>
>> Oh, I didn't know Daniel was planning to rework fbcon/fbdev.
>
> I don't. I've tried, cried and failed, so maybe in my next live ;-)
> But what I've tried was "let's fix fbcon" which was probably a bit too
> ambigitous. Splitting the notifier chains should be a lot more doable
> (but still lots of work I guess). The problem is it's not just the
> notifier chain that's broken with fbcon, but a lot more things:
> - initial modeset is done while holding the console lock. Would be
> true even after we fix this, since it's done as part of the con setup
> done by con_register. We'd need to do the modeset upfront, before
> registering fbcon.
> - panic handling is a joke with drm drivers and fbdev emulation plus
> fbcon - there's way too many sleeps and way too much code in the
> fbcon+drm panic path for this to ever work reliably as is. Would need
> massive rework. One of the most glaring things is that we have about 3
> things that tried to set up fbcon on panic (drm fbdev emulation panic
> handler, console unblanking on panic and fbcon panic handling).

yep - vt + printk + console lock + fbcon/fbdev + drm helper + kms driver
is a total nightmare.

> It's imo unfixable overall, so my long term plan is to get rid of
> fbdev emulation, fbcon and all console_lock paths from kms drivers (we
> have that now with I915_FBDEV=n). And then provide consoles with
> userspace deamons (kmscon) and a very minimalistic crash-dump tooling
> for panic handling on bare-bones kms drivers (not yet there, but
> patches from David Herrmann are floating around).

Interesting, I'll have to look into that.

Regards,
Peter Hurleykmscon

2014-12-16 16:40:35

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On Tue, 2014-12-16 at 10:00 -0500, Peter Hurley wrote:
> On 12/16/2014 09:38 AM, Imre Deak wrote:
> > On Tue, 2014-12-16 at 07:50 -0500, Peter Hurley wrote:
> >> On 12/16/2014 05:23 AM, Imre Deak wrote:
> >>> On Tue, 2014-12-16 at 08:53 +0100, Daniel Vetter wrote:
> >>>> On Tue, Dec 16, 2014 at 12:16:01AM +0200, Imre Deak wrote:
> >>>>> Currently there is a lock order problem between the console lock and the
> >>>>> kernfs s_active lock of the console driver's bind sysfs entry. When
> >>>>> writing to the sysfs entry the lock order is first s_active then console
> >>>>> lock, when unregistering the console driver via
> >>>>> do_unregister_con_driver() the order is the opposite. See the below
> >>>>> bugzilla reference for one instance of a lockdep backtrace.
> >>>>>
> >>>>> Fix this by unregistering the console driver from a deferred work, where
> >>>>> we can safely drop the console lock while unregistering the device and
> >>>>> corresponding sysfs entries (which in turn acquire s_active). Note that
> >>>>> we have to keep the console driver slot in the registered_con_driver
> >>>>> array reserved for the driver that's being unregistered until it's fully
> >>>>> removed. Otherwise a concurrent call to do_register_con_driver could
> >>>>> try to reuse the same slot and fail when registering the corresponding
> >>>>> device with a minor index that's still in use.
> >>>>>
> >>>>> Note that the referenced bug report contains two dmesg logs with two
> >>>>> distinct lockdep reports: [1] is about a locking scenario involving
> >>>>> s_active, console_lock and the fb_notifier list lock, while [2] is
> >>>>> about a locking scenario involving only s_active and console_lock.
> >>>>> In [1] locking fb_notifier triggers the lockdep warning only because
> >>>>> of its dependence on console_lock, otherwise case [1] is the same
> >>>>> s_active<->console_lock dependency problem fixed by this patch.
> >>>>> Before this change we have the following locking scenarios involving
> >>>>> the 3 locks:
> >>>>>
> >>>>> a) via do_unregister_framebuffer()->...->do_unregister_con_driver():
> >>>>> 1. console lock 2. fb_notifier lock 3. s_active lock
> >>>>> b) for example via give_up_console()->do_unregister_con_driver():
> >>>>> 1. console lock 2. s_active lock
> >>>>> c) via vt_bind()/vt_unbind():
> >>>>> 1. s_active lock 2. console lock
> >>>>>
> >>>>> Since c) is the console bind sysfs entry's write code path we can't
> >>>>> change the locking order there. We can only fix this issue by removing
> >>>>> s_active's dependence on the other two locks in a) and b). We can do
> >>>>> this only in the vt code which owns the corresponding sysfs entry, so
> >>>>> that after the change we have:
> >>>>>
> >>>>> a) 1. console lock 2. fb_notifier lock
> >>>>> b) 1. console lock
> >>>>> c) 1. s_active lock 2. console lock
> >>>>> d) in the new con_driver_unregister_callback():
> >>>>> 1. s_active lock
> >>>>>
> >>>>> [1] https://bugs.freedesktop.org/attachment.cgi?id=87716
> >>>>> [2] https://bugs.freedesktop.org/attachment.cgi?id=107602
> >>>>>
> >>>>> v2:
> >>>>> - get console_lock earlier in con_driver_unregister_callback(), so we
> >>>>> protect the following console driver field assignments there
> >>>>> - add code coment explaining the reason for deferring the sysfs entry
> >>>>> removal
> >>>>> - add a third paragraph to the commit message explaining why there are
> >>>>> two distinct lockdep reports and that this issue is independent of
> >>>>> fb/fbcon. (Peter Hurley)
> >>>>> v3:
> >>>>> - clarify further the third paragraph
> >>>>> v4:
> >>>>> - rebased on v4 of patch 1/3
> >>>>>
> >>>>> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=70523
> >>>>> Signed-off-by: Imre Deak <[email protected]>
> >>>>
> >>>> So the normal/simple solution would be to remove the con driver from the
> >>>> registration while holding required locks, but destroy sysfs stuff after
> >>>> the critical section.
> >>>>
> >>>> The problem is that console unbind/bind/reg/unreg are all done with the
> >>>> locks held already, and the reason for that is the fbcon/fbdev notifier
> >>>> chain. You can read up on the story by chasing the commits that added the
> >>>> various locked do_* functions over the past years.
> >>>>
> >>>> Short story is that the notifier chain has it's own mutex and any call
> >>>> mediated through it introces that lock into the chain, which creates a
> >>>> massive pile of additional depencies. The only solution is to grab _all_
> >>>> required locks outside of that notifier chain, which means we've spent a
> >>>> lot of patches growing the scope of console_lock. Which is bad, since
> >>>> anything holding console_lock can't get dmesg lines out when it dies.
> >>>
> >>> These are independent issues from what this patch fixes. It doesn't try
> >>> to grow the scope of console_lock either, it makes the sysfs s_active
> >>> lock independent of console_lock.
> >>>
> >>>> This patch here is another step into the wrong direction imo. It's also
> >>>> for a feature that mere users should never use (even though it's in
> >>>> sysfs). Heck it's a regression which has been introduced by pulling
> >>>> console_lock out&up, before that effort sysfs files worked correctly and
> >>>> implemented locking as described.
> >>>
> >>> It doesn't matter where you take console_lock, since it's fixed where
> >>> s_active is taken (in the vfs code before the sysfs entry's write hook
> >>> is called). So again this issue is not related to the growing scope of
> >>> console_lock. This fix makes the s_active lock independent of
> >>> console_lock, so I don't see why it's a step in the wrong direction.
> >>>
> >>>> The right solution imo here is to at least cut up the fbdev notifier into
> >>>> the different uses so that the locking artificial locking inversions go
> >>>> away. Most of the calls are used to go from fbdev core (fbmem.c) to the
> >>>> fbcon.c. Apparently someone though it would be great to allow fb.ko and
> >>>> fbcon.ko to be able to load in any order whatsoever. Then there's various
> >>>> other calls that go the other direction (e.g. fbcon calling into backlight
> >>>> logic) and those introduce the locking inversion. So if we split things
> >>>> into 2 notifier chais, one to exclusively call into fbcon and the other
> >>>> for everything else we could revert all the patche that move console_lock
> >>>> out and fix this bug here properly.
> >>>>
> >>>> So NACK from me for this.
> >>>
> >>> I think I proved it in the commit message that this issue is independent
> >>> of fbcon/fbdev, so refactoring these will not solve it. This patch fixes
> >>> an issue in vt and while your points may be valid, they are not really
> >>> about this issue or how the patch fixes it.
> >>
> >> I think you may have missed Daniel's point. Which is what I was trying to
> >> point out earlier but in an overly terse manner.
> >
> > I did get what Daniel said and they are valid points. They require more
> > work though and since things are broken in the vt code right now we
> > should try to fix it there, instead of waiting for those planned changes
> > elsewhere to take place.
> >
> > The fix will be anyway the same in principal even after Daniel's planned
> > rework for fbcon/fbdev: not holding the console_lock while freeing the
> > sysfs entries.
>
> Oh, I didn't know Daniel was planning to rework fbcon/fbdev.
>
> And these are not 'the same in principal'. Deferring to a worker thread
> is not a magic bullet; it allows for race conditions that don't otherwise
> exist.

True, there is a race with the current approach, but it's there even if
you didn't have a deferred work. As soon as you drop the console_lock
you'd have to make sure the given slot in registered_con_driver isn't
reused until you free the underlying struct device. One way to do that
is using the new flag I added.

> > The only difference is that this happens in a deferred
> > work in my patch vs. the same thread after the planned refactoring.
> >
> >> If you start by just moving the sysfs teardown outside the console_lock
> >> (but still in the same thread of execution), then the direct lock inversion
> >> between console_lock and the sysfs lock goes away.
> >>
> >> However, you'll now realize that you can't move the sysfs teardown outside
> >> the console lock because fbcon is doing teardown from inside its notifier,
> >> which means that there would be a lock inversion between the console lock
> >> and the notifier lock.
> >>
> >> Which is why we're pointing out that the real problem here is the
> >> fb notifier call chain lock.
> >
> > Right, I agree that the ideal solution would be to not have a deferred
> > work for this, but I don't know how much refactoring that requires
> > elsewhere. The fix is technically correct as Daniel pointed out and imo
> > it's simple enough to apply it while the bigger refactoring takes place.
> > That way we can have a working way to reload modules, which is broken
> > atm.
>
> Fine. Just another expedient fix piled on top of other expedient fixes
> that go back past 3.9 with no end in sight.

I'm also happy to look into narrowing down the scope of console_lock in
fbdev/fbcon as was suggested. But doing that as a follow-up to this
change still makes sense to me since it will take more time and have the
risk of regressions that are not related to what this change fixes.

--Imre

2014-12-16 17:15:20

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] vt: fix console lock vs. kernfs s_active lock order

On 12/16/2014 11:22 AM, Imre Deak wrote:
> On Tue, 2014-12-16 at 10:00 -0500, Peter Hurley wrote:

>> Fine. Just another expedient fix piled on top of other expedient fixes
>> that go back past 3.9 with no end in sight.
>
> I'm also happy to look into narrowing down the scope of console_lock in
> fbdev/fbcon as was suggested. But doing that as a follow-up to this
> change still makes sense to me since it will take more time and have the
> risk of regressions that are not related to what this change fixes.

I apologize for my tone. I'm not blaming you for the current situation,
nor is it your responsibility to go fix vt/fbcon/fbdev driver stack
inversion. I'm just trying to bring some awareness of the larger scope,
so that collectively we take action and resolve the underlying problems.

Regards,
Peter Hurley

2014-12-16 17:42:09