[PATCH] Bluetooth: purge unhandled skbs

[PATCH] Bluetooth: purge unhandled skbs

[Jiri Slaby]

The write handler allocates skbs and queues them into data->readq.
Read side should read them, if there is any. If there is none, skbs
should be dropped by hdev->flush. But this happens only if the device
is HCI_UP, i.e. hdev->power_on work was triggered already. When it was
not, skbs stay allocated in the queue when /dev/vhci is closed. So
purge the queue in ->release.

Program to reproduce:
	#include <err.h>
	#include <fcntl.h>
	#include <stdio.h>
	#include <unistd.h>

	#include <sys/stat.h>
	#include <sys/types.h>
	#include <sys/uio.h>

	int main()
	{
		char buf[] = { 0xff, 0 };
		struct iovec iov = {
			.iov_base = buf,
			.iov_len = sizeof(buf),
		};
		int fd;

		while (1) {
			fd = open("/dev/vhci", O_RDWR);
			if (fd < 0)
				err(1, "open");

			usleep(50);

			if (writev(fd, &iov, 1) < 0)
				err(1, "writev");

			usleep(50);

			close(fd);
		}

		return 0;
	}

Result:
kmemleak: 4609 new suspected memory leaks
unreferenced object 0xffff88059f4d5440 (size 232):
  comm "vhci", pid 1084, jiffies 4294912542 (age 37569.296s)
  hex dump (first 32 bytes):
    20 f0 23 87 05 88 ff ff 20 f0 23 87 05 88 ff ff   .#..... .#.....
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
...
    [<ffffffff81ece010>] __alloc_skb+0x0/0x5a0
    [<ffffffffa021886c>] vhci_create_device+0x5c/0x580 [hci_vhci]
    [<ffffffffa0219436>] vhci_write+0x306/0x4c8 [hci_vhci]

Fixes: 23424c0d31 (Bluetooth: Add support creating virtual AMP controllers)
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: <linux-bluetooth@vger.kernel.org>
Cc: stable 3.13+ <stable@vger.kernel.org>
---
 drivers/bluetooth/hci_vhci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 3ec580e38c17..f67ea1c090cb 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -344,6 +344,7 @@ static int vhci_release(struct inode *inode, struct file *file)
 		hci_free_dev(hdev);
 	}
 
+	skb_queue_purge(&data->readq);
 	file->private_data = NULL;
 	kfree(data);
 
-- 
2.7.4

Re: [PATCH] Bluetooth: purge unhandled skbs

[Marcel Holtmann]

Hi Jiri,

> The write handler allocates skbs and queues them into data->readq.
> Read side should read them, if there is any. If there is none, skbs
> should be dropped by hdev->flush. But this happens only if the device
> is HCI_UP, i.e. hdev->power_on work was triggered already. When it was
> not, skbs stay allocated in the queue when /dev/vhci is closed. So
> purge the queue in ->release.
> 
> Program to reproduce:
> 	#include <err.h>
> 	#include <fcntl.h>
> 	#include <stdio.h>
> 	#include <unistd.h>
> 
> 	#include <sys/stat.h>
> 	#include <sys/types.h>
> 	#include <sys/uio.h>
> 
> 	int main()
> 	{
> 		char buf[] = { 0xff, 0 };
> 		struct iovec iov = {
> 			.iov_base = buf,
> 			.iov_len = sizeof(buf),
> 		};
> 		int fd;
> 
> 		while (1) {
> 			fd = open("/dev/vhci", O_RDWR);
> 			if (fd < 0)
> 				err(1, "open");
> 
> 			usleep(50);
> 
> 			if (writev(fd, &iov, 1) < 0)
> 				err(1, "writev");
> 
> 			usleep(50);
> 
> 			close(fd);
> 		}
> 
> 		return 0;
> 	}
> 
> Result:
> kmemleak: 4609 new suspected memory leaks
> unreferenced object 0xffff88059f4d5440 (size 232):
>  comm "vhci", pid 1084, jiffies 4294912542 (age 37569.296s)
>  hex dump (first 32 bytes):
>    20 f0 23 87 05 88 ff ff 20 f0 23 87 05 88 ff ff   .#..... .#.....
>    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>  backtrace:
> ...
>    [<ffffffff81ece010>] __alloc_skb+0x0/0x5a0
>    [<ffffffffa021886c>] vhci_create_device+0x5c/0x580 [hci_vhci]
>    [<ffffffffa0219436>] vhci_write+0x306/0x4c8 [hci_vhci]
> 
> Fixes: 23424c0d31 (Bluetooth: Add support creating virtual AMP controllers)
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> Cc: <linux-bluetooth@vger.kernel.org>
> Cc: stable 3.13+ <stable@vger.kernel.org>
> ---
> drivers/bluetooth/hci_vhci.c | 1 +
> 1 file changed, 1 insertion(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH] Bluetooth: purge unhandled skbs

[Takashi Iwai]

