Closing the last framebuffer descriptor can deadlock against PicoLCD's
deferred update worker. fb_release() holds info->lock while waiting for
deferred work, and picolcd_fb_update() takes the same lock. Lockdep
reports the cycle through deferred-work completion and fbdefio_state->lock.
Repeated framebuffer open/write/close with concurrent device destruction
reproduces the hang in a PREEMPT_RT QEMU guest.
Use a private update mutex in the deferred worker instead of info->lock.
Take it in picolcd_set_par() as well to preserve serialization of pixel
format conversion and framebuffer updates. Initialize it before exposing
the framebuffer.
The test with the preceding output-request fix hung in the first round
and reported a circular locking dependency. With this change, all five
rounds completed, including 29 successful framebuffer write cycles and
concurrent LCD, backlight, two LED and UHID destroy operations, without
BUG/WARNING. The original syzkaller reproducer and persistent-open
framebuffer tests also passed again. These are bounded virtual-device
tests; physical hardware and suspend/resume remain untested.
Fixes: 3efc61d95259 ("fbdev: Fix invalid page access after closing
deferred I/O devices")
Signed-off-by: Aveline Noir <[email protected]>
---
drivers/hid/hid-picolcd.h | 2 ++
drivers/hid/hid-picolcd_fb.c | 33 ++++++++++++++++++++++-----------
2 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-picolcd.h b/drivers/hid/hid-picolcd.h
index 846a8ceb95..33e6654ca0 100644
--- a/drivers/hid/hid-picolcd.h
+++ b/drivers/hid/hid-picolcd.h
@@ -115,6 +115,8 @@ struct picolcd_data {
struct picolcd_fb_data {
/* Framebuffer stuff */
spinlock_t lock;
+ /* Deferred I/O runs while fbdefio_state->lock is held. */
+ struct mutex update_lock;
struct picolcd_data *picolcd;
u8 update_rate;
u8 bpp;
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index c17104fd60..258c7c4fa2 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -231,7 +231,8 @@ static void picolcd_fb_update(struct fb_info *info)
struct picolcd_fb_data *fbdata = info->par;
struct picolcd_data *data;
- mutex_lock(&info->lock);
+ /* fb_release() flushes this work while holding info->lock. */
+ mutex_lock(&fbdata->update_lock);
spin_lock_irqsave(&fbdata->lock, flags);
data = !fbdata->ready ? fbdata->picolcd : NULL;
@@ -258,11 +259,11 @@ static void picolcd_fb_update(struct fb_info *info)
spin_lock_irqsave(&fbdata->lock, flags);
data = fbdata->picolcd;
spin_unlock_irqrestore(&fbdata->lock, flags);
- mutex_unlock(&info->lock);
+ mutex_unlock(&fbdata->update_lock);
if (!data)
return;
hid_hw_wait(data->hdev);
- mutex_lock(&info->lock);
+ mutex_lock(&fbdata->update_lock);
n = 0;
}
spin_lock_irqsave(&fbdata->lock, flags);
@@ -277,13 +278,13 @@ static void picolcd_fb_update(struct fb_info *info)
spin_lock_irqsave(&fbdata->lock, flags);
data = fbdata->picolcd;
spin_unlock_irqrestore(&fbdata->lock, flags);
- mutex_unlock(&info->lock);
+ mutex_unlock(&fbdata->update_lock);
if (data)
hid_hw_wait(data->hdev);
return;
}
out:
- mutex_unlock(&info->lock);
+ mutex_unlock(&fbdata->update_lock);
}
static int picolcd_fb_blank(int blank, struct fb_info *info)
@@ -332,17 +333,24 @@ static int picolcd_set_par(struct fb_info *info)
{
struct picolcd_fb_data *fbdata = info->par;
u8 *tmp_fb, *o_fb;
+ int ret = 0;
+
+ mutex_lock(&fbdata->update_lock);
if (info->var.bits_per_pixel == fbdata->bpp)
- return 0;
+ goto out;
/* switch between 1/8 bit depths */
- if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
- return -EINVAL;
+ if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8) {
+ ret = -EINVAL;
+ goto out;
+ }
o_fb = fbdata->bitmap;
tmp_fb = kmalloc_array(PICOLCDFB_SIZE, info->var.bits_per_pixel,
GFP_KERNEL);
- if (!tmp_fb)
- return -ENOMEM;
+ if (!tmp_fb) {
+ ret = -ENOMEM;
+ goto out;
+ }
/* translate FB content to new bits-per-pixel */
if (info->var.bits_per_pixel == 1) {
@@ -369,7 +377,9 @@ static int picolcd_set_par(struct fb_info *info)
kfree(tmp_fb);
fbdata->bpp = info->var.bits_per_pixel;
- return 0;
+out:
+ mutex_unlock(&fbdata->update_lock);
+ return ret;
}
static void picolcdfb_ops_damage_range(struct fb_info *info, off_t
off, size_t len)
@@ -506,6 +516,7 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
fbdata = info->par;
spin_lock_init(&fbdata->lock);
+ mutex_init(&fbdata->update_lock);
fbdata->picolcd = data;
fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
fbdata->bpp = picolcdfb_var.bits_per_pixel;
--
2.55.0
