[PATCH 0/6] get rid of on-stack dma buffers

[PATCH 0/6] get rid of on-stack dma buffers

[Florian Mickler]

Hi all!

These patches get rid of on-stack dma buffers for some of the dvb-usb drivers. 
I do not own the hardware, so these are only compile tested. I would 
appreciate testing and review.
They were previously sent to the list, but some error on my side
prevented (some of?) them from beeing delivered to all parties (the lists).

These changes are motivated by 
https://bugzilla.kernel.org/show_bug.cgi?id=15977 .

The patches which got tested already were submitted to Mauro (and
lkml/linux-media) yesterday seperately. Those fix this same issue for ec168,
ce6230, au6610 and lmedm04. 

A fix for vp702x has been submitted seperately for review on the list. I have
similiar fixes like the vp702x-fix for dib0700 (overlooked some on-stack
buffers in there in my original submission as well) and gp8psk, but I am
holding them back 'till I got time to recheck those and getting some feedback
on vp702x.

Please review and test.

Regards,
Flo

Florian Mickler (6):
  [media] a800: get rid of on-stack dma buffers
  [media v2] vp7045: get rid of on-stack dma buffers
  [media] friio: get rid of on-stack dma buffers
  [media] dw2102: get rid of on-stack dma buffer
  [media] m920x: get rid of on-stack dma buffers
  [media] opera1: get rid of on-stack dma buffer

 drivers/media/dvb/dvb-usb/a800.c   |   17 ++++++++++---
 drivers/media/dvb/dvb-usb/dw2102.c |   10 ++++++-
 drivers/media/dvb/dvb-usb/friio.c  |   23 ++++++++++++++---
 drivers/media/dvb/dvb-usb/m920x.c  |   33 ++++++++++++++++--------
 drivers/media/dvb/dvb-usb/opera1.c |   31 +++++++++++++++--------
 drivers/media/dvb/dvb-usb/vp7045.c |   47 ++++++++++++++++++++++++++----------
 6 files changed, 116 insertions(+), 45 deletions(-)

-- 
1.7.4.1

[PATCH 1/6] [media] a800: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/a800.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/a800.c b/drivers/media/dvb/dvb-usb/a800.c
index 53b93a4..fe2366b 100644
--- a/drivers/media/dvb/dvb-usb/a800.c
+++ b/drivers/media/dvb/dvb-usb/a800.c
@@ -78,17 +78,26 @@ static struct rc_map_table rc_map_a800_table[] = {
 
 static int a800_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 {
-	u8 key[5];
+	int ret;
+	u8 *key = kmalloc(5, GFP_KERNEL);
+	if (!key)
+		return -ENOMEM;
+
 	if (usb_control_msg(d->udev,usb_rcvctrlpipe(d->udev,0),
 				0x04, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0, key, 5,
-				2000) != 5)
-		return -ENODEV;
+				2000) != 5) {
+		ret = -ENODEV;
+		goto out;
+	}
 
 	/* call the universal NEC remote processor, to find out the key's state and event */
 	dvb_usb_nec_rc_key_to_event(d,key,event,state);
 	if (key[0] != 0)
 		deb_rc("key: %x %x %x %x %x\n",key[0],key[1],key[2],key[3],key[4]);
-	return 0;
+	ret = 0;
+out:
+	kfree(key);
+	return ret;
 }
 
 /* USB Driver stuff */
-- 
1.7.4.1

[PATCH 2/6 v2] [media] vp7045: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>

---
[v2: pulled buffer access under the mutex]

drivers/media/dvb/dvb-usb/vp7045.c |   47 ++++++++++++++++++++++++++----------
 1 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/vp7045.c b/drivers/media/dvb/dvb-usb/vp7045.c
