| Message ID | 20220503201934.681276-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] fbdev: Use helper to get fb_info in all file operations | expand |
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> Acked-by: Sam Ravnborg <sam@ravnborg.org>
Hi Am 03.05.22 um 22:19 schrieb Javier Martinez Canillas: > 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> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Please see my comment below. > --- > > Changes in v2: > - Fix copy & paste error passing file->private_data instead of file > to fb_file_fb_info() function (Sam Ravnborg). > > drivers/video/fbdev/core/fb_defio.c | 5 ++++- > drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++--------- > include/linux/fb.h | 1 + > 3 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c > index 842c66b3e33d..ccdf903c48bd 100644 > --- a/drivers/video/fbdev/core/fb_defio.c > +++ b/drivers/video/fbdev/core/fb_defio.c > @@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf) > > int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync) > { > - struct fb_info *info = file->private_data; > + struct fb_info *info = fb_file_fb_info(file); > struct inode *inode = file_inode(file); > int err = file_write_and_wait_range(file, start, end); > if (err) > return err; > > + if (!info) > + return -ENODEV; > + This is consistent with other functions, but it's probably not the correct errno code. It means that a device is not available for opening. But the situation here is rather as with close() on a disconnected-network file. The call to close() returns EIO in this case. Maybe we should consider changing this in a separate patch. Best regards Thomas > /* Skip if deferred io is compiled-in but disabled on this fbdev */ > if (!info->fbdefio) > return 0; > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 97eb0dee411c..ba2c14a1087d 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused proc_fb_seq_ops = { > * So look up the fb_info using the inode minor number, > * and just verify it against the reference we have. > */ > -static struct fb_info *file_fb_info(struct file *file) > +struct fb_info *fb_file_fb_info(struct file *file) > { > struct inode *inode = file_inode(file); > int fbidx = iminor(inode); > @@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file) > info = NULL; > return info; > } > +EXPORT_SYMBOL(fb_file_fb_info); > > static ssize_t > fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) > { > unsigned long p = *ppos; > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > u8 *buffer, *dst; > u8 __iomem *src; > int c, cnt = 0, err = 0; > @@ -825,7 +826,7 @@ static ssize_t > fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos) > { > unsigned long p = *ppos; > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > u8 *buffer, *src; > u8 __iomem *dst; > int c, cnt = 0, err = 0; > @@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > { > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > > if (!info) > return -ENODEV; > @@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd, > static long fb_compat_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > const struct fb_ops *fb; > long ret = -ENOIOCTLCMD; > > @@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, > static int > fb_mmap(struct file *file, struct vm_area_struct * vma) > { > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma); > unsigned long mmio_pgoff; > unsigned long start; > @@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file) > __acquires(&info->lock) > __releases(&info->lock) > { > - struct fb_info * const info = file_fb_info(file); > + struct fb_info * const info = fb_file_fb_info(file); > > if (!info) > return -ENODEV; > @@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp, > unsigned long addr, unsigned long len, > unsigned long pgoff, unsigned long flags) > { > - struct fb_info * const info = filp->private_data; > - unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len); > + struct fb_info * const info = fb_file_fb_info(filp); > + unsigned long fb_size; > + > + if (!info) > + return -ENODEV; > + > + fb_size = PAGE_ALIGN(info->fix.smem_len); > > if (pgoff > fb_size || len > fb_size - pgoff) > return -EINVAL; > diff --git a/include/linux/fb.h b/include/linux/fb.h > index 9a77ab615c36..3004b8b8c5c2 100644 > --- a/include/linux/fb.h > +++ b/include/linux/fb.h > @@ -624,6 +624,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var, > struct fb_fix_screeninfo *fix); > extern int fb_get_options(const char *name, char **option); > extern int fb_new_modelist(struct fb_info *info); > +extern struct fb_info *fb_file_fb_info(struct file *file); > > extern struct fb_info *registered_fb[FB_MAX]; > extern int num_registered_fb;
Hello Thomas, On 5/4/22 10:15, Thomas Zimmermann wrote: > Hi > > Am 03.05.22 um 22:19 schrieb Javier Martinez Canillas: >> 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> > > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > Thanks. > Please see my comment below. [snip] >> >> + if (!info) >> + return -ENODEV; >> + > > This is consistent with other functions, but it's probably not the > correct errno code. It means that a device is not available for opening. > > But the situation here is rather as with close() on a > disconnected-network file. The call to close() returns EIO in this case. > Maybe we should consider changing this in a separate patch. > Indeed. Agree that -EIO makes more sense here. > Best regards > Thomas >
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> Cheers, Daniel > --- > > Changes in v2: > - Fix copy & paste error passing file->private_data instead of file > to fb_file_fb_info() function (Sam Ravnborg). > > drivers/video/fbdev/core/fb_defio.c | 5 ++++- > drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++--------- > include/linux/fb.h | 1 + > 3 files changed, 20 insertions(+), 10 deletions(-) > > diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c > index 842c66b3e33d..ccdf903c48bd 100644 > --- a/drivers/video/fbdev/core/fb_defio.c > +++ b/drivers/video/fbdev/core/fb_defio.c > @@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf) > > int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync) > { > - struct fb_info *info = file->private_data; > + struct fb_info *info = fb_file_fb_info(file); > struct inode *inode = file_inode(file); > int err = file_write_and_wait_range(file, start, end); > if (err) > return err; > > + if (!info) > + return -ENODEV; > + > /* Skip if deferred io is compiled-in but disabled on this fbdev */ > if (!info->fbdefio) > return 0; > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 97eb0dee411c..ba2c14a1087d 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused proc_fb_seq_ops = { > * So look up the fb_info using the inode minor number, > * and just verify it against the reference we have. > */ > -static struct fb_info *file_fb_info(struct file *file) > +struct fb_info *fb_file_fb_info(struct file *file) > { > struct inode *inode = file_inode(file); > int fbidx = iminor(inode); > @@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file) > info = NULL; > return info; > } > +EXPORT_SYMBOL(fb_file_fb_info); > > static ssize_t > fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) > { > unsigned long p = *ppos; > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > u8 *buffer, *dst; > u8 __iomem *src; > int c, cnt = 0, err = 0; > @@ -825,7 +826,7 @@ static ssize_t > fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos) > { > unsigned long p = *ppos; > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > u8 *buffer, *src; > u8 __iomem *dst; > int c, cnt = 0, err = 0; > @@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > { > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > > if (!info) > return -ENODEV; > @@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd, > static long fb_compat_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) > { > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > const struct fb_ops *fb; > long ret = -ENOIOCTLCMD; > > @@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, > static int > fb_mmap(struct file *file, struct vm_area_struct * vma) > { > - struct fb_info *info = file_fb_info(file); > + struct fb_info *info = fb_file_fb_info(file); > int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma); > unsigned long mmio_pgoff; > unsigned long start; > @@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file) > __acquires(&info->lock) > __releases(&info->lock) > { > - struct fb_info * const info = file_fb_info(file); > + struct fb_info * const info = fb_file_fb_info(file); > > if (!info) > return -ENODEV; > @@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp, > unsigned long addr, unsigned long len, > unsigned long pgoff, unsigned long flags) > { > - struct fb_info * const info = filp->private_data; > - unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len); > + struct fb_info * const info = fb_file_fb_info(filp); > + unsigned long fb_size; > + > + if (!info) > + return -ENODEV; > + > + fb_size = PAGE_ALIGN(info->fix.smem_len); > > if (pgoff > fb_size || len > fb_size - pgoff) > return -EINVAL; > diff --git a/include/linux/fb.h b/include/linux/fb.h > index 9a77ab615c36..3004b8b8c5c2 100644 > --- a/include/linux/fb.h > +++ b/include/linux/fb.h > @@ -624,6 +624,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var, > struct fb_fix_screeninfo *fix); > extern int fb_get_options(const char *name, char **option); > extern int fb_new_modelist(struct fb_info *info); > +extern struct fb_info *fb_file_fb_info(struct file *file); > > extern struct fb_info *registered_fb[FB_MAX]; > extern int num_registered_fb; > -- > 2.35.1 >
Hi Am 04.05.22 um 11:02 schrieb Daniel Vetter: > 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. Can we acquire registration_lock around [1]? [1] https://elixir.bootlin.com/linux/v5.17.5/source/drivers/video/fbdev/core/fbmem.c#L756 > > Either way this doesn't really make anything worse, so > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cheers, Daniel > >> --- >> >> Changes in v2: >> - Fix copy & paste error passing file->private_data instead of file >> to fb_file_fb_info() function (Sam Ravnborg). >> >> drivers/video/fbdev/core/fb_defio.c | 5 ++++- >> drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++--------- >> include/linux/fb.h | 1 + >> 3 files changed, 20 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c >> index 842c66b3e33d..ccdf903c48bd 100644 >> --- a/drivers/video/fbdev/core/fb_defio.c >> +++ b/drivers/video/fbdev/core/fb_defio.c >> @@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf) >> >> int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync) >> { >> - struct fb_info *info = file->private_data; >> + struct fb_info *info = fb_file_fb_info(file); >> struct inode *inode = file_inode(file); >> int err = file_write_and_wait_range(file, start, end); >> if (err) >> return err; >> >> + if (!info) >> + return -ENODEV; >> + >> /* Skip if deferred io is compiled-in but disabled on this fbdev */ >> if (!info->fbdefio) >> return 0; >> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c >> index 97eb0dee411c..ba2c14a1087d 100644 >> --- a/drivers/video/fbdev/core/fbmem.c >> +++ b/drivers/video/fbdev/core/fbmem.c >> @@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused proc_fb_seq_ops = { >> * So look up the fb_info using the inode minor number, >> * and just verify it against the reference we have. >> */ >> -static struct fb_info *file_fb_info(struct file *file) >> +struct fb_info *fb_file_fb_info(struct file *file) >> { >> struct inode *inode = file_inode(file); >> int fbidx = iminor(inode); >> @@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file) >> info = NULL; >> return info; >> } >> +EXPORT_SYMBOL(fb_file_fb_info); >> >> static ssize_t >> fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) >> { >> unsigned long p = *ppos; >> - struct fb_info *info = file_fb_info(file); >> + struct fb_info *info = fb_file_fb_info(file); >> u8 *buffer, *dst; >> u8 __iomem *src; >> int c, cnt = 0, err = 0; >> @@ -825,7 +826,7 @@ static ssize_t >> fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos) >> { >> unsigned long p = *ppos; >> - struct fb_info *info = file_fb_info(file); >> + struct fb_info *info = fb_file_fb_info(file); >> u8 *buffer, *src; >> u8 __iomem *dst; >> int c, cnt = 0, err = 0; >> @@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, >> >> static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg) >> { >> - struct fb_info *info = file_fb_info(file); >> + struct fb_info *info = fb_file_fb_info(file); >> >> if (!info) >> return -ENODEV; >> @@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd, >> static long fb_compat_ioctl(struct file *file, unsigned int cmd, >> unsigned long arg) >> { >> - struct fb_info *info = file_fb_info(file); >> + struct fb_info *info = fb_file_fb_info(file); >> const struct fb_ops *fb; >> long ret = -ENOIOCTLCMD; >> >> @@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, >> static int >> fb_mmap(struct file *file, struct vm_area_struct * vma) >> { >> - struct fb_info *info = file_fb_info(file); >> + struct fb_info *info = fb_file_fb_info(file); >> int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma); >> unsigned long mmio_pgoff; >> unsigned long start; >> @@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file) >> __acquires(&info->lock) >> __releases(&info->lock) >> { >> - struct fb_info * const info = file_fb_info(file); >> + struct fb_info * const info = fb_file_fb_info(file); >> >> if (!info) >> return -ENODEV; >> @@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp, >> unsigned long addr, unsigned long len, >> unsigned long pgoff, unsigned long flags) >> { >> - struct fb_info * const info = filp->private_data; >> - unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len); >> + struct fb_info * const info = fb_file_fb_info(filp); >> + unsigned long fb_size; >> + >> + if (!info) >> + return -ENODEV; >> + >> + fb_size = PAGE_ALIGN(info->fix.smem_len); >> >> if (pgoff > fb_size || len > fb_size - pgoff) >> return -EINVAL; >> diff --git a/include/linux/fb.h b/include/linux/fb.h >> index 9a77ab615c36..3004b8b8c5c2 100644 >> --- a/include/linux/fb.h >> +++ b/include/linux/fb.h >> @@ -624,6 +624,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var, >> struct fb_fix_screeninfo *fix); >> extern int fb_get_options(const char *name, char **option); >> extern int fb_new_modelist(struct fb_info *info); >> +extern struct fb_info *fb_file_fb_info(struct file *file); >> >> extern struct fb_info *registered_fb[FB_MAX]; >> extern int num_registered_fb; >> -- >> 2.35.1 >> >
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 ?
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@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 ? 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
Hi Am 04.05.22 um 12:55 schrieb 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@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 ? > > 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. While the problem has been there ever since, the bug didn't happen until we fixed hot-unplugging for fbdev. Not doing anything is probably not the right thing. Best regards Thomas > -Daniel
Hello Thomas, On 5/4/22 13:08, Thomas Zimmermann wrote: [snip] >>> 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. > > While the problem has been there ever since, the bug didn't happen until > we fixed hot-unplugging for fbdev. Not doing anything is probably not > the right thing. > Actually, this issue shouldn't happen if the fbdev drivers are not buggy and do the proper cleanup at .fb_release() time rather than at .remove(). I'll post patches for simplefb and efifb which are the drivers that we mostly care at this point. So we should be good and not need more fixes.
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c index 842c66b3e33d..ccdf903c48bd 100644 --- a/drivers/video/fbdev/core/fb_defio.c +++ b/drivers/video/fbdev/core/fb_defio.c @@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf) int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync) { - struct fb_info *info = file->private_data; + struct fb_info *info = fb_file_fb_info(file); struct inode *inode = file_inode(file); int err = file_write_and_wait_range(file, start, end); if (err) return err; + if (!info) + return -ENODEV; + /* Skip if deferred io is compiled-in but disabled on this fbdev */ if (!info->fbdefio) return 0; diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 97eb0dee411c..ba2c14a1087d 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -745,7 +745,7 @@ static const struct seq_operations __maybe_unused proc_fb_seq_ops = { * So look up the fb_info using the inode minor number, * and just verify it against the reference we have. */ -static struct fb_info *file_fb_info(struct file *file) +struct fb_info *fb_file_fb_info(struct file *file) { struct inode *inode = file_inode(file); int fbidx = iminor(inode); @@ -755,12 +755,13 @@ static struct fb_info *file_fb_info(struct file *file) info = NULL; return info; } +EXPORT_SYMBOL(fb_file_fb_info); static ssize_t fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) { unsigned long p = *ppos; - struct fb_info *info = file_fb_info(file); + struct fb_info *info = fb_file_fb_info(file); u8 *buffer, *dst; u8 __iomem *src; int c, cnt = 0, err = 0; @@ -825,7 +826,7 @@ static ssize_t fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { unsigned long p = *ppos; - struct fb_info *info = file_fb_info(file); + struct fb_info *info = fb_file_fb_info(file); u8 *buffer, *src; u8 __iomem *dst; int c, cnt = 0, err = 0; @@ -1181,7 +1182,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, static long fb_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { - struct fb_info *info = file_fb_info(file); + struct fb_info *info = fb_file_fb_info(file); if (!info) return -ENODEV; @@ -1293,7 +1294,7 @@ static int fb_get_fscreeninfo(struct fb_info *info, unsigned int cmd, static long fb_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { - struct fb_info *info = file_fb_info(file); + struct fb_info *info = fb_file_fb_info(file); const struct fb_ops *fb; long ret = -ENOIOCTLCMD; @@ -1333,7 +1334,7 @@ static long fb_compat_ioctl(struct file *file, unsigned int cmd, static int fb_mmap(struct file *file, struct vm_area_struct * vma) { - struct fb_info *info = file_fb_info(file); + struct fb_info *info = fb_file_fb_info(file); int (*fb_mmap_fn)(struct fb_info *info, struct vm_area_struct *vma); unsigned long mmio_pgoff; unsigned long start; @@ -1434,7 +1435,7 @@ fb_release(struct inode *inode, struct file *file) __acquires(&info->lock) __releases(&info->lock) { - struct fb_info * const info = file_fb_info(file); + struct fb_info * const info = fb_file_fb_info(file); if (!info) return -ENODEV; @@ -1453,8 +1454,13 @@ unsigned long get_fb_unmapped_area(struct file *filp, unsigned long addr, unsigned long len, unsigned long pgoff, unsigned long flags) { - struct fb_info * const info = filp->private_data; - unsigned long fb_size = PAGE_ALIGN(info->fix.smem_len); + struct fb_info * const info = fb_file_fb_info(filp); + unsigned long fb_size; + + if (!info) + return -ENODEV; + + fb_size = PAGE_ALIGN(info->fix.smem_len); if (pgoff > fb_size || len > fb_size - pgoff) return -EINVAL; diff --git a/include/linux/fb.h b/include/linux/fb.h index 9a77ab615c36..3004b8b8c5c2 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -624,6 +624,7 @@ extern int fb_get_color_depth(struct fb_var_screeninfo *var, struct fb_fix_screeninfo *fix); extern int fb_get_options(const char *name, char **option); extern int fb_new_modelist(struct fb_info *info); +extern struct fb_info *fb_file_fb_info(struct file *file); extern struct fb_info *registered_fb[FB_MAX]; extern int num_registered_fb;
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> --- Changes in v2: - Fix copy & paste error passing file->private_data instead of file to fb_file_fb_info() function (Sam Ravnborg). drivers/video/fbdev/core/fb_defio.c | 5 ++++- drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++--------- include/linux/fb.h | 1 + 3 files changed, 20 insertions(+), 10 deletions(-)