* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-08-10 17:11 ` Zwane Mwaikambo
0 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-08-10 17:11 UTC (permalink / raw)
To: tcamuso; +Cc: Linux Kernel, dri-devel, dkwon
Hi Folks,
I know this thread eventually dropped off due to not identifying
the underlying issue. It's still occuring on 5.8 and in my case it
happened because the udev device nodes for the DP aux devices were not
cleaned up whereas the kernel had no association with them. I can
reproduce the bug just by creating a device node for a non-existent minor
device and calling open().
To me it still makes sense to just check aux_dev because the chardev has
no way to check before calling.
(gdb) list *drm_dp_aux_dev_get_by_minor+0x29
0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
61 {
62 struct drm_dp_aux_dev *aux_dev = NULL;
63
64 mutex_lock(&aux_idr_mutex);
65 aux_dev = idr_find(&aux_idr, index);
66 if (!kref_get_unless_zero(&aux_dev->refcount))
67 aux_dev = NULL;
68 mutex_unlock(&aux_idr_mutex);
69
(gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
$8 = 0x18
static int auxdev_open(struct inode *inode, struct file *file)
{
unsigned int minor = iminor(inode);
struct drm_dp_aux_dev *aux_dev;
aux_dev = drm_dp_aux_dev_get_by_minor(minor);
if (!aux_dev)
return -ENODEV;
file->private_data = aux_dev;
return 0;
}
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-08-10 17:11 ` Zwane Mwaikambo
0 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-08-10 17:11 UTC (permalink / raw)
To: tcamuso; +Cc: dkwon, Linux Kernel, dri-devel
Hi Folks,
I know this thread eventually dropped off due to not identifying
the underlying issue. It's still occuring on 5.8 and in my case it
happened because the udev device nodes for the DP aux devices were not
cleaned up whereas the kernel had no association with them. I can
reproduce the bug just by creating a device node for a non-existent minor
device and calling open().
To me it still makes sense to just check aux_dev because the chardev has
no way to check before calling.
(gdb) list *drm_dp_aux_dev_get_by_minor+0x29
0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
61 {
62 struct drm_dp_aux_dev *aux_dev = NULL;
63
64 mutex_lock(&aux_idr_mutex);
65 aux_dev = idr_find(&aux_idr, index);
66 if (!kref_get_unless_zero(&aux_dev->refcount))
67 aux_dev = NULL;
68 mutex_unlock(&aux_idr_mutex);
69
(gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
$8 = 0x18
static int auxdev_open(struct inode *inode, struct file *file)
{
unsigned int minor = iminor(inode);
struct drm_dp_aux_dev *aux_dev;
aux_dev = drm_dp_aux_dev_get_by_minor(minor);
if (!aux_dev)
return -ENODEV;
file->private_data = aux_dev;
return 0;
}
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
2020-08-10 17:11 ` Zwane Mwaikambo
@ 2020-08-11 8:58 ` Daniel Vetter
-1 siblings, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2020-08-11 8:58 UTC (permalink / raw)
To: Zwane Mwaikambo; +Cc: tcamuso, dkwon, Linux Kernel, dri-devel
On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> Hi Folks,
> I know this thread eventually dropped off due to not identifying
> the underlying issue. It's still occuring on 5.8 and in my case it
> happened because the udev device nodes for the DP aux devices were not
> cleaned up whereas the kernel had no association with them. I can
> reproduce the bug just by creating a device node for a non-existent minor
> device and calling open().
Hm I don't have that thread anymore, but generally these bugs are solved
by not registering the device before it's ready for use. We do have
drm_connector->late_register for that stuff. Just a guess since I'm not
seeing full details here.
-Daniel
>
> To me it still makes sense to just check aux_dev because the chardev has
> no way to check before calling.
>
> (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> 61 {
> 62 struct drm_dp_aux_dev *aux_dev = NULL;
> 63
> 64 mutex_lock(&aux_idr_mutex);
> 65 aux_dev = idr_find(&aux_idr, index);
> 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> 67 aux_dev = NULL;
> 68 mutex_unlock(&aux_idr_mutex);
> 69
> (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> $8 = 0x18
>
> static int auxdev_open(struct inode *inode, struct file *file)
> {
> unsigned int minor = iminor(inode);
> struct drm_dp_aux_dev *aux_dev;
>
> aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> if (!aux_dev)
> return -ENODEV;
>
> file->private_data = aux_dev;
> return 0;
> }
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-08-11 8:58 ` Daniel Vetter
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2020-08-11 8:58 UTC (permalink / raw)
To: Zwane Mwaikambo; +Cc: dkwon, Linux Kernel, dri-devel, tcamuso
On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> Hi Folks,
> I know this thread eventually dropped off due to not identifying
> the underlying issue. It's still occuring on 5.8 and in my case it
> happened because the udev device nodes for the DP aux devices were not
> cleaned up whereas the kernel had no association with them. I can
> reproduce the bug just by creating a device node for a non-existent minor
> device and calling open().
Hm I don't have that thread anymore, but generally these bugs are solved
by not registering the device before it's ready for use. We do have
drm_connector->late_register for that stuff. Just a guess since I'm not
seeing full details here.
-Daniel
>
> To me it still makes sense to just check aux_dev because the chardev has
> no way to check before calling.
>
> (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> 61 {
> 62 struct drm_dp_aux_dev *aux_dev = NULL;
> 63
> 64 mutex_lock(&aux_idr_mutex);
> 65 aux_dev = idr_find(&aux_idr, index);
> 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> 67 aux_dev = NULL;
> 68 mutex_unlock(&aux_idr_mutex);
> 69
> (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> $8 = 0x18
>
> static int auxdev_open(struct inode *inode, struct file *file)
> {
> unsigned int minor = iminor(inode);
> struct drm_dp_aux_dev *aux_dev;
>
> aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> if (!aux_dev)
> return -ENODEV;
>
> file->private_data = aux_dev;
> return 0;
> }
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
2020-08-11 8:58 ` Daniel Vetter
@ 2020-08-11 22:16 ` Zwane Mwaikambo
-1 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-08-11 22:16 UTC (permalink / raw)
To: Daniel Vetter; +Cc: tcamuso, dkwon, Linux Kernel, dri-devel
On Tue, 11 Aug 2020, Daniel Vetter wrote:
> On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > Hi Folks,
> > I know this thread eventually dropped off due to not identifying
> > the underlying issue. It's still occuring on 5.8 and in my case it
> > happened because the udev device nodes for the DP aux devices were not
> > cleaned up whereas the kernel had no association with them. I can
> > reproduce the bug just by creating a device node for a non-existent minor
> > device and calling open().
>
> Hm I don't have that thread anymore, but generally these bugs are solved
> by not registering the device before it's ready for use. We do have
> drm_connector->late_register for that stuff. Just a guess since I'm not
> seeing full details here.
In this particular case, the physical device disappeared before the nodes
were cleaned up. It involves putting a computer to sleep with a monitor
plugged in and then waking it up with the monitor unplugged.
> >
> > To me it still makes sense to just check aux_dev because the chardev has
> > no way to check before calling.
> >
> > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> > 61 {
> > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > 63
> > 64 mutex_lock(&aux_idr_mutex);
> > 65 aux_dev = idr_find(&aux_idr, index);
> > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > 67 aux_dev = NULL;
> > 68 mutex_unlock(&aux_idr_mutex);
> > 69
> > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > $8 = 0x18
> >
> > static int auxdev_open(struct inode *inode, struct file *file)
> > {
> > unsigned int minor = iminor(inode);
> > struct drm_dp_aux_dev *aux_dev;
> >
> > aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > if (!aux_dev)
> > return -ENODEV;
> >
> > file->private_data = aux_dev;
> > return 0;
> > }
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-08-11 22:16 ` Zwane Mwaikambo
0 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-08-11 22:16 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dkwon, Linux Kernel, dri-devel, tcamuso
On Tue, 11 Aug 2020, Daniel Vetter wrote:
> On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > Hi Folks,
> > I know this thread eventually dropped off due to not identifying
> > the underlying issue. It's still occuring on 5.8 and in my case it
> > happened because the udev device nodes for the DP aux devices were not
> > cleaned up whereas the kernel had no association with them. I can
> > reproduce the bug just by creating a device node for a non-existent minor
> > device and calling open().
>
> Hm I don't have that thread anymore, but generally these bugs are solved
> by not registering the device before it's ready for use. We do have
> drm_connector->late_register for that stuff. Just a guess since I'm not
> seeing full details here.
In this particular case, the physical device disappeared before the nodes
were cleaned up. It involves putting a computer to sleep with a monitor
plugged in and then waking it up with the monitor unplugged.
> >
> > To me it still makes sense to just check aux_dev because the chardev has
> > no way to check before calling.
> >
> > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> > 61 {
> > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > 63
> > 64 mutex_lock(&aux_idr_mutex);
> > 65 aux_dev = idr_find(&aux_idr, index);
> > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > 67 aux_dev = NULL;
> > 68 mutex_unlock(&aux_idr_mutex);
> > 69
> > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > $8 = 0x18
> >
> > static int auxdev_open(struct inode *inode, struct file *file)
> > {
> > unsigned int minor = iminor(inode);
> > struct drm_dp_aux_dev *aux_dev;
> >
> > aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > if (!aux_dev)
> > return -ENODEV;
> >
> > file->private_data = aux_dev;
> > return 0;
> > }
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
2020-08-11 22:16 ` Zwane Mwaikambo
@ 2020-08-12 14:10 ` Daniel Vetter
-1 siblings, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2020-08-12 14:10 UTC (permalink / raw)
To: Zwane Mwaikambo, Lyude; +Cc: tcamuso, dkwon, Linux Kernel, dri-devel
On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com> wrote:
>
> On Tue, 11 Aug 2020, Daniel Vetter wrote:
>
> > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > Hi Folks,
> > > I know this thread eventually dropped off due to not identifying
> > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > happened because the udev device nodes for the DP aux devices were not
> > > cleaned up whereas the kernel had no association with them. I can
> > > reproduce the bug just by creating a device node for a non-existent minor
> > > device and calling open().
> >
> > Hm I don't have that thread anymore, but generally these bugs are solved
> > by not registering the device before it's ready for use. We do have
> > drm_connector->late_register for that stuff. Just a guess since I'm not
> > seeing full details here.
>
> In this particular case, the physical device disappeared before the nodes
> were cleaned up. It involves putting a computer to sleep with a monitor
> plugged in and then waking it up with the monitor unplugged.
We also have early_unregister for the reverse, but yes this sounds
more tricky ... Adding Lyude who's been working on way too much
lifetime fun around dp recently.
-Daniel
>
>
> > >
> > > To me it still makes sense to just check aux_dev because the chardev has
> > > no way to check before calling.
> > >
> > > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > > 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > > 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> > > 61 {
> > > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > > 63
> > > 64 mutex_lock(&aux_idr_mutex);
> > > 65 aux_dev = idr_find(&aux_idr, index);
> > > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > > 67 aux_dev = NULL;
> > > 68 mutex_unlock(&aux_idr_mutex);
> > > 69
> > > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > > $8 = 0x18
> > >
> > > static int auxdev_open(struct inode *inode, struct file *file)
> > > {
> > > unsigned int minor = iminor(inode);
> > > struct drm_dp_aux_dev *aux_dev;
> > >
> > > aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > > if (!aux_dev)
> > > return -ENODEV;
> > >
> > > file->private_data = aux_dev;
> > > return 0;
> > > }
> > >
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-08-12 14:10 ` Daniel Vetter
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2020-08-12 14:10 UTC (permalink / raw)
To: Zwane Mwaikambo, Lyude; +Cc: dkwon, Linux Kernel, dri-devel, tcamuso
On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com> wrote:
>
> On Tue, 11 Aug 2020, Daniel Vetter wrote:
>
> > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > Hi Folks,
> > > I know this thread eventually dropped off due to not identifying
> > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > happened because the udev device nodes for the DP aux devices were not
> > > cleaned up whereas the kernel had no association with them. I can
> > > reproduce the bug just by creating a device node for a non-existent minor
> > > device and calling open().
> >
> > Hm I don't have that thread anymore, but generally these bugs are solved
> > by not registering the device before it's ready for use. We do have
> > drm_connector->late_register for that stuff. Just a guess since I'm not
> > seeing full details here.
>
> In this particular case, the physical device disappeared before the nodes
> were cleaned up. It involves putting a computer to sleep with a monitor
> plugged in and then waking it up with the monitor unplugged.
We also have early_unregister for the reverse, but yes this sounds
more tricky ... Adding Lyude who's been working on way too much
lifetime fun around dp recently.
-Daniel
>
>
> > >
> > > To me it still makes sense to just check aux_dev because the chardev has
> > > no way to check before calling.
> > >
> > > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > > 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > > 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> > > 61 {
> > > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > > 63
> > > 64 mutex_lock(&aux_idr_mutex);
> > > 65 aux_dev = idr_find(&aux_idr, index);
> > > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > > 67 aux_dev = NULL;
> > > 68 mutex_unlock(&aux_idr_mutex);
> > > 69
> > > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > > $8 = 0x18
> > >
> > > static int auxdev_open(struct inode *inode, struct file *file)
> > > {
> > > unsigned int minor = iminor(inode);
> > > struct drm_dp_aux_dev *aux_dev;
> > >
> > > aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > > if (!aux_dev)
> > > return -ENODEV;
> > >
> > > file->private_data = aux_dev;
> > > return 0;
> > > }
> > >
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
2020-08-12 14:10 ` Daniel Vetter
@ 2020-08-12 15:44 ` Lyude Paul
-1 siblings, 0 replies; 22+ messages in thread
From: Lyude Paul @ 2020-08-12 15:44 UTC (permalink / raw)
To: Daniel Vetter, Zwane Mwaikambo; +Cc: tcamuso, dkwon, Linux Kernel, dri-devel
On Wed, 2020-08-12 at 16:10 +0200, Daniel Vetter wrote:
> On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com> wrote:
> > On Tue, 11 Aug 2020, Daniel Vetter wrote:
> >
> > > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > > Hi Folks,
> > > > I know this thread eventually dropped off due to not identifying
> > > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > > happened because the udev device nodes for the DP aux devices were not
> > > > cleaned up whereas the kernel had no association with them. I can
> > > > reproduce the bug just by creating a device node for a non-existent
> > > > minor
> > > > device and calling open().
> > >
> > > Hm I don't have that thread anymore, but generally these bugs are solved
> > > by not registering the device before it's ready for use. We do have
> > > drm_connector->late_register for that stuff. Just a guess since I'm not
> > > seeing full details here.
> >
> > In this particular case, the physical device disappeared before the nodes
> > were cleaned up. It involves putting a computer to sleep with a monitor
> > plugged in and then waking it up with the monitor unplugged.
>
> We also have early_unregister for the reverse, but yes this sounds
> more tricky ... Adding Lyude who's been working on way too much
> lifetime fun around dp recently.
> -Daniel
>
Hi-I think just checking whether the auxdev is NULL or not is a reasonable
fix, although I am curious as to how exactly the aux dev's parent is getting
destroyed before it's child, which I would have thought would be the only way
you could hit this?
> >
> > > > To me it still makes sense to just check aux_dev because the chardev
> > > > has
> > > > no way to check before calling.
> > > >
> > > > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > > > 0x17b39 is in drm_dp_aux_dev_get_by_minor
> > > > (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > > > 60 static struct drm_dp_aux_dev
> > > > *drm_dp_aux_dev_get_by_minor(unsigned index)
> > > > 61 {
> > > > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > > > 63
> > > > 64 mutex_lock(&aux_idr_mutex);
> > > > 65 aux_dev = idr_find(&aux_idr, index);
> > > > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > > > 67 aux_dev = NULL;
> > > > 68 mutex_unlock(&aux_idr_mutex);
> > > > 69
> > > > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > > > $8 = 0x18
> > > >
> > > > static int auxdev_open(struct inode *inode, struct file *file)
> > > > {
> > > > unsigned int minor = iminor(inode);
> > > > struct drm_dp_aux_dev *aux_dev;
> > > >
> > > > aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > > > if (!aux_dev)
> > > > return -ENODEV;
> > > >
> > > > file->private_data = aux_dev;
> > > > return 0;
> > > > }
> > > >
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-08-12 15:44 ` Lyude Paul
0 siblings, 0 replies; 22+ messages in thread
From: Lyude Paul @ 2020-08-12 15:44 UTC (permalink / raw)
To: Daniel Vetter, Zwane Mwaikambo; +Cc: dkwon, Linux Kernel, dri-devel, tcamuso
On Wed, 2020-08-12 at 16:10 +0200, Daniel Vetter wrote:
> On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com> wrote:
> > On Tue, 11 Aug 2020, Daniel Vetter wrote:
> >
> > > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > > Hi Folks,
> > > > I know this thread eventually dropped off due to not identifying
> > > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > > happened because the udev device nodes for the DP aux devices were not
> > > > cleaned up whereas the kernel had no association with them. I can
> > > > reproduce the bug just by creating a device node for a non-existent
> > > > minor
> > > > device and calling open().
> > >
> > > Hm I don't have that thread anymore, but generally these bugs are solved
> > > by not registering the device before it's ready for use. We do have
> > > drm_connector->late_register for that stuff. Just a guess since I'm not
> > > seeing full details here.
> >
> > In this particular case, the physical device disappeared before the nodes
> > were cleaned up. It involves putting a computer to sleep with a monitor
> > plugged in and then waking it up with the monitor unplugged.
>
> We also have early_unregister for the reverse, but yes this sounds
> more tricky ... Adding Lyude who's been working on way too much
> lifetime fun around dp recently.
> -Daniel
>
Hi-I think just checking whether the auxdev is NULL or not is a reasonable
fix, although I am curious as to how exactly the aux dev's parent is getting
destroyed before it's child, which I would have thought would be the only way
you could hit this?
> >
> > > > To me it still makes sense to just check aux_dev because the chardev
> > > > has
> > > > no way to check before calling.
> > > >
> > > > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > > > 0x17b39 is in drm_dp_aux_dev_get_by_minor
> > > > (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > > > 60 static struct drm_dp_aux_dev
> > > > *drm_dp_aux_dev_get_by_minor(unsigned index)
> > > > 61 {
> > > > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > > > 63
> > > > 64 mutex_lock(&aux_idr_mutex);
> > > > 65 aux_dev = idr_find(&aux_idr, index);
> > > > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > > > 67 aux_dev = NULL;
> > > > 68 mutex_unlock(&aux_idr_mutex);
> > > > 69
> > > > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > > > $8 = 0x18
> > > >
> > > > static int auxdev_open(struct inode *inode, struct file *file)
> > > > {
> > > > unsigned int minor = iminor(inode);
> > > > struct drm_dp_aux_dev *aux_dev;
> > > >
> > > > aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > > > if (!aux_dev)
> > > > return -ENODEV;
> > > >
> > > > file->private_data = aux_dev;
> > > > return 0;
> > > > }
> > > >
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
2020-08-12 15:44 ` Lyude Paul
@ 2020-08-12 20:21 ` Zwane Mwaikambo
-1 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-08-12 20:21 UTC (permalink / raw)
To: Lyude Paul; +Cc: Daniel Vetter, tcamuso, dkwon, Linux Kernel, dri-devel
On Wed, 12 Aug 2020, Lyude Paul wrote:
> On Wed, 2020-08-12 at 16:10 +0200, Daniel Vetter wrote:
> > On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com> wrote:
> > > On Tue, 11 Aug 2020, Daniel Vetter wrote:
> > >
> > > > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > > > Hi Folks,
> > > > > I know this thread eventually dropped off due to not identifying
> > > > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > > > happened because the udev device nodes for the DP aux devices were not
> > > > > cleaned up whereas the kernel had no association with them. I can
> > > > > reproduce the bug just by creating a device node for a non-existent
> > > > > minor
> > > > > device and calling open().
> > > >
> > > > Hm I don't have that thread anymore, but generally these bugs are solved
> > > > by not registering the device before it's ready for use. We do have
> > > > drm_connector->late_register for that stuff. Just a guess since I'm not
> > > > seeing full details here.
> > >
> > > In this particular case, the physical device disappeared before the nodes
> > > were cleaned up. It involves putting a computer to sleep with a monitor
> > > plugged in and then waking it up with the monitor unplugged.
> >
> > We also have early_unregister for the reverse, but yes this sounds
> > more tricky ... Adding Lyude who's been working on way too much
> > lifetime fun around dp recently.
> > -Daniel
> >
> Hi-I think just checking whether the auxdev is NULL or not is a reasonable
> fix, although I am curious as to how exactly the aux dev's parent is getting
> destroyed before it's child, which I would have thought would be the only way
> you could hit this?
Here is what it looks like without (1) and with (2) monitor connected. In
the case where the monitor disappears during suspend, the device nodes
aux3,4 are still around
1) No monitor connected
ls -l /dev/drm*
crw------- 1 root root 238, 0 Aug 6 22:32 /dev/drm_dp_aux0
crw------- 1 root root 238, 1 Aug 6 22:32 /dev/drm_dp_aux1
2) Monitor connected
crw------- 1 root root 238, 0 Aug 6 22:32 /dev/drm_dp_aux0
crw------- 1 root root 238, 1 Aug 6 22:32 /dev/drm_dp_aux1
crw------- 1 root root 238, 2 Aug 11 14:51 /dev/drm_dp_aux2
crw------- 1 root root 238, 3 Aug 11 14:51 /dev/drm_dp_aux3
crw------- 1 root root 238, 4 Aug 11 14:51 /dev/drm_dp_aux4
>
> > >
> > > > > To me it still makes sense to just check aux_dev because the chardev
> > > > > has
> > > > > no way to check before calling.
> > > > >
> > > > > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > > > > 0x17b39 is in drm_dp_aux_dev_get_by_minor
> > > > > (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > > > > 60 static struct drm_dp_aux_dev
> > > > > *drm_dp_aux_dev_get_by_minor(unsigned index)
> > > > > 61 {
> > > > > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > > > > 63
> > > > > 64 mutex_lock(&aux_idr_mutex);
> > > > > 65 aux_dev = idr_find(&aux_idr, index);
> > > > > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > > > > 67 aux_dev = NULL;
> > > > > 68 mutex_unlock(&aux_idr_mutex);
> > > > > 69
> > > > > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > > > > $8 = 0x18
> > > > >
> > > > > static int auxdev_open(struct inode *inode, struct file *file)
> > > > > {
> > > > > unsigned int minor = iminor(inode);
> > > > > struct drm_dp_aux_dev *aux_dev;
> > > > >
> > > > > aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > > > > if (!aux_dev)
> > > > > return -ENODEV;
> > > > >
> > > > > file->private_data = aux_dev;
> > > > > return 0;
> > > > > }
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-08-12 20:21 ` Zwane Mwaikambo
0 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-08-12 20:21 UTC (permalink / raw)
To: Lyude Paul; +Cc: dri-devel, dkwon, Linux Kernel, tcamuso
On Wed, 12 Aug 2020, Lyude Paul wrote:
> On Wed, 2020-08-12 at 16:10 +0200, Daniel Vetter wrote:
> > On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com> wrote:
> > > On Tue, 11 Aug 2020, Daniel Vetter wrote:
> > >
> > > > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > > > Hi Folks,
> > > > > I know this thread eventually dropped off due to not identifying
> > > > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > > > happened because the udev device nodes for the DP aux devices were not
> > > > > cleaned up whereas the kernel had no association with them. I can
> > > > > reproduce the bug just by creating a device node for a non-existent
> > > > > minor
> > > > > device and calling open().
> > > >
> > > > Hm I don't have that thread anymore, but generally these bugs are solved
> > > > by not registering the device before it's ready for use. We do have
> > > > drm_connector->late_register for that stuff. Just a guess since I'm not
> > > > seeing full details here.
> > >
> > > In this particular case, the physical device disappeared before the nodes
> > > were cleaned up. It involves putting a computer to sleep with a monitor
> > > plugged in and then waking it up with the monitor unplugged.
> >
> > We also have early_unregister for the reverse, but yes this sounds
> > more tricky ... Adding Lyude who's been working on way too much
> > lifetime fun around dp recently.
> > -Daniel
> >
> Hi-I think just checking whether the auxdev is NULL or not is a reasonable
> fix, although I am curious as to how exactly the aux dev's parent is getting
> destroyed before it's child, which I would have thought would be the only way
> you could hit this?
Here is what it looks like without (1) and with (2) monitor connected. In
the case where the monitor disappears during suspend, the device nodes
aux3,4 are still around
1) No monitor connected
ls -l /dev/drm*
crw------- 1 root root 238, 0 Aug 6 22:32 /dev/drm_dp_aux0
crw------- 1 root root 238, 1 Aug 6 22:32 /dev/drm_dp_aux1
2) Monitor connected
crw------- 1 root root 238, 0 Aug 6 22:32 /dev/drm_dp_aux0
crw------- 1 root root 238, 1 Aug 6 22:32 /dev/drm_dp_aux1
crw------- 1 root root 238, 2 Aug 11 14:51 /dev/drm_dp_aux2
crw------- 1 root root 238, 3 Aug 11 14:51 /dev/drm_dp_aux3
crw------- 1 root root 238, 4 Aug 11 14:51 /dev/drm_dp_aux4
>
> > >
> > > > > To me it still makes sense to just check aux_dev because the chardev
> > > > > has
> > > > > no way to check before calling.
> > > > >
> > > > > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > > > > 0x17b39 is in drm_dp_aux_dev_get_by_minor
> > > > > (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > > > > 60 static struct drm_dp_aux_dev
> > > > > *drm_dp_aux_dev_get_by_minor(unsigned index)
> > > > > 61 {
> > > > > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > > > > 63
> > > > > 64 mutex_lock(&aux_idr_mutex);
> > > > > 65 aux_dev = idr_find(&aux_idr, index);
> > > > > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > > > > 67 aux_dev = NULL;
> > > > > 68 mutex_unlock(&aux_idr_mutex);
> > > > > 69
> > > > > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > > > > $8 = 0x18
> > > > >
> > > > > static int auxdev_open(struct inode *inode, struct file *file)
> > > > > {
> > > > > unsigned int minor = iminor(inode);
> > > > > struct drm_dp_aux_dev *aux_dev;
> > > > >
> > > > > aux_dev = drm_dp_aux_dev_get_by_minor(minor);
> > > > > if (!aux_dev)
> > > > > return -ENODEV;
> > > > >
> > > > > file->private_data = aux_dev;
> > > > > return 0;
> > > > > }
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
2020-08-12 15:44 ` Lyude Paul
@ 2020-08-18 17:58 ` Zwane Mwaikambo
-1 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-08-18 17:58 UTC (permalink / raw)
To: Lyude Paul; +Cc: Daniel Vetter, tcamuso, dkwon, Linux Kernel, dri-devel
On Wed, 12 Aug 2020, Lyude Paul wrote:
> On Wed, 2020-08-12 at 16:10 +0200, Daniel Vetter wrote:
> > On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com> wrote:
> > > On Tue, 11 Aug 2020, Daniel Vetter wrote:
> > >
> > > > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > > > Hi Folks,
> > > > > I know this thread eventually dropped off due to not identifying
> > > > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > > > happened because the udev device nodes for the DP aux devices were not
> > > > > cleaned up whereas the kernel had no association with them. I can
> > > > > reproduce the bug just by creating a device node for a non-existent
> > > > > minor
> > > > > device and calling open().
> > > >
> > > > Hm I don't have that thread anymore, but generally these bugs are solved
> > > > by not registering the device before it's ready for use. We do have
> > > > drm_connector->late_register for that stuff. Just a guess since I'm not
> > > > seeing full details here.
> > >
> > > In this particular case, the physical device disappeared before the nodes
> > > were cleaned up. It involves putting a computer to sleep with a monitor
> > > plugged in and then waking it up with the monitor unplugged.
> >
> > We also have early_unregister for the reverse, but yes this sounds
> > more tricky ... Adding Lyude who's been working on way too much
> > lifetime fun around dp recently.
> > -Daniel
> >
> Hi-I think just checking whether the auxdev is NULL or not is a reasonable
> fix, although I am curious as to how exactly the aux dev's parent is getting
> destroyed before it's child, which I would have thought would be the only way
> you could hit this?
Hi, If this is acceptable, would you consider an updated patch against
5.8?
Thanks,
Zwane
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-08-18 17:58 ` Zwane Mwaikambo
0 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-08-18 17:58 UTC (permalink / raw)
To: Lyude Paul; +Cc: dri-devel, dkwon, Linux Kernel, tcamuso
On Wed, 12 Aug 2020, Lyude Paul wrote:
> On Wed, 2020-08-12 at 16:10 +0200, Daniel Vetter wrote:
> > On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com> wrote:
> > > On Tue, 11 Aug 2020, Daniel Vetter wrote:
> > >
> > > > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > > > Hi Folks,
> > > > > I know this thread eventually dropped off due to not identifying
> > > > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > > > happened because the udev device nodes for the DP aux devices were not
> > > > > cleaned up whereas the kernel had no association with them. I can
> > > > > reproduce the bug just by creating a device node for a non-existent
> > > > > minor
> > > > > device and calling open().
> > > >
> > > > Hm I don't have that thread anymore, but generally these bugs are solved
> > > > by not registering the device before it's ready for use. We do have
> > > > drm_connector->late_register for that stuff. Just a guess since I'm not
> > > > seeing full details here.
> > >
> > > In this particular case, the physical device disappeared before the nodes
> > > were cleaned up. It involves putting a computer to sleep with a monitor
> > > plugged in and then waking it up with the monitor unplugged.
> >
> > We also have early_unregister for the reverse, but yes this sounds
> > more tricky ... Adding Lyude who's been working on way too much
> > lifetime fun around dp recently.
> > -Daniel
> >
> Hi-I think just checking whether the auxdev is NULL or not is a reasonable
> fix, although I am curious as to how exactly the aux dev's parent is getting
> destroyed before it's child, which I would have thought would be the only way
> you could hit this?
Hi, If this is acceptable, would you consider an updated patch against
5.8?
Thanks,
Zwane
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()
2020-08-12 20:21 ` Zwane Mwaikambo
@ 2020-09-04 7:21 ` Zwane Mwaikambo
-1 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-09-04 7:21 UTC (permalink / raw)
To: Lyude Paul; +Cc: Daniel Vetter, dkwon, Linux Kernel, dri-devel, zwanem
I observed this when unplugging a DP monitor whilst a computer is asleep
and then waking it up. This left DP chardev nodes still being present on
the filesystem and accessing these device nodes caused an oops because
drm_dp_aux_dev_get_by_minor() assumes a device exists if it is opened.
This can also be reproduced by creating a device node with mknod(1) and
issuing an open(2)
[166164.933198] BUG: kernel NULL pointer dereference, address: 0000000000000018
[166164.933202] #PF: supervisor read access in kernel mode
[166164.933204] #PF: error_code(0x0000) - not-present page
[166164.933205] PGD 0 P4D 0
[166164.933208] Oops: 0000 [#1] PREEMPT SMP NOPTI
[166164.933211] CPU: 4 PID: 99071 Comm: fwupd Tainted: G W
5.8.0-rc6+ #1
[166164.933213] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W
(1.11 ) 04/21/2020
[166164.933232] RIP: 0010:drm_dp_aux_dev_get_by_minor+0x29/0x70
[drm_kms_helper]
[166164.933234] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 54 41 89 fc 48 c7
c7 60 01 a4 c0 e8 26 ab 30 d7 44 89 e6 48 c7 c7 80 01 a4 c0 e8 47 94 d6 d6
<8b> 50 18 49 89 c4 48 8d 78 18 85 d2 74 33 8d 4a 01 89 d0 f0 0f b1
[166164.933236] RSP: 0018:ffffb7d7c41cbbf0 EFLAGS: 00010246
[166164.933237] RAX: 0000000000000000 RBX: ffff8a90001fe900 RCX: 0000000000000000
[166164.933238] RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffffffffc0a40180
[166164.933239] RBP: ffffb7d7c41cbbf8 R08: 0000000000000000 R09: ffff8a93e157d6d0
[166164.933240] R10: 0000000000000000 R11: ffffffffc0a40188 R12: 0000000000000003
[166164.933241] R13: ffff8a9402200e80 R14: ffff8a90001fe900 R15: 0000000000000000
[166164.933244] FS: 00007f7fb041eb00(0000) GS:ffff8a9411500000(0000)
knlGS:0000000000000000
[166164.933245] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[166164.933246] CR2: 0000000000000018 CR3: 00000000352c2003 CR4: 00000000003606e0
[166164.933247] Call Trace:
[166164.933264] auxdev_open+0x1b/0x40 [drm_kms_helper]
[166164.933278] chrdev_open+0xa7/0x1c0
[166164.933282] ? cdev_put.part.0+0x20/0x20
[166164.933287] do_dentry_open+0x161/0x3c0
[166164.933291] vfs_open+0x2d/0x30
[166164.933297] path_openat+0xb27/0x10e0
[166164.933306] ? atime_needs_update+0x73/0xd0
[166164.933309] do_filp_open+0x91/0x100
[166164.933313] ? __alloc_fd+0xb2/0x150
[166164.933316] do_sys_openat2+0x210/0x2d0
[166164.933318] do_sys_open+0x46/0x80
[166164.933320] __x64_sys_openat+0x20/0x30
[166164.933328] do_syscall_64+0x52/0xc0
[166164.933336] entry_SYSCALL_64_after_hwframe+0x44/0xa9
(gdb) disassemble drm_dp_aux_dev_get_by_minor+0x29
Dump of assembler code for function drm_dp_aux_dev_get_by_minor:
0x0000000000017b10 <+0>: callq 0x17b15 <drm_dp_aux_dev_get_by_minor+5>
0x0000000000017b15 <+5>: push %rbp
0x0000000000017b16 <+6>: mov %rsp,%rbp
0x0000000000017b19 <+9>: push %r12
0x0000000000017b1b <+11>: mov %edi,%r12d
0x0000000000017b1e <+14>: mov $0x0,%rdi
0x0000000000017b25 <+21>: callq 0x17b2a <drm_dp_aux_dev_get_by_minor+26>
0x0000000000017b2a <+26>: mov %r12d,%esi
0x0000000000017b2d <+29>: mov $0x0,%rdi
0x0000000000017b34 <+36>: callq 0x17b39 <drm_dp_aux_dev_get_by_minor+41>
0x0000000000017b39 <+41>: mov 0x18(%rax),%edx <=========
0x0000000000017b3c <+44>: mov %rax,%r12
0x0000000000017b3f <+47>: lea 0x18(%rax),%rdi
0x0000000000017b43 <+51>: test %edx,%edx
0x0000000000017b45 <+53>: je 0x17b7a <drm_dp_aux_dev_get_by_minor+106>
0x0000000000017b47 <+55>: lea 0x1(%rdx),%ecx
0x0000000000017b4a <+58>: mov %edx,%eax
0x0000000000017b4c <+60>: lock cmpxchg %ecx,(%rdi)
0x0000000000017b50 <+64>: jne 0x17b76 <drm_dp_aux_dev_get_by_minor+102>
0x0000000000017b52 <+66>: test %edx,%edx
0x0000000000017b54 <+68>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
0x0000000000017b56 <+70>: test %ecx,%ecx
0x0000000000017b58 <+72>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
0x0000000000017b5a <+74>: mov $0x0,%rdi
0x0000000000017b61 <+81>: callq 0x17b66 <drm_dp_aux_dev_get_by_minor+86>
0x0000000000017b66 <+86>: mov %r12,%rax
0x0000000000017b69 <+89>: pop %r12
0x0000000000017b6b <+91>: pop %rbp
0x0000000000017b6c <+92>: retq
0x0000000000017b6d <+93>: xor %esi,%esi
0x0000000000017b6f <+95>: callq 0x17b74 <drm_dp_aux_dev_get_by_minor+100>
0x0000000000017b74 <+100>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
0x0000000000017b76 <+102>: mov %eax,%edx
0x0000000000017b78 <+104>: jmp 0x17b43 <drm_dp_aux_dev_get_by_minor+51>
0x0000000000017b7a <+106>: xor %r12d,%r12d
0x0000000000017b7d <+109>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
End of assembler dump.
(gdb) list *drm_dp_aux_dev_get_by_minor+0x29
0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
61 {
62 struct drm_dp_aux_dev *aux_dev = NULL;
63
64 mutex_lock(&aux_idr_mutex);
65 aux_dev = idr_find(&aux_idr, index);
66 if (!kref_get_unless_zero(&aux_dev->refcount))
67 aux_dev = NULL;
68 mutex_unlock(&aux_idr_mutex);
69
(gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
$8 = 0x18
Looking at the caller, checks on the minor are pushed down to
drm_dp_aux_dev_get_by_minor()
static int auxdev_open(struct inode *inode, struct file *file)
{
unsigned int minor = iminor(inode);
struct drm_dp_aux_dev *aux_dev;
aux_dev = drm_dp_aux_dev_get_by_minor(minor); <====
if (!aux_dev)
return -ENODEV;
file->private_data = aux_dev;
return 0;
}
Fixes: e94cb37b34eb8 ("Add a drm_aux-dev module for reading/writing dpcd registers")
Cc: stable@vger.kernel.org
Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
---
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
index 2510717d5a08..e25181bf2c48 100644
--- a/drivers/gpu/drm/drm_dp_aux_dev.c
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -63,7 +63,7 @@ static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
mutex_lock(&aux_idr_mutex);
aux_dev = idr_find(&aux_idr, index);
- if (!kref_get_unless_zero(&aux_dev->refcount))
+ if (aux_dev && !kref_get_unless_zero(&aux_dev->refcount))
aux_dev = NULL;
mutex_unlock(&aux_idr_mutex);
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()
@ 2020-09-04 7:21 ` Zwane Mwaikambo
0 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-09-04 7:21 UTC (permalink / raw)
To: Lyude Paul; +Cc: dri-devel, zwanem, dkwon, Linux Kernel
I observed this when unplugging a DP monitor whilst a computer is asleep
and then waking it up. This left DP chardev nodes still being present on
the filesystem and accessing these device nodes caused an oops because
drm_dp_aux_dev_get_by_minor() assumes a device exists if it is opened.
This can also be reproduced by creating a device node with mknod(1) and
issuing an open(2)
[166164.933198] BUG: kernel NULL pointer dereference, address: 0000000000000018
[166164.933202] #PF: supervisor read access in kernel mode
[166164.933204] #PF: error_code(0x0000) - not-present page
[166164.933205] PGD 0 P4D 0
[166164.933208] Oops: 0000 [#1] PREEMPT SMP NOPTI
[166164.933211] CPU: 4 PID: 99071 Comm: fwupd Tainted: G W
5.8.0-rc6+ #1
[166164.933213] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W
(1.11 ) 04/21/2020
[166164.933232] RIP: 0010:drm_dp_aux_dev_get_by_minor+0x29/0x70
[drm_kms_helper]
[166164.933234] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 54 41 89 fc 48 c7
c7 60 01 a4 c0 e8 26 ab 30 d7 44 89 e6 48 c7 c7 80 01 a4 c0 e8 47 94 d6 d6
<8b> 50 18 49 89 c4 48 8d 78 18 85 d2 74 33 8d 4a 01 89 d0 f0 0f b1
[166164.933236] RSP: 0018:ffffb7d7c41cbbf0 EFLAGS: 00010246
[166164.933237] RAX: 0000000000000000 RBX: ffff8a90001fe900 RCX: 0000000000000000
[166164.933238] RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffffffffc0a40180
[166164.933239] RBP: ffffb7d7c41cbbf8 R08: 0000000000000000 R09: ffff8a93e157d6d0
[166164.933240] R10: 0000000000000000 R11: ffffffffc0a40188 R12: 0000000000000003
[166164.933241] R13: ffff8a9402200e80 R14: ffff8a90001fe900 R15: 0000000000000000
[166164.933244] FS: 00007f7fb041eb00(0000) GS:ffff8a9411500000(0000)
knlGS:0000000000000000
[166164.933245] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[166164.933246] CR2: 0000000000000018 CR3: 00000000352c2003 CR4: 00000000003606e0
[166164.933247] Call Trace:
[166164.933264] auxdev_open+0x1b/0x40 [drm_kms_helper]
[166164.933278] chrdev_open+0xa7/0x1c0
[166164.933282] ? cdev_put.part.0+0x20/0x20
[166164.933287] do_dentry_open+0x161/0x3c0
[166164.933291] vfs_open+0x2d/0x30
[166164.933297] path_openat+0xb27/0x10e0
[166164.933306] ? atime_needs_update+0x73/0xd0
[166164.933309] do_filp_open+0x91/0x100
[166164.933313] ? __alloc_fd+0xb2/0x150
[166164.933316] do_sys_openat2+0x210/0x2d0
[166164.933318] do_sys_open+0x46/0x80
[166164.933320] __x64_sys_openat+0x20/0x30
[166164.933328] do_syscall_64+0x52/0xc0
[166164.933336] entry_SYSCALL_64_after_hwframe+0x44/0xa9
(gdb) disassemble drm_dp_aux_dev_get_by_minor+0x29
Dump of assembler code for function drm_dp_aux_dev_get_by_minor:
0x0000000000017b10 <+0>: callq 0x17b15 <drm_dp_aux_dev_get_by_minor+5>
0x0000000000017b15 <+5>: push %rbp
0x0000000000017b16 <+6>: mov %rsp,%rbp
0x0000000000017b19 <+9>: push %r12
0x0000000000017b1b <+11>: mov %edi,%r12d
0x0000000000017b1e <+14>: mov $0x0,%rdi
0x0000000000017b25 <+21>: callq 0x17b2a <drm_dp_aux_dev_get_by_minor+26>
0x0000000000017b2a <+26>: mov %r12d,%esi
0x0000000000017b2d <+29>: mov $0x0,%rdi
0x0000000000017b34 <+36>: callq 0x17b39 <drm_dp_aux_dev_get_by_minor+41>
0x0000000000017b39 <+41>: mov 0x18(%rax),%edx <=========
0x0000000000017b3c <+44>: mov %rax,%r12
0x0000000000017b3f <+47>: lea 0x18(%rax),%rdi
0x0000000000017b43 <+51>: test %edx,%edx
0x0000000000017b45 <+53>: je 0x17b7a <drm_dp_aux_dev_get_by_minor+106>
0x0000000000017b47 <+55>: lea 0x1(%rdx),%ecx
0x0000000000017b4a <+58>: mov %edx,%eax
0x0000000000017b4c <+60>: lock cmpxchg %ecx,(%rdi)
0x0000000000017b50 <+64>: jne 0x17b76 <drm_dp_aux_dev_get_by_minor+102>
0x0000000000017b52 <+66>: test %edx,%edx
0x0000000000017b54 <+68>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
0x0000000000017b56 <+70>: test %ecx,%ecx
0x0000000000017b58 <+72>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
0x0000000000017b5a <+74>: mov $0x0,%rdi
0x0000000000017b61 <+81>: callq 0x17b66 <drm_dp_aux_dev_get_by_minor+86>
0x0000000000017b66 <+86>: mov %r12,%rax
0x0000000000017b69 <+89>: pop %r12
0x0000000000017b6b <+91>: pop %rbp
0x0000000000017b6c <+92>: retq
0x0000000000017b6d <+93>: xor %esi,%esi
0x0000000000017b6f <+95>: callq 0x17b74 <drm_dp_aux_dev_get_by_minor+100>
0x0000000000017b74 <+100>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
0x0000000000017b76 <+102>: mov %eax,%edx
0x0000000000017b78 <+104>: jmp 0x17b43 <drm_dp_aux_dev_get_by_minor+51>
0x0000000000017b7a <+106>: xor %r12d,%r12d
0x0000000000017b7d <+109>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
End of assembler dump.
(gdb) list *drm_dp_aux_dev_get_by_minor+0x29
0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
61 {
62 struct drm_dp_aux_dev *aux_dev = NULL;
63
64 mutex_lock(&aux_idr_mutex);
65 aux_dev = idr_find(&aux_idr, index);
66 if (!kref_get_unless_zero(&aux_dev->refcount))
67 aux_dev = NULL;
68 mutex_unlock(&aux_idr_mutex);
69
(gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
$8 = 0x18
Looking at the caller, checks on the minor are pushed down to
drm_dp_aux_dev_get_by_minor()
static int auxdev_open(struct inode *inode, struct file *file)
{
unsigned int minor = iminor(inode);
struct drm_dp_aux_dev *aux_dev;
aux_dev = drm_dp_aux_dev_get_by_minor(minor); <====
if (!aux_dev)
return -ENODEV;
file->private_data = aux_dev;
return 0;
}
Fixes: e94cb37b34eb8 ("Add a drm_aux-dev module for reading/writing dpcd registers")
Cc: stable@vger.kernel.org
Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
---
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
index 2510717d5a08..e25181bf2c48 100644
--- a/drivers/gpu/drm/drm_dp_aux_dev.c
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -63,7 +63,7 @@ static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
mutex_lock(&aux_idr_mutex);
aux_dev = idr_find(&aux_idr, index);
- if (!kref_get_unless_zero(&aux_dev->refcount))
+ if (aux_dev && !kref_get_unless_zero(&aux_dev->refcount))
aux_dev = NULL;
mutex_unlock(&aux_idr_mutex);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()
2020-09-04 7:21 ` Zwane Mwaikambo
@ 2020-09-07 11:05 ` Ville Syrjälä
-1 siblings, 0 replies; 22+ messages in thread
From: Ville Syrjälä @ 2020-09-07 11:05 UTC (permalink / raw)
To: Zwane Mwaikambo; +Cc: Lyude Paul, dri-devel, zwanem, dkwon, Linux Kernel
On Fri, Sep 04, 2020 at 12:21:26AM -0700, Zwane Mwaikambo wrote:
> I observed this when unplugging a DP monitor whilst a computer is asleep
> and then waking it up. This left DP chardev nodes still being present on
> the filesystem and accessing these device nodes caused an oops because
> drm_dp_aux_dev_get_by_minor() assumes a device exists if it is opened.
> This can also be reproduced by creating a device node with mknod(1) and
> issuing an open(2)
>
> [166164.933198] BUG: kernel NULL pointer dereference, address: 0000000000000018
> [166164.933202] #PF: supervisor read access in kernel mode
> [166164.933204] #PF: error_code(0x0000) - not-present page
> [166164.933205] PGD 0 P4D 0
> [166164.933208] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [166164.933211] CPU: 4 PID: 99071 Comm: fwupd Tainted: G W
> 5.8.0-rc6+ #1
> [166164.933213] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W
> (1.11 ) 04/21/2020
> [166164.933232] RIP: 0010:drm_dp_aux_dev_get_by_minor+0x29/0x70
> [drm_kms_helper]
> [166164.933234] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 54 41 89 fc 48 c7
> c7 60 01 a4 c0 e8 26 ab 30 d7 44 89 e6 48 c7 c7 80 01 a4 c0 e8 47 94 d6 d6
> <8b> 50 18 49 89 c4 48 8d 78 18 85 d2 74 33 8d 4a 01 89 d0 f0 0f b1
> [166164.933236] RSP: 0018:ffffb7d7c41cbbf0 EFLAGS: 00010246
> [166164.933237] RAX: 0000000000000000 RBX: ffff8a90001fe900 RCX: 0000000000000000
> [166164.933238] RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffffffffc0a40180
> [166164.933239] RBP: ffffb7d7c41cbbf8 R08: 0000000000000000 R09: ffff8a93e157d6d0
> [166164.933240] R10: 0000000000000000 R11: ffffffffc0a40188 R12: 0000000000000003
> [166164.933241] R13: ffff8a9402200e80 R14: ffff8a90001fe900 R15: 0000000000000000
> [166164.933244] FS: 00007f7fb041eb00(0000) GS:ffff8a9411500000(0000)
> knlGS:0000000000000000
> [166164.933245] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [166164.933246] CR2: 0000000000000018 CR3: 00000000352c2003 CR4: 00000000003606e0
> [166164.933247] Call Trace:
> [166164.933264] auxdev_open+0x1b/0x40 [drm_kms_helper]
> [166164.933278] chrdev_open+0xa7/0x1c0
> [166164.933282] ? cdev_put.part.0+0x20/0x20
> [166164.933287] do_dentry_open+0x161/0x3c0
> [166164.933291] vfs_open+0x2d/0x30
> [166164.933297] path_openat+0xb27/0x10e0
> [166164.933306] ? atime_needs_update+0x73/0xd0
> [166164.933309] do_filp_open+0x91/0x100
> [166164.933313] ? __alloc_fd+0xb2/0x150
> [166164.933316] do_sys_openat2+0x210/0x2d0
> [166164.933318] do_sys_open+0x46/0x80
> [166164.933320] __x64_sys_openat+0x20/0x30
> [166164.933328] do_syscall_64+0x52/0xc0
> [166164.933336] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
>
> (gdb) disassemble drm_dp_aux_dev_get_by_minor+0x29
> Dump of assembler code for function drm_dp_aux_dev_get_by_minor:
> 0x0000000000017b10 <+0>: callq 0x17b15 <drm_dp_aux_dev_get_by_minor+5>
> 0x0000000000017b15 <+5>: push %rbp
> 0x0000000000017b16 <+6>: mov %rsp,%rbp
> 0x0000000000017b19 <+9>: push %r12
> 0x0000000000017b1b <+11>: mov %edi,%r12d
> 0x0000000000017b1e <+14>: mov $0x0,%rdi
> 0x0000000000017b25 <+21>: callq 0x17b2a <drm_dp_aux_dev_get_by_minor+26>
> 0x0000000000017b2a <+26>: mov %r12d,%esi
> 0x0000000000017b2d <+29>: mov $0x0,%rdi
> 0x0000000000017b34 <+36>: callq 0x17b39 <drm_dp_aux_dev_get_by_minor+41>
> 0x0000000000017b39 <+41>: mov 0x18(%rax),%edx <=========
> 0x0000000000017b3c <+44>: mov %rax,%r12
> 0x0000000000017b3f <+47>: lea 0x18(%rax),%rdi
> 0x0000000000017b43 <+51>: test %edx,%edx
> 0x0000000000017b45 <+53>: je 0x17b7a <drm_dp_aux_dev_get_by_minor+106>
> 0x0000000000017b47 <+55>: lea 0x1(%rdx),%ecx
> 0x0000000000017b4a <+58>: mov %edx,%eax
> 0x0000000000017b4c <+60>: lock cmpxchg %ecx,(%rdi)
> 0x0000000000017b50 <+64>: jne 0x17b76 <drm_dp_aux_dev_get_by_minor+102>
> 0x0000000000017b52 <+66>: test %edx,%edx
> 0x0000000000017b54 <+68>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
> 0x0000000000017b56 <+70>: test %ecx,%ecx
> 0x0000000000017b58 <+72>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
> 0x0000000000017b5a <+74>: mov $0x0,%rdi
> 0x0000000000017b61 <+81>: callq 0x17b66 <drm_dp_aux_dev_get_by_minor+86>
> 0x0000000000017b66 <+86>: mov %r12,%rax
> 0x0000000000017b69 <+89>: pop %r12
> 0x0000000000017b6b <+91>: pop %rbp
> 0x0000000000017b6c <+92>: retq
> 0x0000000000017b6d <+93>: xor %esi,%esi
> 0x0000000000017b6f <+95>: callq 0x17b74 <drm_dp_aux_dev_get_by_minor+100>
> 0x0000000000017b74 <+100>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
> 0x0000000000017b76 <+102>: mov %eax,%edx
> 0x0000000000017b78 <+104>: jmp 0x17b43 <drm_dp_aux_dev_get_by_minor+51>
> 0x0000000000017b7a <+106>: xor %r12d,%r12d
> 0x0000000000017b7d <+109>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
> End of assembler dump.
>
> (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> 61 {
> 62 struct drm_dp_aux_dev *aux_dev = NULL;
> 63
> 64 mutex_lock(&aux_idr_mutex);
> 65 aux_dev = idr_find(&aux_idr, index);
> 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> 67 aux_dev = NULL;
> 68 mutex_unlock(&aux_idr_mutex);
> 69
> (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> $8 = 0x18
>
> Looking at the caller, checks on the minor are pushed down to
> drm_dp_aux_dev_get_by_minor()
>
> static int auxdev_open(struct inode *inode, struct file *file)
> {
> unsigned int minor = iminor(inode);
> struct drm_dp_aux_dev *aux_dev;
>
> aux_dev = drm_dp_aux_dev_get_by_minor(minor); <====
> if (!aux_dev)
> return -ENODEV;
>
> file->private_data = aux_dev;
> return 0;
> }
>
>
> Fixes: e94cb37b34eb8 ("Add a drm_aux-dev module for reading/writing dpcd registers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
> ---
>
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 2510717d5a08..e25181bf2c48 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -63,7 +63,7 @@ static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
>
> mutex_lock(&aux_idr_mutex);
> aux_dev = idr_find(&aux_idr, index);
> - if (!kref_get_unless_zero(&aux_dev->refcount))
> + if (aux_dev && !kref_get_unless_zero(&aux_dev->refcount))
Dejavu
https://lists.freedesktop.org/archives/dri-devel/2019-May/218855.html
https://lists.freedesktop.org/archives/dri-devel/2019-July/226168.html
I guess we just got stuck waiting for confirmation that it reproduces
with the bogus device node trick.
> aux_dev = NULL;
> mutex_unlock(&aux_idr_mutex);
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()
@ 2020-09-07 11:05 ` Ville Syrjälä
0 siblings, 0 replies; 22+ messages in thread
From: Ville Syrjälä @ 2020-09-07 11:05 UTC (permalink / raw)
To: Zwane Mwaikambo; +Cc: zwanem, dkwon, Linux Kernel, dri-devel
On Fri, Sep 04, 2020 at 12:21:26AM -0700, Zwane Mwaikambo wrote:
> I observed this when unplugging a DP monitor whilst a computer is asleep
> and then waking it up. This left DP chardev nodes still being present on
> the filesystem and accessing these device nodes caused an oops because
> drm_dp_aux_dev_get_by_minor() assumes a device exists if it is opened.
> This can also be reproduced by creating a device node with mknod(1) and
> issuing an open(2)
>
> [166164.933198] BUG: kernel NULL pointer dereference, address: 0000000000000018
> [166164.933202] #PF: supervisor read access in kernel mode
> [166164.933204] #PF: error_code(0x0000) - not-present page
> [166164.933205] PGD 0 P4D 0
> [166164.933208] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [166164.933211] CPU: 4 PID: 99071 Comm: fwupd Tainted: G W
> 5.8.0-rc6+ #1
> [166164.933213] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W
> (1.11 ) 04/21/2020
> [166164.933232] RIP: 0010:drm_dp_aux_dev_get_by_minor+0x29/0x70
> [drm_kms_helper]
> [166164.933234] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 54 41 89 fc 48 c7
> c7 60 01 a4 c0 e8 26 ab 30 d7 44 89 e6 48 c7 c7 80 01 a4 c0 e8 47 94 d6 d6
> <8b> 50 18 49 89 c4 48 8d 78 18 85 d2 74 33 8d 4a 01 89 d0 f0 0f b1
> [166164.933236] RSP: 0018:ffffb7d7c41cbbf0 EFLAGS: 00010246
> [166164.933237] RAX: 0000000000000000 RBX: ffff8a90001fe900 RCX: 0000000000000000
> [166164.933238] RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffffffffc0a40180
> [166164.933239] RBP: ffffb7d7c41cbbf8 R08: 0000000000000000 R09: ffff8a93e157d6d0
> [166164.933240] R10: 0000000000000000 R11: ffffffffc0a40188 R12: 0000000000000003
> [166164.933241] R13: ffff8a9402200e80 R14: ffff8a90001fe900 R15: 0000000000000000
> [166164.933244] FS: 00007f7fb041eb00(0000) GS:ffff8a9411500000(0000)
> knlGS:0000000000000000
> [166164.933245] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [166164.933246] CR2: 0000000000000018 CR3: 00000000352c2003 CR4: 00000000003606e0
> [166164.933247] Call Trace:
> [166164.933264] auxdev_open+0x1b/0x40 [drm_kms_helper]
> [166164.933278] chrdev_open+0xa7/0x1c0
> [166164.933282] ? cdev_put.part.0+0x20/0x20
> [166164.933287] do_dentry_open+0x161/0x3c0
> [166164.933291] vfs_open+0x2d/0x30
> [166164.933297] path_openat+0xb27/0x10e0
> [166164.933306] ? atime_needs_update+0x73/0xd0
> [166164.933309] do_filp_open+0x91/0x100
> [166164.933313] ? __alloc_fd+0xb2/0x150
> [166164.933316] do_sys_openat2+0x210/0x2d0
> [166164.933318] do_sys_open+0x46/0x80
> [166164.933320] __x64_sys_openat+0x20/0x30
> [166164.933328] do_syscall_64+0x52/0xc0
> [166164.933336] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
>
> (gdb) disassemble drm_dp_aux_dev_get_by_minor+0x29
> Dump of assembler code for function drm_dp_aux_dev_get_by_minor:
> 0x0000000000017b10 <+0>: callq 0x17b15 <drm_dp_aux_dev_get_by_minor+5>
> 0x0000000000017b15 <+5>: push %rbp
> 0x0000000000017b16 <+6>: mov %rsp,%rbp
> 0x0000000000017b19 <+9>: push %r12
> 0x0000000000017b1b <+11>: mov %edi,%r12d
> 0x0000000000017b1e <+14>: mov $0x0,%rdi
> 0x0000000000017b25 <+21>: callq 0x17b2a <drm_dp_aux_dev_get_by_minor+26>
> 0x0000000000017b2a <+26>: mov %r12d,%esi
> 0x0000000000017b2d <+29>: mov $0x0,%rdi
> 0x0000000000017b34 <+36>: callq 0x17b39 <drm_dp_aux_dev_get_by_minor+41>
> 0x0000000000017b39 <+41>: mov 0x18(%rax),%edx <=========
> 0x0000000000017b3c <+44>: mov %rax,%r12
> 0x0000000000017b3f <+47>: lea 0x18(%rax),%rdi
> 0x0000000000017b43 <+51>: test %edx,%edx
> 0x0000000000017b45 <+53>: je 0x17b7a <drm_dp_aux_dev_get_by_minor+106>
> 0x0000000000017b47 <+55>: lea 0x1(%rdx),%ecx
> 0x0000000000017b4a <+58>: mov %edx,%eax
> 0x0000000000017b4c <+60>: lock cmpxchg %ecx,(%rdi)
> 0x0000000000017b50 <+64>: jne 0x17b76 <drm_dp_aux_dev_get_by_minor+102>
> 0x0000000000017b52 <+66>: test %edx,%edx
> 0x0000000000017b54 <+68>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
> 0x0000000000017b56 <+70>: test %ecx,%ecx
> 0x0000000000017b58 <+72>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
> 0x0000000000017b5a <+74>: mov $0x0,%rdi
> 0x0000000000017b61 <+81>: callq 0x17b66 <drm_dp_aux_dev_get_by_minor+86>
> 0x0000000000017b66 <+86>: mov %r12,%rax
> 0x0000000000017b69 <+89>: pop %r12
> 0x0000000000017b6b <+91>: pop %rbp
> 0x0000000000017b6c <+92>: retq
> 0x0000000000017b6d <+93>: xor %esi,%esi
> 0x0000000000017b6f <+95>: callq 0x17b74 <drm_dp_aux_dev_get_by_minor+100>
> 0x0000000000017b74 <+100>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
> 0x0000000000017b76 <+102>: mov %eax,%edx
> 0x0000000000017b78 <+104>: jmp 0x17b43 <drm_dp_aux_dev_get_by_minor+51>
> 0x0000000000017b7a <+106>: xor %r12d,%r12d
> 0x0000000000017b7d <+109>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
> End of assembler dump.
>
> (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> 61 {
> 62 struct drm_dp_aux_dev *aux_dev = NULL;
> 63
> 64 mutex_lock(&aux_idr_mutex);
> 65 aux_dev = idr_find(&aux_idr, index);
> 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> 67 aux_dev = NULL;
> 68 mutex_unlock(&aux_idr_mutex);
> 69
> (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> $8 = 0x18
>
> Looking at the caller, checks on the minor are pushed down to
> drm_dp_aux_dev_get_by_minor()
>
> static int auxdev_open(struct inode *inode, struct file *file)
> {
> unsigned int minor = iminor(inode);
> struct drm_dp_aux_dev *aux_dev;
>
> aux_dev = drm_dp_aux_dev_get_by_minor(minor); <====
> if (!aux_dev)
> return -ENODEV;
>
> file->private_data = aux_dev;
> return 0;
> }
>
>
> Fixes: e94cb37b34eb8 ("Add a drm_aux-dev module for reading/writing dpcd registers")
> Cc: stable@vger.kernel.org
> Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
> ---
>
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 2510717d5a08..e25181bf2c48 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -63,7 +63,7 @@ static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
>
> mutex_lock(&aux_idr_mutex);
> aux_dev = idr_find(&aux_idr, index);
> - if (!kref_get_unless_zero(&aux_dev->refcount))
> + if (aux_dev && !kref_get_unless_zero(&aux_dev->refcount))
Dejavu
https://lists.freedesktop.org/archives/dri-devel/2019-May/218855.html
https://lists.freedesktop.org/archives/dri-devel/2019-July/226168.html
I guess we just got stuck waiting for confirmation that it reproduces
with the bogus device node trick.
> aux_dev = NULL;
> mutex_unlock(&aux_idr_mutex);
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()
2020-09-07 11:05 ` Ville Syrjälä
@ 2020-09-08 16:18 ` Zwane Mwaikambo
-1 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-09-08 16:18 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Zwane Mwaikambo, Lyude Paul, dri-devel, dkwon, Linux Kernel
[-- Attachment #1: Type: text/plain, Size: 7374 bytes --]
On Mon, 7 Sep 2020, Ville Syrjälä wrote:
> On Fri, Sep 04, 2020 at 12:21:26AM -0700, Zwane Mwaikambo wrote:
> > I observed this when unplugging a DP monitor whilst a computer is asleep
> > and then waking it up. This left DP chardev nodes still being present on
> > the filesystem and accessing these device nodes caused an oops because
> > drm_dp_aux_dev_get_by_minor() assumes a device exists if it is opened.
> > This can also be reproduced by creating a device node with mknod(1) and
> > issuing an open(2)
> >
> > [166164.933198] BUG: kernel NULL pointer dereference, address: 0000000000000018
> > [166164.933202] #PF: supervisor read access in kernel mode
> > [166164.933204] #PF: error_code(0x0000) - not-present page
> > [166164.933205] PGD 0 P4D 0
> > [166164.933208] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > [166164.933211] CPU: 4 PID: 99071 Comm: fwupd Tainted: G W
> > 5.8.0-rc6+ #1
> > [166164.933213] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W
> > (1.11 ) 04/21/2020
> > [166164.933232] RIP: 0010:drm_dp_aux_dev_get_by_minor+0x29/0x70
> > [drm_kms_helper]
> > [166164.933234] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 54 41 89 fc 48 c7
> > c7 60 01 a4 c0 e8 26 ab 30 d7 44 89 e6 48 c7 c7 80 01 a4 c0 e8 47 94 d6 d6
> > <8b> 50 18 49 89 c4 48 8d 78 18 85 d2 74 33 8d 4a 01 89 d0 f0 0f b1
> > [166164.933236] RSP: 0018:ffffb7d7c41cbbf0 EFLAGS: 00010246
> > [166164.933237] RAX: 0000000000000000 RBX: ffff8a90001fe900 RCX: 0000000000000000
> > [166164.933238] RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffffffffc0a40180
> > [166164.933239] RBP: ffffb7d7c41cbbf8 R08: 0000000000000000 R09: ffff8a93e157d6d0
> > [166164.933240] R10: 0000000000000000 R11: ffffffffc0a40188 R12: 0000000000000003
> > [166164.933241] R13: ffff8a9402200e80 R14: ffff8a90001fe900 R15: 0000000000000000
> > [166164.933244] FS: 00007f7fb041eb00(0000) GS:ffff8a9411500000(0000)
> > knlGS:0000000000000000
> > [166164.933245] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [166164.933246] CR2: 0000000000000018 CR3: 00000000352c2003 CR4: 00000000003606e0
> > [166164.933247] Call Trace:
> > [166164.933264] auxdev_open+0x1b/0x40 [drm_kms_helper]
> > [166164.933278] chrdev_open+0xa7/0x1c0
> > [166164.933282] ? cdev_put.part.0+0x20/0x20
> > [166164.933287] do_dentry_open+0x161/0x3c0
> > [166164.933291] vfs_open+0x2d/0x30
> > [166164.933297] path_openat+0xb27/0x10e0
> > [166164.933306] ? atime_needs_update+0x73/0xd0
> > [166164.933309] do_filp_open+0x91/0x100
> > [166164.933313] ? __alloc_fd+0xb2/0x150
> > [166164.933316] do_sys_openat2+0x210/0x2d0
> > [166164.933318] do_sys_open+0x46/0x80
> > [166164.933320] __x64_sys_openat+0x20/0x30
> > [166164.933328] do_syscall_64+0x52/0xc0
> > [166164.933336] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >
> >
> > (gdb) disassemble drm_dp_aux_dev_get_by_minor+0x29
> > Dump of assembler code for function drm_dp_aux_dev_get_by_minor:
> > 0x0000000000017b10 <+0>: callq 0x17b15 <drm_dp_aux_dev_get_by_minor+5>
> > 0x0000000000017b15 <+5>: push %rbp
> > 0x0000000000017b16 <+6>: mov %rsp,%rbp
> > 0x0000000000017b19 <+9>: push %r12
> > 0x0000000000017b1b <+11>: mov %edi,%r12d
> > 0x0000000000017b1e <+14>: mov $0x0,%rdi
> > 0x0000000000017b25 <+21>: callq 0x17b2a <drm_dp_aux_dev_get_by_minor+26>
> > 0x0000000000017b2a <+26>: mov %r12d,%esi
> > 0x0000000000017b2d <+29>: mov $0x0,%rdi
> > 0x0000000000017b34 <+36>: callq 0x17b39 <drm_dp_aux_dev_get_by_minor+41>
> > 0x0000000000017b39 <+41>: mov 0x18(%rax),%edx <=========
> > 0x0000000000017b3c <+44>: mov %rax,%r12
> > 0x0000000000017b3f <+47>: lea 0x18(%rax),%rdi
> > 0x0000000000017b43 <+51>: test %edx,%edx
> > 0x0000000000017b45 <+53>: je 0x17b7a <drm_dp_aux_dev_get_by_minor+106>
> > 0x0000000000017b47 <+55>: lea 0x1(%rdx),%ecx
> > 0x0000000000017b4a <+58>: mov %edx,%eax
> > 0x0000000000017b4c <+60>: lock cmpxchg %ecx,(%rdi)
> > 0x0000000000017b50 <+64>: jne 0x17b76 <drm_dp_aux_dev_get_by_minor+102>
> > 0x0000000000017b52 <+66>: test %edx,%edx
> > 0x0000000000017b54 <+68>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
> > 0x0000000000017b56 <+70>: test %ecx,%ecx
> > 0x0000000000017b58 <+72>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
> > 0x0000000000017b5a <+74>: mov $0x0,%rdi
> > 0x0000000000017b61 <+81>: callq 0x17b66 <drm_dp_aux_dev_get_by_minor+86>
> > 0x0000000000017b66 <+86>: mov %r12,%rax
> > 0x0000000000017b69 <+89>: pop %r12
> > 0x0000000000017b6b <+91>: pop %rbp
> > 0x0000000000017b6c <+92>: retq
> > 0x0000000000017b6d <+93>: xor %esi,%esi
> > 0x0000000000017b6f <+95>: callq 0x17b74 <drm_dp_aux_dev_get_by_minor+100>
> > 0x0000000000017b74 <+100>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
> > 0x0000000000017b76 <+102>: mov %eax,%edx
> > 0x0000000000017b78 <+104>: jmp 0x17b43 <drm_dp_aux_dev_get_by_minor+51>
> > 0x0000000000017b7a <+106>: xor %r12d,%r12d
> > 0x0000000000017b7d <+109>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
> > End of assembler dump.
> >
> > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> > 61 {
> > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > 63
> > 64 mutex_lock(&aux_idr_mutex);
> > 65 aux_dev = idr_find(&aux_idr, index);
> > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > 67 aux_dev = NULL;
> > 68 mutex_unlock(&aux_idr_mutex);
> > 69
> > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > $8 = 0x18
> >
> > Looking at the caller, checks on the minor are pushed down to
> > drm_dp_aux_dev_get_by_minor()
> >
> > static int auxdev_open(struct inode *inode, struct file *file)
> > {
> > unsigned int minor = iminor(inode);
> > struct drm_dp_aux_dev *aux_dev;
> >
> > aux_dev = drm_dp_aux_dev_get_by_minor(minor); <====
> > if (!aux_dev)
> > return -ENODEV;
> >
> > file->private_data = aux_dev;
> > return 0;
> > }
> >
> >
> > Fixes: e94cb37b34eb8 ("Add a drm_aux-dev module for reading/writing dpcd registers")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
> > ---
> >
> > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
> > index 2510717d5a08..e25181bf2c48 100644
> > --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > @@ -63,7 +63,7 @@ static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> >
> > mutex_lock(&aux_idr_mutex);
> > aux_dev = idr_find(&aux_idr, index);
> > - if (!kref_get_unless_zero(&aux_dev->refcount))
> > + if (aux_dev && !kref_get_unless_zero(&aux_dev->refcount))
>
> Dejavu
>
> https://lists.freedesktop.org/archives/dri-devel/2019-May/218855.html
> https://lists.freedesktop.org/archives/dri-devel/2019-July/226168.html
>
> I guess we just got stuck waiting for confirmation that it reproduces
> with the bogus device node trick.
Indeed, i hope it sticks this time!
Zwane
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()
@ 2020-09-08 16:18 ` Zwane Mwaikambo
0 siblings, 0 replies; 22+ messages in thread
From: Zwane Mwaikambo @ 2020-09-08 16:18 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Zwane Mwaikambo, dkwon, Linux Kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 7374 bytes --]
On Mon, 7 Sep 2020, Ville Syrjälä wrote:
> On Fri, Sep 04, 2020 at 12:21:26AM -0700, Zwane Mwaikambo wrote:
> > I observed this when unplugging a DP monitor whilst a computer is asleep
> > and then waking it up. This left DP chardev nodes still being present on
> > the filesystem and accessing these device nodes caused an oops because
> > drm_dp_aux_dev_get_by_minor() assumes a device exists if it is opened.
> > This can also be reproduced by creating a device node with mknod(1) and
> > issuing an open(2)
> >
> > [166164.933198] BUG: kernel NULL pointer dereference, address: 0000000000000018
> > [166164.933202] #PF: supervisor read access in kernel mode
> > [166164.933204] #PF: error_code(0x0000) - not-present page
> > [166164.933205] PGD 0 P4D 0
> > [166164.933208] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > [166164.933211] CPU: 4 PID: 99071 Comm: fwupd Tainted: G W
> > 5.8.0-rc6+ #1
> > [166164.933213] Hardware name: LENOVO 20RD002VUS/20RD002VUS, BIOS R16ET25W
> > (1.11 ) 04/21/2020
> > [166164.933232] RIP: 0010:drm_dp_aux_dev_get_by_minor+0x29/0x70
> > [drm_kms_helper]
> > [166164.933234] Code: 00 0f 1f 44 00 00 55 48 89 e5 41 54 41 89 fc 48 c7
> > c7 60 01 a4 c0 e8 26 ab 30 d7 44 89 e6 48 c7 c7 80 01 a4 c0 e8 47 94 d6 d6
> > <8b> 50 18 49 89 c4 48 8d 78 18 85 d2 74 33 8d 4a 01 89 d0 f0 0f b1
> > [166164.933236] RSP: 0018:ffffb7d7c41cbbf0 EFLAGS: 00010246
> > [166164.933237] RAX: 0000000000000000 RBX: ffff8a90001fe900 RCX: 0000000000000000
> > [166164.933238] RDX: 0000000000000000 RSI: 0000000000000003 RDI: ffffffffc0a40180
> > [166164.933239] RBP: ffffb7d7c41cbbf8 R08: 0000000000000000 R09: ffff8a93e157d6d0
> > [166164.933240] R10: 0000000000000000 R11: ffffffffc0a40188 R12: 0000000000000003
> > [166164.933241] R13: ffff8a9402200e80 R14: ffff8a90001fe900 R15: 0000000000000000
> > [166164.933244] FS: 00007f7fb041eb00(0000) GS:ffff8a9411500000(0000)
> > knlGS:0000000000000000
> > [166164.933245] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [166164.933246] CR2: 0000000000000018 CR3: 00000000352c2003 CR4: 00000000003606e0
> > [166164.933247] Call Trace:
> > [166164.933264] auxdev_open+0x1b/0x40 [drm_kms_helper]
> > [166164.933278] chrdev_open+0xa7/0x1c0
> > [166164.933282] ? cdev_put.part.0+0x20/0x20
> > [166164.933287] do_dentry_open+0x161/0x3c0
> > [166164.933291] vfs_open+0x2d/0x30
> > [166164.933297] path_openat+0xb27/0x10e0
> > [166164.933306] ? atime_needs_update+0x73/0xd0
> > [166164.933309] do_filp_open+0x91/0x100
> > [166164.933313] ? __alloc_fd+0xb2/0x150
> > [166164.933316] do_sys_openat2+0x210/0x2d0
> > [166164.933318] do_sys_open+0x46/0x80
> > [166164.933320] __x64_sys_openat+0x20/0x30
> > [166164.933328] do_syscall_64+0x52/0xc0
> > [166164.933336] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >
> >
> > (gdb) disassemble drm_dp_aux_dev_get_by_minor+0x29
> > Dump of assembler code for function drm_dp_aux_dev_get_by_minor:
> > 0x0000000000017b10 <+0>: callq 0x17b15 <drm_dp_aux_dev_get_by_minor+5>
> > 0x0000000000017b15 <+5>: push %rbp
> > 0x0000000000017b16 <+6>: mov %rsp,%rbp
> > 0x0000000000017b19 <+9>: push %r12
> > 0x0000000000017b1b <+11>: mov %edi,%r12d
> > 0x0000000000017b1e <+14>: mov $0x0,%rdi
> > 0x0000000000017b25 <+21>: callq 0x17b2a <drm_dp_aux_dev_get_by_minor+26>
> > 0x0000000000017b2a <+26>: mov %r12d,%esi
> > 0x0000000000017b2d <+29>: mov $0x0,%rdi
> > 0x0000000000017b34 <+36>: callq 0x17b39 <drm_dp_aux_dev_get_by_minor+41>
> > 0x0000000000017b39 <+41>: mov 0x18(%rax),%edx <=========
> > 0x0000000000017b3c <+44>: mov %rax,%r12
> > 0x0000000000017b3f <+47>: lea 0x18(%rax),%rdi
> > 0x0000000000017b43 <+51>: test %edx,%edx
> > 0x0000000000017b45 <+53>: je 0x17b7a <drm_dp_aux_dev_get_by_minor+106>
> > 0x0000000000017b47 <+55>: lea 0x1(%rdx),%ecx
> > 0x0000000000017b4a <+58>: mov %edx,%eax
> > 0x0000000000017b4c <+60>: lock cmpxchg %ecx,(%rdi)
> > 0x0000000000017b50 <+64>: jne 0x17b76 <drm_dp_aux_dev_get_by_minor+102>
> > 0x0000000000017b52 <+66>: test %edx,%edx
> > 0x0000000000017b54 <+68>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
> > 0x0000000000017b56 <+70>: test %ecx,%ecx
> > 0x0000000000017b58 <+72>: js 0x17b6d <drm_dp_aux_dev_get_by_minor+93>
> > 0x0000000000017b5a <+74>: mov $0x0,%rdi
> > 0x0000000000017b61 <+81>: callq 0x17b66 <drm_dp_aux_dev_get_by_minor+86>
> > 0x0000000000017b66 <+86>: mov %r12,%rax
> > 0x0000000000017b69 <+89>: pop %r12
> > 0x0000000000017b6b <+91>: pop %rbp
> > 0x0000000000017b6c <+92>: retq
> > 0x0000000000017b6d <+93>: xor %esi,%esi
> > 0x0000000000017b6f <+95>: callq 0x17b74 <drm_dp_aux_dev_get_by_minor+100>
> > 0x0000000000017b74 <+100>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
> > 0x0000000000017b76 <+102>: mov %eax,%edx
> > 0x0000000000017b78 <+104>: jmp 0x17b43 <drm_dp_aux_dev_get_by_minor+51>
> > 0x0000000000017b7a <+106>: xor %r12d,%r12d
> > 0x0000000000017b7d <+109>: jmp 0x17b5a <drm_dp_aux_dev_get_by_minor+74>
> > End of assembler dump.
> >
> > (gdb) list *drm_dp_aux_dev_get_by_minor+0x29
> > 0x17b39 is in drm_dp_aux_dev_get_by_minor (drivers/gpu/drm/drm_dp_aux_dev.c:65).
> > 60 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> > 61 {
> > 62 struct drm_dp_aux_dev *aux_dev = NULL;
> > 63
> > 64 mutex_lock(&aux_idr_mutex);
> > 65 aux_dev = idr_find(&aux_idr, index);
> > 66 if (!kref_get_unless_zero(&aux_dev->refcount))
> > 67 aux_dev = NULL;
> > 68 mutex_unlock(&aux_idr_mutex);
> > 69
> > (gdb) p/x &((struct drm_dp_aux_dev *)(0x0))->refcount
> > $8 = 0x18
> >
> > Looking at the caller, checks on the minor are pushed down to
> > drm_dp_aux_dev_get_by_minor()
> >
> > static int auxdev_open(struct inode *inode, struct file *file)
> > {
> > unsigned int minor = iminor(inode);
> > struct drm_dp_aux_dev *aux_dev;
> >
> > aux_dev = drm_dp_aux_dev_get_by_minor(minor); <====
> > if (!aux_dev)
> > return -ENODEV;
> >
> > file->private_data = aux_dev;
> > return 0;
> > }
> >
> >
> > Fixes: e94cb37b34eb8 ("Add a drm_aux-dev module for reading/writing dpcd registers")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Zwane Mwaikambo <zwane@yosper.io>
> > ---
> >
> > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
> > index 2510717d5a08..e25181bf2c48 100644
> > --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > @@ -63,7 +63,7 @@ static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
> >
> > mutex_lock(&aux_idr_mutex);
> > aux_dev = idr_find(&aux_idr, index);
> > - if (!kref_get_unless_zero(&aux_dev->refcount))
> > + if (aux_dev && !kref_get_unless_zero(&aux_dev->refcount))
>
> Dejavu
>
> https://lists.freedesktop.org/archives/dri-devel/2019-May/218855.html
> https://lists.freedesktop.org/archives/dri-devel/2019-July/226168.html
>
> I guess we just got stuck waiting for confirmation that it reproduces
> with the bogus device node trick.
Indeed, i hope it sticks this time!
Zwane
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
2020-08-18 17:58 ` Zwane Mwaikambo
@ 2020-09-08 18:41 ` Lyude Paul
-1 siblings, 0 replies; 22+ messages in thread
From: Lyude Paul @ 2020-09-08 18:41 UTC (permalink / raw)
To: Zwane Mwaikambo; +Cc: Daniel Vetter, tcamuso, dkwon, Linux Kernel, dri-devel
On Tue, 2020-08-18 at 10:58 -0700, Zwane Mwaikambo wrote:
> On Wed, 12 Aug 2020, Lyude Paul wrote:
>
> > On Wed, 2020-08-12 at 16:10 +0200, Daniel Vetter wrote:
> > > On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com>
> > > wrote:
> > > > On Tue, 11 Aug 2020, Daniel Vetter wrote:
> > > >
> > > > > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > > > > Hi Folks,
> > > > > > I know this thread eventually dropped off due to not
> > > > > > identifying
> > > > > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > > > > happened because the udev device nodes for the DP aux devices were
> > > > > > not
> > > > > > cleaned up whereas the kernel had no association with them. I can
> > > > > > reproduce the bug just by creating a device node for a non-
> > > > > > existent
> > > > > > minor
> > > > > > device and calling open().
> > > > >
> > > > > Hm I don't have that thread anymore, but generally these bugs are
> > > > > solved
> > > > > by not registering the device before it's ready for use. We do have
> > > > > drm_connector->late_register for that stuff. Just a guess since I'm
> > > > > not
> > > > > seeing full details here.
> > > >
> > > > In this particular case, the physical device disappeared before the
> > > > nodes
> > > > were cleaned up. It involves putting a computer to sleep with a
> > > > monitor
> > > > plugged in and then waking it up with the monitor unplugged.
> > >
> > > We also have early_unregister for the reverse, but yes this sounds
> > > more tricky ... Adding Lyude who's been working on way too much
> > > lifetime fun around dp recently.
> > > -Daniel
> > >
> > Hi-I think just checking whether the auxdev is NULL or not is a reasonable
> > fix, although I am curious as to how exactly the aux dev's parent is
> > getting
> > destroyed before it's child, which I would have thought would be the only
> > way
> > you could hit this?
>
> Hi, If this is acceptable, would you consider an updated patch against
> 5.8?
Sure-although the process to getting this into stable is to get the patch into
drm-next first, then it can get cherry-picked into the stable kernel branches.
See https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>
> Thanks,
> Zwane
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm: assure aux_dev is nonzero before using it
@ 2020-09-08 18:41 ` Lyude Paul
0 siblings, 0 replies; 22+ messages in thread
From: Lyude Paul @ 2020-09-08 18:41 UTC (permalink / raw)
To: Zwane Mwaikambo; +Cc: dri-devel, dkwon, Linux Kernel, tcamuso
On Tue, 2020-08-18 at 10:58 -0700, Zwane Mwaikambo wrote:
> On Wed, 12 Aug 2020, Lyude Paul wrote:
>
> > On Wed, 2020-08-12 at 16:10 +0200, Daniel Vetter wrote:
> > > On Wed, Aug 12, 2020 at 12:16 AM Zwane Mwaikambo <zwanem@gmail.com>
> > > wrote:
> > > > On Tue, 11 Aug 2020, Daniel Vetter wrote:
> > > >
> > > > > On Mon, Aug 10, 2020 at 10:11:50AM -0700, Zwane Mwaikambo wrote:
> > > > > > Hi Folks,
> > > > > > I know this thread eventually dropped off due to not
> > > > > > identifying
> > > > > > the underlying issue. It's still occuring on 5.8 and in my case it
> > > > > > happened because the udev device nodes for the DP aux devices were
> > > > > > not
> > > > > > cleaned up whereas the kernel had no association with them. I can
> > > > > > reproduce the bug just by creating a device node for a non-
> > > > > > existent
> > > > > > minor
> > > > > > device and calling open().
> > > > >
> > > > > Hm I don't have that thread anymore, but generally these bugs are
> > > > > solved
> > > > > by not registering the device before it's ready for use. We do have
> > > > > drm_connector->late_register for that stuff. Just a guess since I'm
> > > > > not
> > > > > seeing full details here.
> > > >
> > > > In this particular case, the physical device disappeared before the
> > > > nodes
> > > > were cleaned up. It involves putting a computer to sleep with a
> > > > monitor
> > > > plugged in and then waking it up with the monitor unplugged.
> > >
> > > We also have early_unregister for the reverse, but yes this sounds
> > > more tricky ... Adding Lyude who's been working on way too much
> > > lifetime fun around dp recently.
> > > -Daniel
> > >
> > Hi-I think just checking whether the auxdev is NULL or not is a reasonable
> > fix, although I am curious as to how exactly the aux dev's parent is
> > getting
> > destroyed before it's child, which I would have thought would be the only
> > way
> > you could hit this?
>
> Hi, If this is acceptable, would you consider an updated patch against
> 5.8?
Sure-although the process to getting this into stable is to get the patch into
drm-next first, then it can get cherry-picked into the stable kernel branches.
See https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>
> Thanks,
> Zwane
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-09-09 7:04 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-10 17:11 [PATCH] drm: assure aux_dev is nonzero before using it Zwane Mwaikambo
2020-08-10 17:11 ` Zwane Mwaikambo
2020-08-11 8:58 ` Daniel Vetter
2020-08-11 8:58 ` Daniel Vetter
2020-08-11 22:16 ` Zwane Mwaikambo
2020-08-11 22:16 ` Zwane Mwaikambo
2020-08-12 14:10 ` Daniel Vetter
2020-08-12 14:10 ` Daniel Vetter
2020-08-12 15:44 ` Lyude Paul
2020-08-12 15:44 ` Lyude Paul
2020-08-12 20:21 ` Zwane Mwaikambo
2020-08-12 20:21 ` Zwane Mwaikambo
2020-09-04 7:21 ` [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor() Zwane Mwaikambo
2020-09-04 7:21 ` Zwane Mwaikambo
2020-09-07 11:05 ` Ville Syrjälä
2020-09-07 11:05 ` Ville Syrjälä
2020-09-08 16:18 ` Zwane Mwaikambo
2020-09-08 16:18 ` Zwane Mwaikambo
2020-08-18 17:58 ` [PATCH] drm: assure aux_dev is nonzero before using it Zwane Mwaikambo
2020-08-18 17:58 ` Zwane Mwaikambo
2020-09-08 18:41 ` Lyude Paul
2020-09-08 18:41 ` Lyude Paul
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.