index ab0ab3c..3db89e3 100644
--- a/drivers/media/dvb/dvb-usb/vp7045.c
+++ b/drivers/media/dvb/dvb-usb/vp7045.c
@@ -28,9 +28,9 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr);
 int vp7045_usb_op(struct dvb_usb_device *d, u8 cmd, u8 *out, int outlen, u8 *in, int inlen, int msec)
 {
 	int ret = 0;
-	u8 inbuf[12] = { 0 }, outbuf[20] = { 0 };
+	u8 *buf = d->priv;
 
-	outbuf[0] = cmd;
+	buf[0] = cmd;
 
 	if (outlen > 19)
 		outlen = 19;
@@ -38,19 +38,21 @@ int vp7045_usb_op(struct dvb_usb_device *d, u8 cmd, u8 *out, int outlen, u8 *in,
 	if (inlen > 11)
 		inlen = 11;
 
+	ret = mutex_lock_interruptible(&d->usb_mutex);
+	if (ret)
+		return ret;
+
 	if (out != NULL && outlen > 0)
-		memcpy(&outbuf[1], out, outlen);
+		memcpy(&buf[1], out, outlen);
 
 	deb_xfer("out buffer: ");
-	debug_dump(outbuf,outlen+1,deb_xfer);
+	debug_dump(buf, outlen+1, deb_xfer);
 
-	if ((ret = mutex_lock_interruptible(&d->usb_mutex)))
-		return ret;
 
 	if (usb_control_msg(d->udev,
 			usb_sndctrlpipe(d->udev,0),
 			TH_COMMAND_OUT, USB_TYPE_VENDOR | USB_DIR_OUT, 0, 0,
-			outbuf, 20, 2000) != 20) {
+			buf, 20, 2000) != 20) {
 		err("USB control message 'out' went wrong.");
 		ret = -EIO;
 		goto unlock;
@@ -61,17 +63,17 @@ int vp7045_usb_op(struct dvb_usb_device *d, u8 cmd, u8 *out, int outlen, u8 *in,
 	if (usb_control_msg(d->udev,
 			usb_rcvctrlpipe(d->udev,0),
 			TH_COMMAND_IN, USB_TYPE_VENDOR | USB_DIR_IN, 0, 0,
-			inbuf, 12, 2000) != 12) {
+			buf, 12, 2000) != 12) {
 		err("USB control message 'in' went wrong.");
 		ret = -EIO;
 		goto unlock;
 	}
 
 	deb_xfer("in buffer: ");
-	debug_dump(inbuf,12,deb_xfer);
+	debug_dump(buf, 12, deb_xfer);
 
 	if (in != NULL && inlen > 0)
-		memcpy(in,&inbuf[1],inlen);
+		memcpy(in, &buf[1], inlen);
 
 unlock:
 	mutex_unlock(&d->usb_mutex);
@@ -222,8 +224,26 @@ static struct dvb_usb_device_properties vp7045_properties;
 static int vp7045_usb_probe(struct usb_interface *intf,
 		const struct usb_device_id *id)
 {
-	return dvb_usb_device_init(intf, &vp7045_properties,
-				   THIS_MODULE, NULL, adapter_nr);
+	struct dvb_usb_device *d;
+	int ret = dvb_usb_device_init(intf, &vp7045_properties,
+				   THIS_MODULE, &d, adapter_nr);
+	if (ret)
+		return ret;
+
+	d->priv = kmalloc(20, GFP_KERNEL);
+	if (!d->priv) {
+		dvb_usb_device_exit(intf);
+		return -ENOMEM;
+	}
+
+	return ret;
+}
+
+static void vp7045_usb_disconnect(struct usb_interface *intf)
+{
+	struct dvb_usb_device *d = usb_get_intfdata(intf);
+	kfree(d->priv);
+	dvb_usb_device_exit(intf);
 }
 
 static struct usb_device_id vp7045_usb_table [] = {
@@ -238,6 +258,7 @@ MODULE_DEVICE_TABLE(usb, vp7045_usb_table);
 static struct dvb_usb_device_properties vp7045_properties = {
 	.usb_ctrl = CYPRESS_FX2,
 	.firmware = "dvb-usb-vp7045-01.fw",
+	.size_of_priv = sizeof(u8 *),
 
 	.num_adapters = 1,
 	.adapter = {
@@ -284,7 +305,7 @@ static struct dvb_usb_device_properties vp7045_properties = {
 static struct usb_driver vp7045_usb_driver = {
 	.name		= "dvb_usb_vp7045",
 	.probe		= vp7045_usb_probe,
-	.disconnect = dvb_usb_device_exit,
+	.disconnect	= vp7045_usb_disconnect,
 	.id_table	= vp7045_usb_table,
 };
 
-- 
1.7.4.1

[PATCH 3/6] [media] friio: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/friio.c |   23 +++++++++++++++++++----
 1 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/friio.c b/drivers/media/dvb/dvb-usb/friio.c
index 14a65b4..76159ae 100644
--- a/drivers/media/dvb/dvb-usb/friio.c
+++ b/drivers/media/dvb/dvb-usb/friio.c
@@ -142,17 +142,20 @@ static u32 gl861_i2c_func(struct i2c_adapter *adapter)
 	return I2C_FUNC_I2C;
 }
 
-
 static int friio_ext_ctl(struct dvb_usb_adapter *adap,
 			 u32 sat_color, int lnb_on)
 {
 	int i;
 	int ret;
 	struct i2c_msg msg;
-	u8 buf[2];
+	u8 *buf;
 	u32 mask;
 	u8 lnb = (lnb_on) ? FRIIO_CTL_LNB : 0;
 
+	buf = kmalloc(2, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
 	msg.addr = 0x00;
 	msg.flags = 0;
 	msg.len = 2;
@@ -189,6 +192,7 @@ static int friio_ext_ctl(struct dvb_usb_adapter *adap,
 	buf[1] |= FRIIO_CTL_CLK;
 	ret += gl861_i2c_xfer(&adap->dev->i2c_adap, &msg, 1);
 
+	kfree(buf);
 	return (ret == 70);
 }
 
@@ -219,11 +223,20 @@ static int friio_initialize(struct dvb_usb_device *d)
 	int ret;
 	int i;
 	int retry = 0;
-	u8 rbuf[2];
-	u8 wbuf[3];
+	u8 *rbuf, *wbuf;
 
 	deb_info("%s called.\n", __func__);
 
+	wbuf = kmalloc(3, GFP_KERNEL);
+	if (!wbuf)
+		return -ENOMEM;
+
+	rbuf = kmalloc(2, GFP_KERNEL);
+	if (!rbuf) {
+		kfree(wbuf);
+		return -ENOMEM;
+	}
+
 	/* use gl861_i2c_msg instead of gl861_i2c_xfer(), */
 	/* because the i2c device is not set up yet. */
 	wbuf[0] = 0x11;
@@ -358,6 +371,8 @@ restart:
 	return 0;
 
 error:
+	kfree(wbuf);
+	kfree(rbuf);
 	deb_info("%s:ret == %d\n", __func__, ret);
 	return -EIO;
 }
-- 
1.7.4.1

[PATCH 4/6] [media] dw2102: get rid of on-stack dma buffer

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/dw2102.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/dw2102.c b/drivers/media/dvb/dvb-usb/dw2102.c
index 2c307ba..4c8ff39 100644
--- a/drivers/media/dvb/dvb-usb/dw2102.c
+++ b/drivers/media/dvb/dvb-usb/dw2102.c
@@ -101,12 +101,16 @@ static int dw210x_op_rw(struct usb_device *dev, u8 request, u16 value,
 			u16 index, u8 * data, u16 len, int flags)
 {
 	int ret;
-	u8 u8buf[len];
-
+	u8 *u8buf;
 	unsigned int pipe = (flags == DW210X_READ_MSG) ?
 				usb_rcvctrlpipe(dev, 0) : usb_sndctrlpipe(dev, 0);
 	u8 request_type = (flags == DW210X_READ_MSG) ? USB_DIR_IN : USB_DIR_OUT;
 
+	u8buf = kmalloc(len, GFP_KERNEL);
+	if (!u8buf)
+		return -ENOMEM;
+
+
 	if (flags == DW210X_WRITE_MSG)
 		memcpy(u8buf, data, len);
 	ret = usb_control_msg(dev, pipe, request, request_type | USB_TYPE_VENDOR,
@@ -114,6 +118,8 @@ static int dw210x_op_rw(struct usb_device *dev, u8 request, u16 value,
 
 	if (flags == DW210X_READ_MSG)
 		memcpy(data, u8buf, len);
+
+	kfree(u8buf);
 	return ret;
 }
 
-- 
1.7.4.1

[PATCH 5/6] [media] m920x: get rid of on-stack dma buffers

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/m920x.c |   33 ++++++++++++++++++++++-----------
 1 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/m920x.c b/drivers/media/dvb/dvb-usb/m920x.c
index da9dc91..f66eaa3 100644
--- a/drivers/media/dvb/dvb-usb/m920x.c
+++ b/drivers/media/dvb/dvb-usb/m920x.c
@@ -134,13 +134,17 @@ static int m920x_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 {
 	struct m920x_state *m = d->priv;
 	int i, ret = 0;
-	u8 rc_state[2];
+	u8 *rc_state;
+
+	rc_state = kmalloc(2, GFP_KERNEL);
+	if (!rc_state)
+		return -ENOMEM;
 
 	if ((ret = m920x_read(d->udev, M9206_CORE, 0x0, M9206_RC_STATE, rc_state, 1)) != 0)
-		goto unlock;
+		goto out;
 
 	if ((ret = m920x_read(d->udev, M9206_CORE, 0x0, M9206_RC_KEY, rc_state + 1, 1)) != 0)
-		goto unlock;
+		goto out;
 
 	for (i = 0; i < d->props.rc.legacy.rc_map_size; i++)
 		if (rc5_data(&d->props.rc.legacy.rc_map_table[i]) == rc_state[1]) {
@@ -149,7 +153,7 @@ static int m920x_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 			switch(rc_state[0]) {
 			case 0x80:
 				*state = REMOTE_NO_KEY_PRESSED;
-				goto unlock;
+				goto out;
 
 			case 0x88: /* framing error or "invalid code" */
 			case 0x99:
@@ -157,7 +161,7 @@ static int m920x_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 			case 0xd8:
 				*state = REMOTE_NO_KEY_PRESSED;
 				m->rep_count = 0;
-				goto unlock;
+				goto out;
 
 			case 0x93:
 			case 0x92:
@@ -165,7 +169,7 @@ static int m920x_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 			case 0x82:
 				m->rep_count = 0;
 				*state = REMOTE_KEY_PRESSED;
-				goto unlock;
+				goto out;
 
 			case 0x91:
 			case 0x81: /* pinnacle PCTV310e */
@@ -174,12 +178,12 @@ static int m920x_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 					*state = REMOTE_KEY_REPEAT;
 				else
 					*state = REMOTE_NO_KEY_PRESSED;
-				goto unlock;
+				goto out;
 
 			default:
 				deb("Unexpected rc state %02x\n", rc_state[0]);
 				*state = REMOTE_NO_KEY_PRESSED;
-				goto unlock;
+				goto out;
 			}
 		}
 
@@ -188,8 +192,8 @@ static int m920x_rc_query(struct dvb_usb_device *d, u32 *event, int *state)
 
 	*state = REMOTE_NO_KEY_PRESSED;
 
- unlock:
-
+ out:
+	kfree(rc_state);
 	return ret;
 }
 
@@ -339,13 +343,19 @@ static int m920x_pid_filter(struct dvb_usb_adapter *adap, int index, u16 pid, in
 static int m920x_firmware_download(struct usb_device *udev, const struct firmware *fw)
 {
 	u16 value, index, size;
-	u8 read[4], *buff;
+	u8 *read, *buff;
 	int i, pass, ret = 0;
 
 	buff = kmalloc(65536, GFP_KERNEL);
 	if (buff == NULL)
 		return -ENOMEM;
 
+	read = kmalloc(4, GFP_KERNEL);
+	if (!read) {
+		kfree(buff);
+		return -ENOMEM;
+	}
+
 	if ((ret = m920x_read(udev, M9206_FILTER, 0x0, 0x8000, read, 4)) != 0)
 		goto done;
 	deb("%x %x %x %x\n", read[0], read[1], read[2], read[3]);
@@ -396,6 +406,7 @@ static int m920x_firmware_download(struct usb_device *udev, const struct firmwar
 	deb("firmware uploaded!\n");
 
  done:
+	kfree(read);
 	kfree(buff);
 
 	return ret;
-- 
1.7.4.1

[PATCH 6/6] [media] opera1: get rid of on-stack dma buffer

[Florian Mickler]

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
	WARNING: at lib/dma-debug.c:866 check_for_stack

Note: This change is tested to compile only, as I don't have the hardware.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 drivers/media/dvb/dvb-usb/opera1.c |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/opera1.c b/drivers/media/dvb/dvb-usb/opera1.c
index 1f1b7d6..2ca6e87 100644
--- a/drivers/media/dvb/dvb-usb/opera1.c
+++ b/drivers/media/dvb/dvb-usb/opera1.c
@@ -53,27 +53,36 @@ static int opera1_xilinx_rw(struct usb_device *dev, u8 request, u16 value,
 			    u8 * data, u16 len, int flags)
 {
 	int ret;
-	u8 r;
-	u8 u8buf[len];
-
+	u8 tmp;
+	u8 *buf;
 	unsigned int pipe = (flags == OPERA_READ_MSG) ?
 		usb_rcvctrlpipe(dev,0) : usb_sndctrlpipe(dev, 0);
 	u8 request_type = (flags == OPERA_READ_MSG) ? USB_DIR_IN : USB_DIR_OUT;
 
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
 	if (flags == OPERA_WRITE_MSG)
-		memcpy(u8buf, data, len);
-	ret =
-		usb_control_msg(dev, pipe, request, request_type | USB_TYPE_VENDOR,
-			value, 0x0, u8buf, len, 2000);
+		memcpy(buf, data, len);
+	ret = usb_control_msg(dev, pipe, request,
+			request_type | USB_TYPE_VENDOR, value, 0x0,
+			buf, len, 2000);
 
 	if (request == OPERA_TUNER_REQ) {
+		tmp = buf[0];
 		if (usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
-				OPERA_TUNER_REQ, USB_DIR_IN | USB_TYPE_VENDOR,
-				0x01, 0x0, &r, 1, 2000)<1 || r!=0x08)
-					return 0;
+			    OPERA_TUNER_REQ, USB_DIR_IN | USB_TYPE_VENDOR,
+			    0x01, 0x0, buf, 1, 2000) < 1 || buf[0] != 0x08) {
+			ret = 0;
+			goto out;
+		}
+		buf[0] = tmp;
 	}
 	if (flags == OPERA_READ_MSG)
-		memcpy(data, u8buf, len);
+		memcpy(data, buf, len);
+out:
+	kfree(buf);
 	return ret;
 }
 
-- 
1.7.4.1

Re: [PATCH 0/6] get rid of on-stack dma buffers

[Andy Walls]

Florian Mickler <florian@mickler.org> wrote:

>Hi all!
>
>These patches get rid of on-stack dma buffers for some of the dvb-usb
>drivers. 
>I do not own the hardware, so these are only compile tested. I would 
>appreciate testing and review.
>They were previously sent to the list, but some error on my side
>prevented (some of?) them from beeing delivered to all parties (the
>lists).
>
>These changes are motivated by 
>https://bugzilla.kernel.org/show_bug.cgi?id=15977 .
>
>The patches which got tested already were submitted to Mauro (and
>lkml/linux-media) yesterday seperately. Those fix this same issue for
>ec168,
>ce6230, au6610 and lmedm04. 
>
>A fix for vp702x has been submitted seperately for review on the list.
>I have
>similiar fixes like the vp702x-fix for dib0700 (overlooked some
>on-stack
>buffers in there in my original submission as well) and gp8psk, but I
>am
>holding them back 'till I got time to recheck those and getting some
>feedback
>on vp702x.
>
>Please review and test.
>
>Regards,
>Flo
>
>Florian Mickler (6):
>  [media] a800: get rid of on-stack dma buffers
>  [media v2] vp7045: get rid of on-stack dma buffers
>  [media] friio: get rid of on-stack dma buffers
>  [media] dw2102: get rid of on-stack dma buffer
>  [media] m920x: get rid of on-stack dma buffers
>  [media] opera1: get rid of on-stack dma buffer
>
> drivers/media/dvb/dvb-usb/a800.c   |   17 ++++++++++---
> drivers/media/dvb/dvb-usb/dw2102.c |   10 ++++++-
> drivers/media/dvb/dvb-usb/friio.c  |   23 ++++++++++++++---
> drivers/media/dvb/dvb-usb/m920x.c  |   33 ++++++++++++++++--------
> drivers/media/dvb/dvb-usb/opera1.c |   31 +++++++++++++++--------
>drivers/media/dvb/dvb-usb/vp7045.c |   47
>++++++++++++++++++++++++++----------
> 6 files changed, 116 insertions(+), 45 deletions(-)
>
>-- 
>1.7.4.1
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media"
>in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Florian,

For all of these, what happens when the USB call times out and you kfree() the buffer?  Can the USB DMA actually complete after this kfree(), possibly corrupting space that has been reallocated off the heap, since the kfree()?

This is the scenario for which I assume allocating off the stack is bad.  

Do these changes simply make corruption less noticable since heap gets corrupted vs stack?

Regards,
Andy

Re: [PATCH 0/6] get rid of on-stack dma buffers

[Florian Mickler]

On Mon, 21 Mar 2011 15:26:43 -0400
Andy Walls <awalls@md.metrocast.net> wrote:

> Florian Mickler <florian@mickler.org> wrote:
> 
> >Hi all!
> >
> >These patches get rid of on-stack dma buffers for some of the dvb-usb
> >drivers. 
> >I do not own the hardware, so these are only compile tested. I would 
> >appreciate testing and review.
> >They were previously sent to the list, but some error on my side
> >prevented (some of?) them from beeing delivered to all parties (the
> >lists).
> >
> >These changes are motivated by 
> >https://bugzilla.kernel.org/show_bug.cgi?id=15977 .
> >
> >The patches which got tested already were submitted to Mauro (and
> >lkml/linux-media) yesterday seperately. Those fix this same issue for
> >ec168,
> >ce6230, au6610 and lmedm04. 
> >
> >A fix for vp702x has been submitted seperately for review on the list.
> >I have
> >similiar fixes like the vp702x-fix for dib0700 (overlooked some
> >on-stack
> >buffers in there in my original submission as well) and gp8psk, but I
> >am
> >holding them back 'till I got time to recheck those and getting some
> >feedback
> >on vp702x.
> >
> >Please review and test.
> >
> >Regards,
> >Flo
> >
> >Florian Mickler (6):
> >  [media] a800: get rid of on-stack dma buffers
> >  [media v2] vp7045: get rid of on-stack dma buffers
> >  [media] friio: get rid of on-stack dma buffers
> >  [media] dw2102: get rid of on-stack dma buffer
> >  [media] m920x: get rid of on-stack dma buffers
> >  [media] opera1: get rid of on-stack dma buffer
> >
> > drivers/media/dvb/dvb-usb/a800.c   |   17 ++++++++++---
> > drivers/media/dvb/dvb-usb/dw2102.c |   10 ++++++-
> > drivers/media/dvb/dvb-usb/friio.c  |   23 ++++++++++++++---
> > drivers/media/dvb/dvb-usb/m920x.c  |   33 ++++++++++++++++--------
> > drivers/media/dvb/dvb-usb/opera1.c |   31 +++++++++++++++--------
> >drivers/media/dvb/dvb-usb/vp7045.c |   47
> >++++++++++++++++++++++++++----------
> > 6 files changed, 116 insertions(+), 45 deletions(-)
> >
> >-- 
> >1.7.4.1
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-media"
> >in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

> Florian,
> 
> For all of these, what happens when the USB call times out and you kfree() the buffer?  Can the USB DMA actually complete after this kfree(), possibly corrupting space that has been reallocated off the heap, since the kfree()?
> 
> This is the scenario for which I assume allocating off the stack is bad.  
> 
> Do these changes simply make corruption less noticable since heap gets corrupted vs stack?
> 
> Regards,
> Andy

To be blunt, I'm not shure I fully understand the requirements myself.
But as far as I grasped it, the main problem is that we need memory
which the processor can see as soon as the device has scribbled upon
it. (think caches and the like)

Somewhere down the line, the buffer to usb_control_msg get's to be
a parameter to dma_map_single which is described as part of
the DMA API in Documentation/DMA-API.txt 

The main point I filter out from that is that the memory has to begin
exactly at a cache line boundary... 

I guess (not verified), that the dma api takes sufficient precautions
to abort the dma transfer if a timeout happens.  So freeing _should_
not be an issue. (At least, I would expect big fat warnings everywhere
if that were the case)

I cc'd some people that hopefully will correct me if I'm wrong...

regards,
Flo

Re: [PATCH 0/6] get rid of on-stack dma buffers

[Roedel, Joerg]

On Mon, Mar 21, 2011 at 05:03:15PM -0400, Florian Mickler wrote:
> I guess (not verified), that the dma api takes sufficient precautions
> to abort the dma transfer if a timeout happens.  So freeing _should_
> not be an issue. (At least, I would expect big fat warnings everywhere
> if that were the case)

Freeing is very well an issue. All you can expect from the DMA-API is to
give you a valid DMA handle for your device. But it can not prevent that
a device uses this handle after you returned it. You need to make sure
yourself that any pending DMA is canceled before calling kfree().


		Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 0/6] get rid of on-stack dma buffers

[Jiri Kosina]

On Mon, 21 Mar 2011, Florian Mickler wrote:

> To be blunt, I'm not shure I fully understand the requirements myself. 
> But as far as I grasped it, the main problem is that we need memory 
> which the processor can see as soon as the device has scribbled upon it. 
> (think caches and the like)
> 
> Somewhere down the line, the buffer to usb_control_msg get's to be a 
> parameter to dma_map_single which is described as part of the DMA API in 
> Documentation/DMA-API.txt
> 
> The main point I filter out from that is that the memory has to begin
> exactly at a cache line boundary... 
> 
> I guess (not verified), that the dma api takes sufficient precautions to 
> abort the dma transfer if a timeout happens.  So freeing _should_ not be 
> an issue. (At least, I would expect big fat warnings everywhere if that 
> were the case)
> 
> I cc'd some people that hopefully will correct me if I'm wrong...

It all boils down to making sure that you don't free the memory which is 
used as target of DMA transfer.

This is very likely to hit you when DMA memory region is on stack, but 
blindly just converting this to kmalloc()/kfree() isn't any better if you 
don't make sure that all the DMA transfers have been finished and device 
will not be making any more to that particular memory region.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 0/6] get rid of on-stack dma buffers

[Johannes Stezenbach]

On Tue, Mar 22, 2011 at 11:59:32AM +0100, Jiri Kosina wrote:
> On Mon, 21 Mar 2011, Florian Mickler wrote:
> 
> > To be blunt, I'm not shure I fully understand the requirements myself. 
> > But as far as I grasped it, the main problem is that we need memory 
> > which the processor can see as soon as the device has scribbled upon it. 
> > (think caches and the like)
> > 
> > Somewhere down the line, the buffer to usb_control_msg get's to be a 
> > parameter to dma_map_single which is described as part of the DMA API in 
> > Documentation/DMA-API.txt
> > 
> > The main point I filter out from that is that the memory has to begin
> > exactly at a cache line boundary... 
> > 
> > I guess (not verified), that the dma api takes sufficient precautions to 
> > abort the dma transfer if a timeout happens.  So freeing _should_ not be 
> > an issue. (At least, I would expect big fat warnings everywhere if that 
> > were the case)
> > 
> > I cc'd some people that hopefully will correct me if I'm wrong...
> 
> It all boils down to making sure that you don't free the memory which is 
> used as target of DMA transfer.
> 
> This is very likely to hit you when DMA memory region is on stack, but 
> blindly just converting this to kmalloc()/kfree() isn't any better if you 
> don't make sure that all the DMA transfers have been finished and device 
> will not be making any more to that particular memory region.

I think it is important that one cache line is not shared between
a DMA buffer and other objects, especially on architectures where
cache coherency is managed in software (ARM, MIPS etc.).  If you
use kmalloc() for the DMA buffer that is guaranteed.
(E.g. DMA API will do writeback/invalidate before the DMA starts, but
if the CPU accesses a variable which is next to the buffer
while DMA is still pending then the whole cacheline is read back into
the cache.  If the CPU is then trying to read the DMA buffer after
the DMA finished it will see stale data from the cache.  You lose.)

It depends on the device doing DMA if it needs special alignment.
If you meet its alignment requirements, and wait for the end of the DMA before
returning, and make sure the buffer does not share cache lines with
neighbouring objects on the stack, then you can use DMA buffers on
stack.  In practice it's simpler and much less error prone to use kmalloc().

Regarding usb_control_msg(), since it is a synchronous API which
waits for the end of the transfer I'm relatively sure there is no
DMA pending when it returns, even if it aborts with timeout or any
other error code.


Johannes

Re: [PATCH 0/6] get rid of on-stack dma buffers

[Oliver Neukum]

Am Dienstag, 22. März 2011, 14:08:17 schrieb Florian Mickler:
> Am 22.03.2011 12:10 schrieb "Roedel, Joerg" <Joerg.Roedel@amd.com>:
> >
> > On Mon, Mar 21, 2011 at 05:03:15PM -0400, Florian Mickler wrote:
> > > I guess (not verified), that the dma api takes sufficient precautions
> > > to abort the dma transfer if a timeout happens.  So freeing _should_
> > > not be an issue. (At least, I would expect big fat warnings everywhere
> > > if that were the case)
> >
> > Freeing is very well an issue. All you can expect from the DMA-API is to
> > give you a valid DMA handle for your device. But it can not prevent that
> > a device uses this handle after you returned it. You need to make sure
> > yourself that any pending DMA is canceled before calling kfree().
> 
> Does usb_control_msg do this? It waits for completion but takes also a
> timeout parameter. I will recheck this once I'm home.

It uses usb_start_wait_urb() which upon a timeout kills the URB. The
buffer is unused after usb_control_msg() returns.

	HTH
		Oliver

Re: [PATCH 0/6] get rid of on-stack dma buffers

[James Bottomley]

On Mon, 2011-03-21 at 22:03 +0100, Florian Mickler wrote:
> On Mon, 21 Mar 2011 15:26:43 -0400
> Andy Walls <awalls@md.metrocast.net> wrote:
> 
> > Florian Mickler <florian@mickler.org> wrote:
> > 
> > >Hi all!
> > >
> > >These patches get rid of on-stack dma buffers for some of the dvb-usb
> > >drivers. 
> > >I do not own the hardware, so these are only compile tested. I would 
> > >appreciate testing and review.
> > >They were previously sent to the list, but some error on my side
> > >prevented (some of?) them from beeing delivered to all parties (the
> > >lists).
> > >
> > >These changes are motivated by 
> > >https://bugzilla.kernel.org/show_bug.cgi?id=15977 .
> > >
> > >The patches which got tested already were submitted to Mauro (and
> > >lkml/linux-media) yesterday seperately. Those fix this same issue for
> > >ec168,
> > >ce6230, au6610 and lmedm04. 
> > >
> > >A fix for vp702x has been submitted seperately for review on the list.
> > >I have
> > >similiar fixes like the vp702x-fix for dib0700 (overlooked some
> > >on-stack
> > >buffers in there in my original submission as well) and gp8psk, but I
> > >am
> > >holding them back 'till I got time to recheck those and getting some
> > >feedback
> > >on vp702x.
> > >
> > >Please review and test.
> > >
> > >Regards,
> > >Flo
> > >
> > >Florian Mickler (6):
> > >  [media] a800: get rid of on-stack dma buffers
> > >  [media v2] vp7045: get rid of on-stack dma buffers
> > >  [media] friio: get rid of on-stack dma buffers
> > >  [media] dw2102: get rid of on-stack dma buffer
> > >  [media] m920x: get rid of on-stack dma buffers
> > >  [media] opera1: get rid of on-stack dma buffer
> > >
> > > drivers/media/dvb/dvb-usb/a800.c   |   17 ++++++++++---
> > > drivers/media/dvb/dvb-usb/dw2102.c |   10 ++++++-
> > > drivers/media/dvb/dvb-usb/friio.c  |   23 ++++++++++++++---
> > > drivers/media/dvb/dvb-usb/m920x.c  |   33 ++++++++++++++++--------
> > > drivers/media/dvb/dvb-usb/opera1.c |   31 +++++++++++++++--------
> > >drivers/media/dvb/dvb-usb/vp7045.c |   47
> > >++++++++++++++++++++++++++----------
> > > 6 files changed, 116 insertions(+), 45 deletions(-)
> > >
> > >-- 
> > >1.7.4.1
> > >
> > >--
> > >To unsubscribe from this list: send the line "unsubscribe linux-media"
> > >in
> > >the body of a message to majordomo@vger.kernel.org
> > >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> > Florian,
> > 
> > For all of these, what happens when the USB call times out and you kfree() the buffer?  Can the USB DMA actually complete after this kfree(), possibly corrupting space that has been reallocated off the heap, since the kfree()?
> > 
> > This is the scenario for which I assume allocating off the stack is bad.  
> > 
> > Do these changes simply make corruption less noticable since heap gets corrupted vs stack?
> > 
> > Regards,
> > Andy
> 
> To be blunt, I'm not shure I fully understand the requirements myself.
> But as far as I grasped it, the main problem is that we need memory
> which the processor can see as soon as the device has scribbled upon
> it. (think caches and the like)
> 
> Somewhere down the line, the buffer to usb_control_msg get's to be
> a parameter to dma_map_single which is described as part of
> the DMA API in Documentation/DMA-API.txt 
> 
> The main point I filter out from that is that the memory has to begin
> exactly at a cache line boundary... 

The API will round up so that the correct region covers the API.
However, if you have other structures packed into the space (as very
often happens on stack), you get cache line interference in the CPU if
they get accessed:  The act of accessing an adjacent object pulls in
cache above your object and destroys DMA coherence.  This is the
principle reason why DMA to stack is a bad idea.

> I guess (not verified), that the dma api takes sufficient precautions
> to abort the dma transfer if a timeout happens.  So freeing _should_
> not be an issue. (At least, I would expect big fat warnings everywhere
> if that were the case)

No, it doesn't take any precautions like this.  the DMA API is just
mapping (possibly via an IOMMU).  If the transfer times out, that's done
in the DMA engine of the card, and must be cleaned up by the driver and
unmapped.

The general rule though is never DMA to stack.  On some processors, the
way stack is allocated can actually make this not work.

James

Re: [PATCH 0/6] get rid of on-stack dma buffers

[Florian Mickler]

2011/3/22 James Bottomley <James.Bottomley@hansenpartnership.com>:
> On Mon, 2011-03-21 at 22:03 +0100, Florian Mickler wrote:
>> On Mon, 21 Mar 2011 15:26:43 -0400
>> Andy Walls <awalls@md.metrocast.net> wrote:
>>
>> > Florian Mickler <florian@mickler.org> wrote:
>>
>> To be blunt, I'm not shure I fully understand the requirements myself.
>> But as far as I grasped it, the main problem is that we need memory
>> which the processor can see as soon as the device has scribbled upon
>> it. (think caches and the like)
>>
>> Somewhere down the line, the buffer to usb_control_msg get's to be
>> a parameter to dma_map_single which is described as part of
>> the DMA API in Documentation/DMA-API.txt
>>
>> The main point I filter out from that is that the memory has to begin
>> exactly at a cache line boundary...
>
> The API will round up so that the correct region covers the API.
> However, if you have other structures packed into the space (as very
> often happens on stack), you get cache line interference in the CPU if
> they get accessed:  The act of accessing an adjacent object pulls in
> cache above your object and destroys DMA coherence.  This is the
> principle reason why DMA to stack is a bad idea.

Thanks, this was the missing piece of information to make sense of
 why it's bad for stack memory to be part of this.

>
>> I guess (not verified), that the dma api takes sufficient precautions
>> to abort the dma transfer if a timeout happens.  So freeing _should_
>> not be an issue. (At least, I would expect big fat warnings everywhere
>> if that were the case)

I did mean s/dma api/usb_control_msg/ in the above paragraph. As that is the
 ''dma api'' these drivers are using... sorry for the confusion there...

>
> No, it doesn't take any precautions like this.  the DMA API is just
> mapping (possibly via an IOMMU).  If the transfer times out, that's done
> in the DMA engine of the card, and must be cleaned up by the driver and
> unmapped.

ok.

> The general rule though is never DMA to stack.  On some processors, the
> way stack is allocated can actually make this not work.
>
> James

thanks,
Flo

p.s.: hope this message get's through to the list... I am on the road
at the moment,
 so I'm not shure that there won't be any html in it again :(

Re: [PATCH 0/6] get rid of on-stack dma buffers

[Florian Mickler]

2011/3/22 Roedel, Joerg <Joerg.Roedel@amd.com>:
> On Mon, Mar 21, 2011 at 05:03:15PM -0400, Florian Mickler wrote:
>> I guess (not verified), that the dma api takes sufficient precautions
>> to abort the dma transfer if a timeout happens.  So freeing _should_
>> not be an issue. (At least, I would expect big fat warnings everywhere
>> if that were the case)
>
> Freeing is very well an issue. All you can expect from the DMA-API is to
> give you a valid DMA handle for your device. But it can not prevent that
> a device uses this handle after you returned it. You need to make sure
> yourself that any pending DMA is canceled before calling kfree().

sorry, I meant usb_control_msg above when I said 'dma api'... as that
is the function these
drivers use to do the dma.

Regards,
Flo

Re: [PATCH 0/6] get rid of on-stack dma buffers

[David Miller]

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Tue, 22 Mar 2011 08:35:04 -0500

> The API will round up so that the correct region covers the API.
> However, if you have other structures packed into the space (as very
> often happens on stack), you get cache line interference in the CPU if
> they get accessed:  The act of accessing an adjacent object pulls in
> cache above your object and destroys DMA coherence.  This is the
> principle reason why DMA to stack is a bad idea.

Another major real reason we can't DMA on-stack stuff is because the
stack is mapped virtually on some platforms.

And that is the original reason the restriction was put in place.