From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-05-03 20:21:13
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 <redacted>
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(-)
@@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)intfb_deferred_io_fsync(structfile*file,loff_tstart,loff_tend,intdatasync){-structfb_info*info=file->private_data;+structfb_info*info=fb_file_fb_info(file);structinode*inode=file_inode(file);interr=file_write_and_wait_range(file,start,end);if(err)returnerr;+if(!info)+return-ENODEV;+/* Skip if deferred io is compiled-in but disabled on this fbdev */if(!info->fbdefio)return0;
From: Sam Ravnborg <hidden> Date: 2022-05-03 20:54:04
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 <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-05-04 08:15:36
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 <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Please see my comment below.
quoted hunk
---
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(-)
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
quoted hunk
/* Skip if deferred io is compiled-in but disabled on this fbdev */
if (!info->fbdefio)
return 0;
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-05-04 08:26:45
Hello Thomas,
On 5/4/22 10:15, Thomas Zimmermann wrote:
Hi
Am 03.05.22 um 22:19 schrieb Javier Martinez Canillas:
quoted
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 <redacted>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Thanks.
Please see my comment below.
[snip]
quoted
+ 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
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
From: Daniel Vetter <hidden> Date: 2022-05-04 09:02:27
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 <redacted>
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 <redacted>
Cheers, Daniel
quoted hunk
---
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(-)
@@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)intfb_deferred_io_fsync(structfile*file,loff_tstart,loff_tend,intdatasync){-structfb_info*info=file->private_data;+structfb_info*info=fb_file_fb_info(file);structinode*inode=file_inode(file);interr=file_write_and_wait_range(file,start,end);if(err)returnerr;+if(!info)+return-ENODEV;+/* Skip if deferred io is compiled-in but disabled on this fbdev */if(!info->fbdefio)return0;
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-05-04 09:27:18
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:
quoted
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 <redacted>
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.
@@ -68,12 +68,15 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)intfb_deferred_io_fsync(structfile*file,loff_tstart,loff_tend,intdatasync){-structfb_info*info=file->private_data;+structfb_info*info=fb_file_fb_info(file);structinode*inode=file_inode(file);interr=file_write_and_wait_range(file,start,end);if(err)returnerr;+if(!info)+return-ENODEV;+/* Skip if deferred io is compiled-in but disabled on this fbdev */if(!info->fbdefio)return0;
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-05-04 09:29:09
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:
quoted
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 <redacted>
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 <redacted>
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
From: Daniel Vetter <hidden> Date: 2022-05-04 10:56:08
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:
quoted
On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
quoted
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 <redacted>
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 <redacted>
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
From: Thomas Zimmermann <tzimmermann@suse.de> Date: 2022-05-04 11:08:50
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:
quoted
Hello Daniel,
On 5/4/22 11:02, Daniel Vetter wrote:
quoted
On Tue, May 03, 2022 at 10:19:34PM +0200, Javier Martinez Canillas wrote:
quoted
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 <redacted>
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 <redacted>
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
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
From: Javier Martinez Canillas <javierm@redhat.com> Date: 2022-05-04 11:35:53
Hello Thomas,
On 5/4/22 13:08, Thomas Zimmermann wrote:
[snip]
quoted
quoted
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.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat