| Message ID | 20240910020919.5757-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fbdev/xen-fbfront: Assign fb_info->device | expand |
On Mon, Sep 09, 2024 at 10:09:16PM -0400, Jason Andryuk wrote: > From: Jason Andryuk <jason.andryuk@amd.com> > > Probing xen-fbfront faults in video_is_primary_device(). The passed-in > struct device is NULL since xen-fbfront doesn't assign it and the > memory is kzalloc()-ed. Assign fb_info->device to avoid this. > > This was exposed by the conversion of fb_is_primary_device() to > video_is_primary_device() which dropped a NULL check for struct device. > > Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers") > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com> > Closes: https://lore.kernel.org/xen-devel/CALUcmUncX=LkXWeiSiTKsDY-cOe8QksWhFvcCneOKfrKd0ZajA@mail.gmail.com/ > Tested-by: Arthur Borsboom <arthurborsboom@gmail.com> > CC: stable@vger.kernel.org > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > The other option would be to re-instate the NULL check in > video_is_primary_device() I do think this is needed, or at least an explanation. The commit message in f178e96de7f0 doesn't mention anything about video_is_primary_device() not allowing being passed a NULL device (like it was possible with fb_is_primary_device()). Otherwise callers of video_is_primary_device() would need to be adjusted to check for device != NULL. Thanks, Roger.
Am 10.09.24 um 04:09 schrieb Jason Andryuk: > From: Jason Andryuk <jason.andryuk@amd.com> > > Probing xen-fbfront faults in video_is_primary_device(). The passed-in > struct device is NULL since xen-fbfront doesn't assign it and the > memory is kzalloc()-ed. Assign fb_info->device to avoid this. > > This was exposed by the conversion of fb_is_primary_device() to > video_is_primary_device() which dropped a NULL check for struct device. > > Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers") > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com> > Closes: https://lore.kernel.org/xen-devel/CALUcmUncX=LkXWeiSiTKsDY-cOe8QksWhFvcCneOKfrKd0ZajA@mail.gmail.com/ > Tested-by: Arthur Borsboom <arthurborsboom@gmail.com> > CC: stable@vger.kernel.org > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Thanks a lot. > --- > The other option would be to re-instate the NULL check in > video_is_primary_device() > --- > drivers/video/fbdev/xen-fbfront.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c > index 66d4628a96ae..c90f48ebb15e 100644 > --- a/drivers/video/fbdev/xen-fbfront.c > +++ b/drivers/video/fbdev/xen-fbfront.c > @@ -407,6 +407,7 @@ static int xenfb_probe(struct xenbus_device *dev, > /* complete the abuse: */ > fb_info->pseudo_palette = fb_info->par; > fb_info->par = info; > + fb_info->device = &dev->dev; > > fb_info->screen_buffer = info->fb; >
Hi Am 10.09.24 um 09:22 schrieb Roger Pau Monné: > On Mon, Sep 09, 2024 at 10:09:16PM -0400, Jason Andryuk wrote: >> From: Jason Andryuk <jason.andryuk@amd.com> >> >> Probing xen-fbfront faults in video_is_primary_device(). The passed-in >> struct device is NULL since xen-fbfront doesn't assign it and the >> memory is kzalloc()-ed. Assign fb_info->device to avoid this. >> >> This was exposed by the conversion of fb_is_primary_device() to >> video_is_primary_device() which dropped a NULL check for struct device. >> >> Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers") >> Reported-by: Arthur Borsboom <arthurborsboom@gmail.com> >> Closes: https://lore.kernel.org/xen-devel/CALUcmUncX=LkXWeiSiTKsDY-cOe8QksWhFvcCneOKfrKd0ZajA@mail.gmail.com/ >> Tested-by: Arthur Borsboom <arthurborsboom@gmail.com> >> CC: stable@vger.kernel.org >> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > >> --- >> The other option would be to re-instate the NULL check in >> video_is_primary_device() > I do think this is needed, or at least an explanation. The commit > message in f178e96de7f0 doesn't mention anything about > video_is_primary_device() not allowing being passed a NULL device > (like it was possible with fb_is_primary_device()). > > Otherwise callers of video_is_primary_device() would need to be > adjusted to check for device != NULL. The helper expects a non-NULL pointer. We might want to document this. Best regards Thomas > > Thanks, Roger. >
On Tue, Sep 10, 2024 at 09:29:30AM +0200, Thomas Zimmermann wrote: > Hi > > Am 10.09.24 um 09:22 schrieb Roger Pau Monné: > > On Mon, Sep 09, 2024 at 10:09:16PM -0400, Jason Andryuk wrote: > > > From: Jason Andryuk <jason.andryuk@amd.com> > > > > > > Probing xen-fbfront faults in video_is_primary_device(). The passed-in > > > struct device is NULL since xen-fbfront doesn't assign it and the > > > memory is kzalloc()-ed. Assign fb_info->device to avoid this. > > > > > > This was exposed by the conversion of fb_is_primary_device() to > > > video_is_primary_device() which dropped a NULL check for struct device. > > > > > > Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers") > > > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com> > > > Closes: https://lore.kernel.org/xen-devel/CALUcmUncX=LkXWeiSiTKsDY-cOe8QksWhFvcCneOKfrKd0ZajA@mail.gmail.com/ > > > Tested-by: Arthur Borsboom <arthurborsboom@gmail.com> > > > CC: stable@vger.kernel.org > > > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > --- > > > The other option would be to re-instate the NULL check in > > > video_is_primary_device() > > I do think this is needed, or at least an explanation. The commit > > message in f178e96de7f0 doesn't mention anything about > > video_is_primary_device() not allowing being passed a NULL device > > (like it was possible with fb_is_primary_device()). > > > > Otherwise callers of video_is_primary_device() would need to be > > adjusted to check for device != NULL. > > The helper expects a non-NULL pointer. We might want to document this. A BUG_ON(!dev); might be enough documentation that the function expected a non-NULL dev IMO. Thanks, Roger.
On Tue, Sep 10, 2024 at 10:13:01AM +0200, Roger Pau Monné wrote: > On Tue, Sep 10, 2024 at 09:29:30AM +0200, Thomas Zimmermann wrote: > > Hi > > > > Am 10.09.24 um 09:22 schrieb Roger Pau Monné: > > > On Mon, Sep 09, 2024 at 10:09:16PM -0400, Jason Andryuk wrote: > > > > From: Jason Andryuk <jason.andryuk@amd.com> > > > > > > > > Probing xen-fbfront faults in video_is_primary_device(). The passed-in > > > > struct device is NULL since xen-fbfront doesn't assign it and the > > > > memory is kzalloc()-ed. Assign fb_info->device to avoid this. > > > > > > > > This was exposed by the conversion of fb_is_primary_device() to > > > > video_is_primary_device() which dropped a NULL check for struct device. > > > > > > > > Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers") > > > > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com> > > > > Closes: https://lore.kernel.org/xen-devel/CALUcmUncX=LkXWeiSiTKsDY-cOe8QksWhFvcCneOKfrKd0ZajA@mail.gmail.com/ > > > > Tested-by: Arthur Borsboom <arthurborsboom@gmail.com> > > > > CC: stable@vger.kernel.org > > > > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > > > --- > > > > The other option would be to re-instate the NULL check in > > > > video_is_primary_device() > > > I do think this is needed, or at least an explanation. The commit > > > message in f178e96de7f0 doesn't mention anything about > > > video_is_primary_device() not allowing being passed a NULL device > > > (like it was possible with fb_is_primary_device()). > > > > > > Otherwise callers of video_is_primary_device() would need to be > > > adjusted to check for device != NULL. > > > > The helper expects a non-NULL pointer. We might want to document this. > > A BUG_ON(!dev); might be enough documentation that the function > expected a non-NULL dev IMO. No need for that, don't check for things that will never happen.
On Tue, 10 Sept 2024 at 10:33, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Sep 10, 2024 at 10:13:01AM +0200, Roger Pau Monné wrote: > > On Tue, Sep 10, 2024 at 09:29:30AM +0200, Thomas Zimmermann wrote: > > > Hi > > > > > > Am 10.09.24 um 09:22 schrieb Roger Pau Monné: > > > > On Mon, Sep 09, 2024 at 10:09:16PM -0400, Jason Andryuk wrote: > > > > > From: Jason Andryuk <jason.andryuk@amd.com> > > > > > > > > > > Probing xen-fbfront faults in video_is_primary_device(). The passed-in > > > > > struct device is NULL since xen-fbfront doesn't assign it and the > > > > > memory is kzalloc()-ed. Assign fb_info->device to avoid this. > > > > > > > > > > This was exposed by the conversion of fb_is_primary_device() to > > > > > video_is_primary_device() which dropped a NULL check for struct device. > > > > > > > > > > Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers") > > > > > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com> > > > > > Closes: https://lore.kernel.org/xen-devel/CALUcmUncX=LkXWeiSiTKsDY-cOe8QksWhFvcCneOKfrKd0ZajA@mail.gmail.com/ > > > > > Tested-by: Arthur Borsboom <arthurborsboom@gmail.com> > > > > > CC: stable@vger.kernel.org > > > > > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > > > > > --- > > > > > The other option would be to re-instate the NULL check in > > > > > video_is_primary_device() > > > > I do think this is needed, or at least an explanation. The commit > > > > message in f178e96de7f0 doesn't mention anything about > > > > video_is_primary_device() not allowing being passed a NULL device > > > > (like it was possible with fb_is_primary_device()). > > > > > > > > Otherwise callers of video_is_primary_device() would need to be > > > > adjusted to check for device != NULL. > > > > > > The helper expects a non-NULL pointer. We might want to document this. > > > > A BUG_ON(!dev); might be enough documentation that the function > > expected a non-NULL dev IMO. > > No need for that, don't check for things that will never happen. And yet, here we are, me reporting a kernel/VM crash due to a thing that will never happen, see 'Closes' above. I don't want to suggest BUG_ON is the right approach; I have no idea. I just want to mention that (!dev) did happen. :-)
On Tue, Sep 10, 2024 at 02:18:35PM +0200, Arthur Borsboom wrote: > On Tue, 10 Sept 2024 at 10:33, Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Tue, Sep 10, 2024 at 10:13:01AM +0200, Roger Pau Monné wrote: > > > On Tue, Sep 10, 2024 at 09:29:30AM +0200, Thomas Zimmermann wrote: > > > > Hi > > > > > > > > Am 10.09.24 um 09:22 schrieb Roger Pau Monné: > > > > > On Mon, Sep 09, 2024 at 10:09:16PM -0400, Jason Andryuk wrote: > > > > > > From: Jason Andryuk <jason.andryuk@amd.com> > > > > > > > > > > > > Probing xen-fbfront faults in video_is_primary_device(). The passed-in > > > > > > struct device is NULL since xen-fbfront doesn't assign it and the > > > > > > memory is kzalloc()-ed. Assign fb_info->device to avoid this. > > > > > > > > > > > > This was exposed by the conversion of fb_is_primary_device() to > > > > > > video_is_primary_device() which dropped a NULL check for struct device. > > > > > > > > > > > > Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers") > > > > > > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com> > > > > > > Closes: https://lore.kernel.org/xen-devel/CALUcmUncX=LkXWeiSiTKsDY-cOe8QksWhFvcCneOKfrKd0ZajA@mail.gmail.com/ > > > > > > Tested-by: Arthur Borsboom <arthurborsboom@gmail.com> > > > > > > CC: stable@vger.kernel.org > > > > > > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> > > > > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > > > > > > > > > > > --- > > > > > > The other option would be to re-instate the NULL check in > > > > > > video_is_primary_device() > > > > > I do think this is needed, or at least an explanation. The commit > > > > > message in f178e96de7f0 doesn't mention anything about > > > > > video_is_primary_device() not allowing being passed a NULL device > > > > > (like it was possible with fb_is_primary_device()). > > > > > > > > > > Otherwise callers of video_is_primary_device() would need to be > > > > > adjusted to check for device != NULL. > > > > > > > > The helper expects a non-NULL pointer. We might want to document this. > > > > > > A BUG_ON(!dev); might be enough documentation that the function > > > expected a non-NULL dev IMO. > > > > No need for that, don't check for things that will never happen. > > And yet, here we are, me reporting a kernel/VM crash due to a thing > that will never happen, see 'Closes' above. > > I don't want to suggest BUG_ON is the right approach; I have no idea. > I just want to mention that (!dev) did happen. :-) A BUG_ON() will cause the same crash, so I don't see your point, sorry. greg k-h
On 9/10/24 04:09, Jason Andryuk wrote: > From: Jason Andryuk <jason.andryuk@amd.com> > > Probing xen-fbfront faults in video_is_primary_device(). The passed-in > struct device is NULL since xen-fbfront doesn't assign it and the > memory is kzalloc()-ed. Assign fb_info->device to avoid this. > > This was exposed by the conversion of fb_is_primary_device() to > video_is_primary_device() which dropped a NULL check for struct device. > > Fixes: f178e96de7f0 ("arch: Remove struct fb_info from video helpers") > Reported-by: Arthur Borsboom <arthurborsboom@gmail.com> > Closes: https://lore.kernel.org/xen-devel/CALUcmUncX=LkXWeiSiTKsDY-cOe8QksWhFvcCneOKfrKd0ZajA@mail.gmail.com/ > Tested-by: Arthur Borsboom <arthurborsboom@gmail.com> > CC: stable@vger.kernel.org > Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> applied to fbdev git tree. Thanks! Helge
diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c index 66d4628a96ae..c90f48ebb15e 100644 --- a/drivers/video/fbdev/xen-fbfront.c +++ b/drivers/video/fbdev/xen-fbfront.c @@ -407,6 +407,7 @@ static int xenfb_probe(struct xenbus_device *dev, /* complete the abuse: */ fb_info->pseudo_palette = fb_info->par; fb_info->par = info; + fb_info->device = &dev->dev; fb_info->screen_buffer = info->fb;