Re: [PATCH v2] fbdev: Use helper to get fb_info in all file operations

[Daniel Vetter]

On Wed, May 04, 2022 at 11:28:07AM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
> 
> On 5/4/22 11:02, Daniel Vetter wrote:
> > On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
> >> A reference to the framebuffer device struct fb_info is stored in the file
> >> private data, but this reference could no longer be valid and must not be
> >> accessed directly. Instead, the file_fb_info() accessor function must be
> >> used since it does sanity checking to make sure that the fb_info is valid.
> >>
> >> This can happen for example if the registered framebuffer device is for a
> >> driver that just uses a framebuffer provided by the system firmware. In
> >> that case, the fbdev core would unregister the framebuffer device when a
> >> real video driver is probed and ask to remove conflicting framebuffers.
> >>
> >> Most fbdev file operations already use the helper to get the fb_info but
> >> get_fb_unmapped_area() and fb_deferred_io_fsync() don't. Fix those two.
> >>
> >> Since fb_deferred_io_fsync() is not in fbmem.o, the helper has to be
> >> exported. Rename it and add a fb_ prefix to denote that is public now.
> >>
> >> Reported-by: Junxiao Chang <junxiao.chang@xxxxxxxxx>
> >> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> > 
> > Note that fb_file_info is hilariously racy since there's nothing
> > preventing a concurrenct framebuffer_unregister. Or at least I'm not
> > seeing anything. See cf4a3ae4ef33 ("fbdev: lock_fb_info cannot fail") for
> > context, maybe reference that commit here in your patch.
> >
> > Either way this doesn't really make anything worse, so
> > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >
> 
> Yes, I noticed is racy but at least checking this makes less likely to
> occur. And thanks, I'll reference that commit in the description of v3.
> 
> BTW, I also noticed that the same race that happens with open(),read(),
> close(), etc happens with the VM operations:
> 
> int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> ...
> 	vma->vm_private_data = info;
> ...
> }
> 
> static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> {
> ...
> 	struct fb_info *info = vmf->vma->vm_private_data;
> ...
> }
> 
> static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> {
> ...
> 	struct fb_info *info = vmf->vma->vm_private_data;
> ...
> }
> 
> So something similar to fb_file_fb_info() is needed to check if
> the vm_private_data is still valid. I guess that could be done
> by using the vmf->vma->vm_file and attempting the same trick that
> fb_file_fb_info() does ?

Yeah should work, except if the ptes are set up already there's kinda not
much that this will prevent. We'd need to tear down mappings and SIGBUS or
alternatively have something else in place there so userspace doesn't blow
up in funny ways (which is what we're doing on the drm side, or at least
trying to).

I'm also not sure how much we should care, since ideally for drm drivers
this is all taken care of by drm_dev_enter in the right places. It does
mean though that fbdev mmap either needs to have it's own memory or be
fully redirected to the drm gem mmap.

And then we can afford to just not care to fix fbdev itself.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch