On 11/1/19 8:34 AM, Suwan Kim wrote:
On Tue, Oct 29, 2019 at 05:07:58AM -0600, shuah wrote:
On 10/25/19 9:40 PM, Suwan Kim wrote:
under this check?
I understood. Moving this check after sgl_alloc() does not seem to
require any additional checks on nents.
But I think we need to check for the case that Smatch reported that
use_sg is true and buf_len is zero.
If there is no error check and an error condition occurs, the URB
will be passed to the next step without a buffer.
Yes buf_len needs checking.
I attached the code. If you are okay, I will send a patch.
This code looks good. Couple of comments.
---
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 66edfeea68fe..0b6c4736ffd6 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -476,12 +476,39 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length;
+ if (use_sg && !buf_len) {
+ dev_err(&udev->dev, "sg buffer with zero length\n");
+ goto err_malloc;
This is fine, what happens to the priv allocated by stub_priv_alloc()?
Shouldn't that be released?
Can you add a comment above stub_priv_alloc() indicating that it adds
SDEV_EVENT_ERROR_MALLOC?
+ }
+
/* allocate urb transfer buffer, if needed */
if (buf_len) {
if (use_sg) {
sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);
if (!sgl)
goto err_malloc;
+
+ /* Check if the server's HCD supports SG */
+ if (!udev->bus->sg_tablesize) {
+ /*
+ * If the server's HCD doesn't support SG, break
+ * a single SG request into several URBs and map
+ * each SG list entry to corresponding URB
+ * buffer. The previously allocated SG list is
+ * stored in priv->sgl (If the server's HCD
+ * support SG, SG list is stored only in
+ * urb->sg) and it is used as an indicator that
+ * the server split single SG request into
+ * several URBs. Later, priv->sgl is used by
+ * stub_complete() and stub_send_ret_submit() to
+ * reassemble the divied URBs.
+ */
+ support_sg = 0;
+ num_urbs = nents;
+ priv->completed_urbs = 0;
+ pdu->u.cmd_submit.transfer_flags &=
+ ~URB_DMA_MAP_SG;
+ }
} else {
buffer = kzalloc(buf_len, GFP_KERNEL);
if (!buffer)
@@ -489,24 +516,6 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
}
}
- /* Check if the server's HCD supports SG */
- if (use_sg && !udev->bus->sg_tablesize) {
- /*
- * If the server's HCD doesn't support SG, break a single SG
- * request into several URBs and map each SG list entry to
- * corresponding URB buffer. The previously allocated SG
- * list is stored in priv->sgl (If the server's HCD support SG,
- * SG list is stored only in urb->sg) and it is used as an
- * indicator that the server split single SG request into
- * several URBs. Later, priv->sgl is used by stub_complete() and
- * stub_send_ret_submit() to reassemble the divied URBs.
- */
- support_sg = 0;
- num_urbs = nents;
- priv->completed_urbs = 0;
- pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG;
- }
-
/* allocate urb array */
priv->num_urbs = num_urbs;
priv->urbs = kmalloc_array(num_urbs, sizeof(*priv->urbs), GFP_KERNEL);
thanks,
-- Shuah
