| Message ID | alpine.LNX.2.00.1308070258480.10817@pobox.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Wednesday 07 August 2013 03:01:26 Jiri Kosina wrote: > On Tue, 6 Aug 2013, Peter Wu wrote: > > While debugging upowerd (with Logitech Unifying receiver via hidraw), > > I came across this list corruption warning. > > Peter, > > does the patch below fix the problem you are seeing? That one is already in 3.11-rc4 as far as I can see. Also, that code can probably simplified by moving the mutex_unlock after the out label, removing the need to duplicate the mutex_unlock. Remember what I said about "no Oopses"? Well, it turned out that several memory structures were damaged which causes a general protection fault in sock_alloc_inode and other places. I managed to create a program that can reproduce this bug 100% in a QEMU virtual machine with a Logitech USB receiver passed to it. qemu-system-x86_64 -enable-kvm -m 1G -usb -usbdevice host:046d:c52b (pass -kernel, -initrd, -append as needed) Copy hidraw-test to initrd, boot QEMU and run `hidraw-test`. Result: instant (= +/- 2 seconds) crash. I have applied Manoj's patch[1] on top of 3.11-rc4 which seem to fix the issue. One observation is that the new device is named /dev/hidraw1 instead of /dev/hidraw0. Example: f(){ hidraw-test /dev/hidraw$1 usb1;} # needed for 3.11-rc4 f 1; f 1 # crash # needed for 3.11-rc4 + patch f 1; f 2 # ok Regards, Peter [1]: http://lkml.org/lkml/2013/7/22/248 -- /* cc hidraw-test.c -o hidraw-test * hidraw-test /dev/hidraw0 usb1; hidraw-test /dev/hidraw0 usb1; */ #include <unistd.h> #include <fcntl.h> #include <stdio.h> #include <errno.h> #include <string.h> #include <stdlib.h> int open_and_write(const char *path, const char *data) { int sfd, r; sfd = open(path, O_WRONLY); if (sfd < 0) { perror(path); return 1; } r = write(sfd, data, strlen(data)); if (r < 0) { fprintf(stderr, "write(%s, %s): %s\n", path, data, strerror(errno)); return 1; } close(sfd); return 0; } int dork(const char *hiddev, const char *name) { int fd; char c; fd = open(hiddev, O_RDWR | O_NONBLOCK); if (fd < 0) { perror("open"); return 1; } if (open_and_write("/sys/bus/usb/drivers/usb/unbind", name)) return 1; // does not make a difference //sleep(1); if (open_and_write("/sys/bus/usb/drivers/usb/bind", name)) return 1; // allow devices to get discovered sleep(1); printf("read() = %zi\n", read(fd, &c, 1)); perror("read"); close(fd); return 0; } int main(int argc, char **argv) { if (argc < 3) { fprintf(stderr, "Usage: %s /dev/hidrawN usbN\n", *argv); return 1; } system("modprobe -v usbhid"); system("modprobe -v hid-logitech-dj"); dork(argv[1], argv[2]); return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 7 Aug 2013, Peter Wu wrote: > > does the patch below fix the problem you are seeing? > That one is already in 3.11-rc4 as far as I can see. Also, that code can > probably simplified by moving the mutex_unlock after the out label, removing > the need to duplicate the mutex_unlock. > > Remember what I said about "no Oopses"? Well, it turned out that several > memory structures were damaged which causes a general protection fault in > sock_alloc_inode and other places. > > I managed to create a program that can reproduce this bug 100% in a QEMU > virtual machine with a Logitech USB receiver passed to it. > > qemu-system-x86_64 -enable-kvm -m 1G -usb -usbdevice host:046d:c52b > (pass -kernel, -initrd, -append as needed) > > Copy hidraw-test to initrd, boot QEMU and run `hidraw-test`. Result: instant > (= +/- 2 seconds) crash. > > I have applied Manoj's patch[1] on top of 3.11-rc4 which seem to fix the issue. > One observation is that the new device is named /dev/hidraw1 instead of > /dev/hidraw0. Example: > > f(){ hidraw-test /dev/hidraw$1 usb1;} > # needed for 3.11-rc4 > f 1; f 1 # crash > # needed for 3.11-rc4 + patch > f 1; f 2 # ok > > Regards, > Peter > > [1]: http://lkml.org/lkml/2013/7/22/248 That one I am still reviewing ... can I add your Tested-by: to it when I'll be applying it and pushing to Linus? Thanks. > -- > /* cc hidraw-test.c -o hidraw-test > * hidraw-test /dev/hidraw0 usb1; hidraw-test /dev/hidraw0 usb1; > */ > #include <unistd.h> > #include <fcntl.h> > #include <stdio.h> > #include <errno.h> > #include <string.h> > #include <stdlib.h> > > int open_and_write(const char *path, const char *data) { > int sfd, r; > > sfd = open(path, O_WRONLY); > if (sfd < 0) { > perror(path); > return 1; > } > > r = write(sfd, data, strlen(data)); > if (r < 0) { > fprintf(stderr, "write(%s, %s): %s\n", > path, data, strerror(errno)); > return 1; > } > close(sfd); > return 0; > } > > int dork(const char *hiddev, const char *name) { > int fd; > char c; > > fd = open(hiddev, O_RDWR | O_NONBLOCK); > if (fd < 0) { > perror("open"); > return 1; > } > > if (open_and_write("/sys/bus/usb/drivers/usb/unbind", name)) > return 1; > > // does not make a difference > //sleep(1); > > if (open_and_write("/sys/bus/usb/drivers/usb/bind", name)) > return 1; > > // allow devices to get discovered > sleep(1); > > printf("read() = %zi\n", read(fd, &c, 1)); perror("read"); > close(fd); > return 0; > } > > int main(int argc, char **argv) { > if (argc < 3) { > fprintf(stderr, "Usage: %s /dev/hidrawN usbN\n", *argv); > return 1; > } > > system("modprobe -v usbhid"); > system("modprobe -v hid-logitech-dj"); > > dork(argv[1], argv[2]); > > return 0; > } >
Hi Peter, The patch I posted was solving slab memory corruption issue which was occurring because of the race in device disconnect and device release. We found the some of the device data structure being used after free. Later we figure out the patch which was reverted earlier was solving our issue but there was still some slab memory corruption. That was due to reason that list delete of the device was called after freeing the hidraw. I protect drop_ref by mutex lock and also delete the list before calling drop_ref that solve the issue. If you are seeing memory corruption then the patch could solve your issue. Regards -Manoj -----Original Message----- From: Jiri Kosina [mailto:jkosina@suse.cz] Sent: Wednesday, August 07, 2013 7:04 PM To: Peter Wu Cc: linux-input@vger.kernel.org; Manoj Chourasia; linux-kernel@vger.kernel.org; alnovak@suse.cz Subject: Re: List corruption in hidraw_release in 3.11-rc4 On Wed, 7 Aug 2013, Peter Wu wrote: > > does the patch below fix the problem you are seeing? > That one is already in 3.11-rc4 as far as I can see. Also, that code > can probably simplified by moving the mutex_unlock after the out > label, removing the need to duplicate the mutex_unlock. > > Remember what I said about "no Oopses"? Well, it turned out that > several memory structures were damaged which causes a general > protection fault in sock_alloc_inode and other places. > > I managed to create a program that can reproduce this bug 100% in a > QEMU virtual machine with a Logitech USB receiver passed to it. > > qemu-system-x86_64 -enable-kvm -m 1G -usb -usbdevice host:046d:c52b > (pass -kernel, -initrd, -append as needed) > > Copy hidraw-test to initrd, boot QEMU and run `hidraw-test`. Result: > instant (= +/- 2 seconds) crash. > > I have applied Manoj's patch[1] on top of 3.11-rc4 which seem to fix the issue. > One observation is that the new device is named /dev/hidraw1 instead > of /dev/hidraw0. Example: > > f(){ hidraw-test /dev/hidraw$1 usb1;} > # needed for 3.11-rc4 > f 1; f 1 # crash > # needed for 3.11-rc4 + patch > f 1; f 2 # ok > > Regards, > Peter > > [1]: http://lkml.org/lkml/2013/7/22/248 That one I am still reviewing ... can I add your Tested-by: to it when I'll be applying it and pushing to Linus? Thanks. > -- > /* cc hidraw-test.c -o hidraw-test > * hidraw-test /dev/hidraw0 usb1; hidraw-test /dev/hidraw0 usb1; */ > #include <unistd.h> #include <fcntl.h> #include <stdio.h> #include > <errno.h> #include <string.h> #include <stdlib.h> > > int open_and_write(const char *path, const char *data) { > int sfd, r; > > sfd = open(path, O_WRONLY); > if (sfd < 0) { > perror(path); > return 1; > } > > r = write(sfd, data, strlen(data)); > if (r < 0) { > fprintf(stderr, "write(%s, %s): %s\n", > path, data, strerror(errno)); > return 1; > } > close(sfd); > return 0; > } > > int dork(const char *hiddev, const char *name) { > int fd; > char c; > > fd = open(hiddev, O_RDWR | O_NONBLOCK); > if (fd < 0) { > perror("open"); > return 1; > } > > if (open_and_write("/sys/bus/usb/drivers/usb/unbind", name)) > return 1; > > // does not make a difference > //sleep(1); > > if (open_and_write("/sys/bus/usb/drivers/usb/bind", name)) > return 1; > > // allow devices to get discovered > sleep(1); > > printf("read() = %zi\n", read(fd, &c, 1)); perror("read"); > close(fd); > return 0; > } > > int main(int argc, char **argv) { > if (argc < 3) { > fprintf(stderr, "Usage: %s /dev/hidrawN usbN\n", *argv); > return 1; > } > > system("modprobe -v usbhid"); > system("modprobe -v hid-logitech-dj"); > > dork(argv[1], argv[2]); > > return 0; > } > -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 07 August 2013 15:34:08 Jiri Kosina wrote: > On Wed, 7 Aug 2013, Peter Wu wrote: > > [..] > > I have applied Manoj's patch[1] on top of 3.11-rc4 which seem to fix the > > issue. One observation is that the new device is named /dev/hidraw1 > > instead of /dev/hidraw0. Example: > > > > f(){ hidraw-test /dev/hidraw$1 usb1;} > > # needed for 3.11-rc4 > > f 1; f 1 # crash > > # needed for 3.11-rc4 + patch > > f 1; f 2 # ok > > > > Regards, > > Peter > > > > [1]: http://lkml.org/lkml/2013/7/22/248 > > That one I am still reviewing ... can I add your Tested-by: to it when > I'll be applying it and pushing to Linus? Sure, once you accept it you can add: Tested-by: Peter Wu <lekensteyn@gmail.com> While you are at it, are the other functions also safe? (i.e. hidraw_poll, that one is not protected by any locks?) Regards, Peter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c index a745163..6f1feb2 100644 --- a/drivers/hid/hidraw.c +++ b/drivers/hid/hidraw.c @@ -518,7 +518,6 @@ int hidraw_connect(struct hid_device *hid) goto out; } - mutex_unlock(&minors_lock); init_waitqueue_head(&dev->wait); INIT_LIST_HEAD(&dev->list); @@ -528,6 +527,7 @@ int hidraw_connect(struct hid_device *hid) dev->exist = 1; hid->hidraw = dev; + mutex_unlock(&minors_lock); out: return result;