Re: [PATCH] drm: assure aux_dev is nonzero before using it

Re: [PATCH] drm: assure aux_dev is nonzero before using it

[Zwane Mwaikambo]

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;
}

Re: [PATCH] drm: assure aux_dev is nonzero before using it

[Daniel Vetter]

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

Re: [PATCH] drm: assure aux_dev is nonzero before using it

[Zwane Mwaikambo]

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
> 
> 

Re: [PATCH] drm: assure aux_dev is nonzero before using it

[Daniel Vetter]

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

Re: [PATCH] drm: assure aux_dev is nonzero before using it

[Lyude Paul]

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

Re: [PATCH] drm: assure aux_dev is nonzero before using it

[Zwane Mwaikambo]

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
> > 
> > 
> 

[PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()

[Zwane Mwaikambo]

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);
 

Re: [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()

[Ville Syrjälä]

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

Re: [PATCH]] drm/dp check aux_dev before use in drm_dp_aux_dev_get_by_minor()

[Zwane Mwaikambo]

[-- 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

Re: [PATCH] drm: assure aux_dev is nonzero before using it

[Zwane Mwaikambo]

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

Re: [PATCH] drm: assure aux_dev is nonzero before using it

[Lyude Paul]

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