Skip to contentSkip to conversation

media: go7007: code improvment and bug fixes

PATCH In review Pavel Skripkin 1 participant 4 messages Jun 20, 2021 – Jul 6, 2021

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover media: go7007: code improvment and bug fixes

In this patch series, I fixed memory leak reported by my local syzkaller instance and increased go7007_alloc() execution speed by removing reduntant zeroing members in kzalloc allocated structure.

Pavel Skripkin (2):
media: go7007: fix memory leak in go7007_usb_probe
media: go7007: remove redundant initialization

 drivers/media/usb/go7007/go7007-driver.c | 26 ------------------------
 drivers/media/usb/go7007/go7007-usb.c    |  2 +-
 2 files changed, 1 insertion(+), 27 deletions(-)
Signature · 1 line
2.32.0
#1 lore raw

PATCH 1/2 media: go7007: fix memory leak in go7007_usb_probe

In commit 137641287eb4 ("go7007: add sanity checking for endpoints") endpoint sanity check was introduced, but if check fails it simply returns with leaked pointers.

Cutted log from my local syzbot instance:

BUG: memory leak
unreferenced object 0xffff8880209f0000 (size 8192):
  comm "kworker/0:4", pid 4916, jiffies 4295263583 (age 29.310s)
  hex dump (first 32 bytes):
    30 b0 27 22 80 88 ff ff 75 73 62 2d 64 75 6d 6d  0.'"....usb-dumm
    79 5f 68 63 64 2e 33 2d 31 00 00 00 00 00 00 00  y_hcd.3-1.......
  backtrace:
    [<ffffffff860ca856>] kmalloc include/linux/slab.h:556 [inline]
    [<ffffffff860ca856>] kzalloc include/linux/slab.h:686 [inline]
    [<ffffffff860ca856>] go7007_alloc+0x46/0xb40 drivers/media/usb/go7007/go7007-driver.c:696
    [<ffffffff860de74e>] go7007_usb_probe+0x13e/0x2200 drivers/media/usb/go7007/go7007-usb.c:1114
    [<ffffffff854a5f74>] usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396
    [<ffffffff845a7151>] really_probe+0x291/0xf60 drivers/base/dd.c:576

BUG: memory leak
unreferenced object 0xffff88801e2f2800 (size 512):
  comm "kworker/0:4", pid 4916, jiffies 4295263583 (age 29.310s)
  hex dump (first 32 bytes):
    00 87 40 8a ff ff ff ff 00 00 00 00 00 00 00 00  ..@.............
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff860de794>] kmalloc include/linux/slab.h:556 [inline]
    [<ffffffff860de794>] kzalloc include/linux/slab.h:686 [inline]
    [<ffffffff860de794>] go7007_usb_probe+0x184/0x2200 drivers/media/usb/go7007/go7007-usb.c:1118
    [<ffffffff854a5f74>] usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396
    [<ffffffff845a7151>] really_probe+0x291/0xf60 drivers/base/dd.c:576
Fixes 137641287eb4 ("go7007: add sanity checking for endpoints")
---
 drivers/media/usb/go7007/go7007-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/usb/go7007/go7007-usb.c +1 -1
index dbf0455d5d50..eeb85981e02b 100644
@@ -1134,7 +1134,7 @@ static int go7007_usb_probe(struct usb_interface *intf,
 
 	ep = usb->usbdev->ep_in[4];
 	if (!ep)
-		return -ENODEV;
+		goto allocfail;
 
 	/* Allocate the URB and buffer for receiving incoming interrupts */
 	usb->intr_urb = usb_alloc_urb(0, GFP_KERNEL);
Signature · 1 line
2.32.0
#2 lore raw

PATCH 2/2 media: go7007: remove redundant initialization

In reply to Pavel Skripkin

In go7007_alloc() kzalloc() is used for struct go7007 allocation. It means that there is no need in zeroing any members, because kzalloc will take care of it.

Removing these reduntant initialization steps increases execution speed a lot:

Before:
+ 86.802 us | go7007_alloc();
After:
+ 29.595 us | go7007_alloc();

Fixes 866b8695d67e8 ("Staging: add the go7007 video driver")
---
 drivers/media/usb/go7007/go7007-driver.c | 26 ------------------------
 1 file changed, 26 deletions(-)
drivers/media/usb/go7007/go7007-driver.c +0 -26
index f1767be9d868..6650eab913d8 100644
@@ -691,49 +691,23 @@ struct go7007 *go7007_alloc(const struct go7007_board_info *board,
 						struct device *dev)
 {
 	struct go7007 *go;
-	int i;
 
 	go = kzalloc(sizeof(struct go7007), GFP_KERNEL);
 	if (go == NULL)
 		return NULL;
 	go->dev = dev;
 	go->board_info = board;
-	go->board_id = 0;
 	go->tuner_type = -1;
-	go->channel_number = 0;
-	go->name[0] = 0;
 	mutex_init(&go->hw_lock);
 	init_waitqueue_head(&go->frame_waitq);
 	spin_lock_init(&go->spinlock);
 	go->status = STATUS_INIT;
-	memset(&go->i2c_adapter, 0, sizeof(go->i2c_adapter));
-	go->i2c_adapter_online = 0;
-	go->interrupt_available = 0;
 	init_waitqueue_head(&go->interrupt_waitq);
-	go->input = 0;
 	go7007_update_board(go);
-	go->encoder_h_halve = 0;
-	go->encoder_v_halve = 0;
-	go->encoder_subsample = 0;
 	go->format = V4L2_PIX_FMT_MJPEG;
 	go->bitrate = 1500000;
 	go->fps_scale = 1;
-	go->pali = 0;
 	go->aspect_ratio = GO7007_RATIO_1_1;
-	go->gop_size = 0;
-	go->ipb = 0;
-	go->closed_gop = 0;
-	go->repeat_seqhead = 0;
-	go->seq_header_enable = 0;
-	go->gop_header_enable = 0;
-	go->dvd_mode = 0;
-	go->interlace_coding = 0;
-	for (i = 0; i < 4; ++i)
-		go->modet[i].enable = 0;
-	for (i = 0; i < 1624; ++i)
-		go->modet_map[i] = 0;
-	go->audio_deliver = NULL;
-	go->audio_enabled = 0;
 
 	return go;
 }
Signature · 1 line
2.32.0
#3 lore raw
In reply to Pavel Skripkin
Quoted 6 lines · On Sun, 20 Jun 2021 22:44:33 +0300 Pavel Skripkin <[email protected]> wrote:

In this patch series, I fixed memory leak reported by my local syzkaller instance and increased go7007_alloc() execution speed by removing reduntant zeroing members in kzalloc allocated structure.

Quoted 8 lines

Pavel Skripkin (2):
media: go7007: fix memory leak in go7007_usb_probe
media: go7007: remove redundant initialization

 drivers/media/usb/go7007/go7007-driver.c | 26

------------------------ drivers/media/usb/go7007/go7007-usb.c |
2 +- 2 files changed, 1 insertion(+), 27 deletions(-)

Hi, media developers!

Gentle ping :)

With regards,
Pavel Skripkin