On Fri, 08 Apr 2016 19:23:45 +0200,
Marcel Holtmann wrote:
> 
> Hi Jiri,
> 
> > The write handler allocates skbs and queues them into data->readq.
> > Read side should read them, if there is any. If there is none, skbs
> > should be dropped by hdev->flush. But this happens only if the device
> > is HCI_UP, i.e. hdev->power_on work was triggered already. When it was
> > not, skbs stay allocated in the queue when /dev/vhci is closed. So
> > purge the queue in ->release.
> > 
> > Program to reproduce:
> > 	#include <err.h>
> > 	#include <fcntl.h>
> > 	#include <stdio.h>
> > 	#include <unistd.h>
> > 
> > 	#include <sys/stat.h>
> > 	#include <sys/types.h>
> > 	#include <sys/uio.h>
> > 
> > 	int main()
> > 	{
> > 		char buf[] = { 0xff, 0 };
> > 		struct iovec iov = {
> > 			.iov_base = buf,
> > 			.iov_len = sizeof(buf),
> > 		};
> > 		int fd;
> > 
> > 		while (1) {
> > 			fd = open("/dev/vhci", O_RDWR);
> > 			if (fd < 0)
> > 				err(1, "open");
> > 
> > 			usleep(50);
> > 
> > 			if (writev(fd, &iov, 1) < 0)
> > 				err(1, "writev");
> > 
> > 			usleep(50);
> > 
> > 			close(fd);
> > 		}
> > 
> > 		return 0;
> > 	}
> > 
> > Result:
> > kmemleak: 4609 new suspected memory leaks
> > unreferenced object 0xffff88059f4d5440 (size 232):
> >  comm "vhci", pid 1084, jiffies 4294912542 (age 37569.296s)
> >  hex dump (first 32 bytes):
> >    20 f0 23 87 05 88 ff ff 20 f0 23 87 05 88 ff ff   .#..... .#.....
> >    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >  backtrace:
> > ...
> >    [<ffffffff81ece010>] __alloc_skb+0x0/0x5a0
> >    [<ffffffffa021886c>] vhci_create_device+0x5c/0x580 [hci_vhci]
> >    [<ffffffffa0219436>] vhci_write+0x306/0x4c8 [hci_vhci]
> > 
> > Fixes: 23424c0d31 (Bluetooth: Add support creating virtual AMP controllers)
> > Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> > Cc: Marcel Holtmann <marcel@holtmann.org>
> > Cc: Gustavo Padovan <gustavo@padovan.org>
> > Cc: Johan Hedberg <johan.hedberg@gmail.com>
> > Cc: <linux-bluetooth@vger.kernel.org>
> > Cc: stable 3.13+ <stable@vger.kernel.org>
> > ---
> > drivers/bluetooth/hci_vhci.c | 1 +
> > 1 file changed, 1 insertion(+)
> 
> patch has been applied to bluetooth-next tree.

Could you check another race of vhci mentioned in the thread?
  https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1106134.html

If the suggested fix is OK, I'll submit a properly cooked patch.


thanks,

Takashi

Re: [PATCH] Bluetooth: purge unhandled skbs

[Marcel Holtmann]

Hi Takashi,

>>> The write handler allocates skbs and queues them into data->readq.
>>> Read side should read them, if there is any. If there is none, skbs
>>> should be dropped by hdev->flush. But this happens only if the device
>>> is HCI_UP, i.e. hdev->power_on work was triggered already. When it was
>>> not, skbs stay allocated in the queue when /dev/vhci is closed. So
>>> purge the queue in ->release.
>>> 
>>> Program to reproduce:
>>> 	#include <err.h>
>>> 	#include <fcntl.h>
>>> 	#include <stdio.h>
>>> 	#include <unistd.h>
>>> 
>>> 	#include <sys/stat.h>
>>> 	#include <sys/types.h>
>>> 	#include <sys/uio.h>
>>> 
>>> 	int main()
>>> 	{
>>> 		char buf[] = { 0xff, 0 };
>>> 		struct iovec iov = {
>>> 			.iov_base = buf,
>>> 			.iov_len = sizeof(buf),
>>> 		};
>>> 		int fd;
>>> 
>>> 		while (1) {
>>> 			fd = open("/dev/vhci", O_RDWR);
>>> 			if (fd < 0)
>>> 				err(1, "open");
>>> 
>>> 			usleep(50);
>>> 
>>> 			if (writev(fd, &iov, 1) < 0)
>>> 				err(1, "writev");
>>> 
>>> 			usleep(50);
>>> 
>>> 			close(fd);
>>> 		}
>>> 
>>> 		return 0;
>>> 	}
>>> 
>>> Result:
>>> kmemleak: 4609 new suspected memory leaks
>>> unreferenced object 0xffff88059f4d5440 (size 232):
>>> comm "vhci", pid 1084, jiffies 4294912542 (age 37569.296s)
>>> hex dump (first 32 bytes):
>>>   20 f0 23 87 05 88 ff ff 20 f0 23 87 05 88 ff ff   .#..... .#.....
>>>   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>> backtrace:
>>> ...
>>>   [<ffffffff81ece010>] __alloc_skb+0x0/0x5a0
>>>   [<ffffffffa021886c>] vhci_create_device+0x5c/0x580 [hci_vhci]
>>>   [<ffffffffa0219436>] vhci_write+0x306/0x4c8 [hci_vhci]
>>> 
>>> Fixes: 23424c0d31 (Bluetooth: Add support creating virtual AMP controllers)
>>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>>> Cc: Marcel Holtmann <marcel@holtmann.org>
>>> Cc: Gustavo Padovan <gustavo@padovan.org>
>>> Cc: Johan Hedberg <johan.hedberg@gmail.com>
>>> Cc: <linux-bluetooth@vger.kernel.org>
>>> Cc: stable 3.13+ <stable@vger.kernel.org>
>>> ---
>>> drivers/bluetooth/hci_vhci.c | 1 +
>>> 1 file changed, 1 insertion(+)
>> 
>> patch has been applied to bluetooth-next tree.
> 
> Could you check another race of vhci mentioned in the thread?
>  https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1106134.html
> 
> If the suggested fix is OK, I'll submit a properly cooked patch.

