Message ID | 20220420085303.100654-4-javierm@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | Fix some race conditions that exists between fbmem and sysfb | expand |
Hi Am 20.04.22 um 10:53 schrieb Javier Martinez Canillas: > Drivers that want to remove registered conflicting framebuffers prior to > register their own framebuffer, calls remove_conflicting_framebuffers(). > > This function takes the registration_lock mutex, to prevent a races when > drivers register framebuffer devices. But if a conflicting framebuffer > device is found, the underlaying platform device is unregistered and this > will lead to the platform driver .remove callback to be called, which in > turn will call to the unregister_framebuffer() that takes the same lock. > > To prevent this, a struct fb_info.forced_out field was used as indication > to unregister_framebuffer() whether the mutex has to be grabbed or not. > > A cleaner solution is to drop the lock before platform_device_unregister() > so unregister_framebuffer() can take it when called from the fbdev driver, > and just grab the lock again after the device has been registered and do > a removal loop restart. I don't see how this patch improves the situation. So far, do_remove_conflicting_framebuffers() had no business in maintaining locks. And now it's doing this in in a goto-loop where it keeps getting/dropping locks. That's asking for bugs IMHO. Best regards Thomas > > Since the framebuffer devices will already be removed, the loop would just > finish when no more conflicting framebuffers are found. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > > (no changes since v1) > > drivers/video/fbdev/core/fbmem.c | 22 +++++++++++++++------- > include/linux/fb.h | 1 - > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 84427470367b..0bb459258df3 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1553,6 +1553,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, > { > int i; > > +restart_removal: > /* check all firmware fbs and kick off if the base addr overlaps */ > for_each_registered_fb(i) { > struct apertures_struct *gen_aper; > @@ -1585,12 +1586,23 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, > pr_warn("fb%d: no device set\n", i); > do_unregister_framebuffer(registered_fb[i]); > } else if (dev_is_platform(device)) { > - registered_fb[i]->forced_out = true; > + /* > + * Drop the lock because if the device is unregistered, its > + * driver will call to unregister_framebuffer(), that takes > + * this lock. > + */ > + mutex_unlock(®istration_lock); > platform_device_unregister(to_platform_device(device)); > + mutex_lock(®istration_lock); > } else { > pr_warn("fb%d: cannot remove device\n", i); > do_unregister_framebuffer(registered_fb[i]); > } > + /* > + * Restart the removal loop now that the device has been > + * unregistered and its associated framebuffer gone. > + */ > + goto restart_removal; > } > } > } > @@ -1897,13 +1909,9 @@ EXPORT_SYMBOL(register_framebuffer); > void > unregister_framebuffer(struct fb_info *fb_info) > { > - bool forced_out = fb_info->forced_out; > - > - if (!forced_out) > - mutex_lock(®istration_lock); > + mutex_lock(®istration_lock); > do_unregister_framebuffer(fb_info); > - if (!forced_out) > - mutex_unlock(®istration_lock); > + mutex_unlock(®istration_lock); > } > EXPORT_SYMBOL(unregister_framebuffer); > > diff --git a/include/linux/fb.h b/include/linux/fb.h > index f95da1af9ff6..b781bc721113 100644 > --- a/include/linux/fb.h > +++ b/include/linux/fb.h > @@ -502,7 +502,6 @@ struct fb_info { > } *apertures; > > bool skip_vt_switch; /* no VT switch on suspend/resume required */ > - bool forced_out; /* set when being removed by another driver */ > }; > > static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
Hello Thomas, Thanks for the feedback. On 4/25/22 10:27, Thomas Zimmermann wrote: > Hi > > Am 20.04.22 um 10:53 schrieb Javier Martinez Canillas: >> Drivers that want to remove registered conflicting framebuffers prior to >> register their own framebuffer, calls remove_conflicting_framebuffers(). >> >> This function takes the registration_lock mutex, to prevent a races when >> drivers register framebuffer devices. But if a conflicting framebuffer >> device is found, the underlaying platform device is unregistered and this >> will lead to the platform driver .remove callback to be called, which in >> turn will call to the unregister_framebuffer() that takes the same lock. >> >> To prevent this, a struct fb_info.forced_out field was used as indication >> to unregister_framebuffer() whether the mutex has to be grabbed or not. >> >> A cleaner solution is to drop the lock before platform_device_unregister() >> so unregister_framebuffer() can take it when called from the fbdev driver, >> and just grab the lock again after the device has been registered and do >> a removal loop restart. > > I don't see how this patch improves the situation. So far, > do_remove_conflicting_framebuffers() had no business in maintaining > locks. And now it's doing this in in a goto-loop where it keeps > getting/dropping locks. That's asking for bugs IMHO. > It's true that do_remove_conflicting_framebuffers() gets more complicated with all the locks release/re-acquire but OTOH unregister_framebuffer() doesn't do conditionally locking, and more importantly the drivers .remove callback isn't called with the lock held, which IMHO is also quite fragile.
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 84427470367b..0bb459258df3 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1553,6 +1553,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, { int i; +restart_removal: /* check all firmware fbs and kick off if the base addr overlaps */ for_each_registered_fb(i) { struct apertures_struct *gen_aper; @@ -1585,12 +1586,23 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, pr_warn("fb%d: no device set\n", i); do_unregister_framebuffer(registered_fb[i]); } else if (dev_is_platform(device)) { - registered_fb[i]->forced_out = true; + /* + * Drop the lock because if the device is unregistered, its + * driver will call to unregister_framebuffer(), that takes + * this lock. + */ + mutex_unlock(®istration_lock); platform_device_unregister(to_platform_device(device)); + mutex_lock(®istration_lock); } else { pr_warn("fb%d: cannot remove device\n", i); do_unregister_framebuffer(registered_fb[i]); } + /* + * Restart the removal loop now that the device has been + * unregistered and its associated framebuffer gone. + */ + goto restart_removal; } } } @@ -1897,13 +1909,9 @@ EXPORT_SYMBOL(register_framebuffer); void unregister_framebuffer(struct fb_info *fb_info) { - bool forced_out = fb_info->forced_out; - - if (!forced_out) - mutex_lock(®istration_lock); + mutex_lock(®istration_lock); do_unregister_framebuffer(fb_info); - if (!forced_out) - mutex_unlock(®istration_lock); + mutex_unlock(®istration_lock); } EXPORT_SYMBOL(unregister_framebuffer); diff --git a/include/linux/fb.h b/include/linux/fb.h index f95da1af9ff6..b781bc721113 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -502,7 +502,6 @@ struct fb_info { } *apertures; bool skip_vt_switch; /* no VT switch on suspend/resume required */ - bool forced_out; /* set when being removed by another driver */ }; static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {