Messages in this thread | | | Date | Wed, 4 May 2022 11:28:07 +0200 | Subject | Re: [PATCH v2] fbdev: Use helper to get fb_info in all file operations | From | Javier Martinez Canillas <> |
| |
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@intel.com> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > 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@ffwll.ch> >
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 ?
-- Best regards,
Javier Martinez Canillas Linux Engineering Red Hat
| |