lets get the patch send to the mailing list and then I can have a look at it. I might need to test the final patch so that it works with legacy userspace.

Regards

Marcel

Re: [PATCH] Bluetooth: purge unhandled skbs

[Takashi Iwai]

On Thu, 14 Apr 2016 16:41:34 +0200,
Marcel Holtmann wrote:
> 
> Hi Takashi,
> 
> >>> The write handler allocates skbs and queues them into data->readq.
> >>> Read side should read them, if there is any. If there is none, skbs
> >>> should be dropped by hdev->flush. But this happens only if the device
> >>> is HCI_UP, i.e. hdev->power_on work was triggered already. When it was
> >>> not, skbs stay allocated in the queue when /dev/vhci is closed. So
> >>> purge the queue in ->release.
> >>> 
> >>> Program to reproduce:
> >>> 	#include <err.h>
> >>> 	#include <fcntl.h>
> >>> 	#include <stdio.h>
> >>> 	#include <unistd.h>
> >>> 
> >>> 	#include <sys/stat.h>
> >>> 	#include <sys/types.h>
> >>> 	#include <sys/uio.h>
> >>> 
> >>> 	int main()
> >>> 	{
> >>> 		char buf[] = { 0xff, 0 };
> >>> 		struct iovec iov = {
> >>> 			.iov_base = buf,
> >>> 			.iov_len = sizeof(buf),
> >>> 		};
> >>> 		int fd;
> >>> 
> >>> 		while (1) {
> >>> 			fd = open("/dev/vhci", O_RDWR);
> >>> 			if (fd < 0)
> >>> 				err(1, "open");
> >>> 
> >>> 			usleep(50);
> >>> 
> >>> 			if (writev(fd, &iov, 1) < 0)
> >>> 				err(1, "writev");
> >>> 
> >>> 			usleep(50);
> >>> 
> >>> 			close(fd);
> >>> 		}
> >>> 
> >>> 		return 0;
> >>> 	}
> >>> 
> >>> Result:
> >>> kmemleak: 4609 new suspected memory leaks
> >>> unreferenced object 0xffff88059f4d5440 (size 232):
> >>> comm "vhci", pid 1084, jiffies 4294912542 (age 37569.296s)
> >>> hex dump (first 32 bytes):
> >>>   20 f0 23 87 05 88 ff ff 20 f0 23 87 05 88 ff ff   .#..... .#.....
> >>>   00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >>> backtrace:
> >>> ...
> >>>   [<ffffffff81ece010>] __alloc_skb+0x0/0x5a0
> >>>   [<ffffffffa021886c>] vhci_create_device+0x5c/0x580 [hci_vhci]
> >>>   [<ffffffffa0219436>] vhci_write+0x306/0x4c8 [hci_vhci]
> >>> 
> >>> Fixes: 23424c0d31 (Bluetooth: Add support creating virtual AMP controllers)
> >>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> >>> Cc: Marcel Holtmann <marcel@holtmann.org>
> >>> Cc: Gustavo Padovan <gustavo@padovan.org>
> >>> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> >>> Cc: <linux-bluetooth@vger.kernel.org>
> >>> Cc: stable 3.13+ <stable@vger.kernel.org>
> >>> ---
> >>> drivers/bluetooth/hci_vhci.c | 1 +
> >>> 1 file changed, 1 insertion(+)
> >> 
> >> patch has been applied to bluetooth-next tree.
> > 
> > Could you check another race of vhci mentioned in the thread?
> >  https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1106134.html
> > 
> > If the suggested fix is OK, I'll submit a properly cooked patch.
> 
> lets get the patch send to the mailing list and then I can have a look at it. I might need to test the final patch so that it works with legacy userspace.

Alright, will submit it soon later.


thanks,

Takashi