drivers/usb/usbip/stub_rx.c:505 stub_recv_cmd_submit() error: uninitialized symbol 'nents'.
Subsystems (2)
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks ago
If you fix the issue, kindly add following tag
New smatch warnings:drivers/usb/usbip/stub_rx.c:505 stub_recv_cmd_submit() error: uninitialized symbol 'nents'.
Old smatch warnings:drivers/usb/usbip/stub_rx.c:450 stub_recv_xbuff() error: uninitialized symbol 'ret'.
# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea44d190764b4422af4d1c29eaeb9e69e353b406
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkout ea44d190764b4422af4d1c29eaeb9e69e353b406
vim +/nents +505 drivers/usb/usbip/stub_rx.c
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 453 static void stub_recv_cmd_submit(struct stub_device *sdev,
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 454 struct usbip_header *pdu)
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 455 {
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 456 struct stub_priv *priv;
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 457 struct usbip_device *ud = &sdev->ud;
2d8f4595d1f275 drivers/staging/usbip/stub_rx.c Max Vozeler 2011-01-12 458 struct usb_device *udev = sdev->udev;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 459 struct scatterlist *sgl = NULL, *sg;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 460 void *buffer = NULL;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 461 unsigned long long buf_len;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 462 int nents;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 463 int num_urbs = 1;
c6688ef9f29762 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 464 int pipe = get_pipe(sdev, pdu);
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 465 int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 466 int support_sg = 1;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 467 int np = 0;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 468 int ret, i;
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 469
635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 470 if (pipe == -1)
635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 471 return;
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 472
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 473 priv = stub_priv_alloc(sdev, pdu);
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 474 if (!priv)
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 475 return;
4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 476
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 477 buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 478
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 479 /* allocate urb transfer buffer, if needed */
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 480 if (buf_len) {
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 481 if (use_sg) {
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 482 sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 483 if (!sgl)
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 484 goto err_malloc;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 485 } else {
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 486 buffer = kzalloc(buf_len, GFP_KERNEL);
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 487 if (!buffer)
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 488 goto err_malloc;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 489 }
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 490 }
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 491
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 492 /* Check if the server's HCD supports SG */
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 493 if (use_sg && !udev->bus->sg_tablesize) {Smatch thinks "use_sg" can be true when "buf_len" is zero. It's hard to tell if Smatch is right or wrong without more context...
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 494 /*
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 495 * If the server's HCD doesn't support SG, break a single SG
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 496 * request into several URBs and map each SG list entry to
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 497 * corresponding URB buffer. The previously allocated SG
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 498 * list is stored in priv->sgl (If the server's HCD support SG,
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 499 * SG list is stored only in urb->sg) and it is used as an
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 500 * indicator that the server split single SG request into
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 501 * several URBs. Later, priv->sgl is used by stub_complete() and
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 502 * stub_send_ret_submit() to reassemble the divied URBs.
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 503 */
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 504 support_sg = 0;
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 @505 num_urbs = nents;
^^^^^^^^^^^^^^^^ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 506 priv->completed_urbs = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 507 pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 508 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Quoted 63 lines · Dan Carpenter
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks agoIf you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>New smatch warnings:
drivers/usb/usbip/stub_rx.c:505stub_recv_cmd_submit()error: uninitialized symbol 'nents'.Old smatch warnings:
drivers/usb/usbip/stub_rx.c:450stub_recv_xbuff()error: uninitialized symbol 'ret'.# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea44d190764b4422af4d1c29eaeb9e69e353b406
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkoutea44d190764b4422af4d1c29eaeb9e69e353b406
vim +/nents +505drivers/usb/usbip/stub_rx.c4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 453 static void stub_recv_cmd_submit(struct stub_device *sdev, 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 454 struct usbip_header *pdu) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 455 { 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 456 struct stub_priv *priv; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 457 struct usbip_device *ud = &sdev->ud; 2d8f4595d1f275 drivers/staging/usbip/stub_rx.c Max Vozeler 2011-01-12 458 struct usb_device *udev = sdev->udev; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 459 struct scatterlist *sgl = NULL, *sg; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 460 void *buffer = NULL; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 461 unsigned long long buf_len; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 462 int nents; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 463 int num_urbs = 1; c6688ef9f29762 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 464 int pipe = get_pipe(sdev, pdu); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 465 int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 466 int support_sg = 1; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 467 int np = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 468 int ret, i; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 469 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 470 if (pipe == -1) 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 471 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 472 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 473 priv = stub_priv_alloc(sdev, pdu); 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 474 if (!priv) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 475 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 476 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 477 buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 478 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 479 /* allocate urb transfer buffer, if needed */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 480 if (buf_len) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 481 if (use_sg) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 482 sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 483 if (!sgl) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 484 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 485 } else { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 486 buffer = kzalloc(buf_len, GFP_KERNEL); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 487 if (!buffer) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 488 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 489 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 490 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 491 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 492 /* Check if the server's HCD supports SG */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 493 if (use_sg && !udev->bus->sg_tablesize) {
Quoted 2 lines
Smatch thinks "
use_sg" can be true when "buf_len" is zero. It's hard to tell if Smatch is right or wrong without more context...
This is a bit strange. The meaning of "use_sg" is that client will use scatter-gather and client's urb->num_sgs is not zero. And buffer length should not be zero.
usb_sg and buf_len are both client-dependent variables, so I think if they have wrong value in the server side, the client must have sent use_sg and buf_len with incorrect values.
Did this error occur when compiling? If then, Did Smatch also consider vhci tx side?
Regards
Suwan Kim
Quoted 21 lines
ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 494 /* ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 495 * If the server's HCD doesn't support SG, break a single SG ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 496 * request into several URBs and map each SG list entry to ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 497 * corresponding URB buffer. The previously allocated SG ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 498 * list is stored in priv->sgl (If the server's HCD support SG, ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 499 * SG list is stored only in urb->sg) and it is used as an ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 500 * indicator that the server split single SG request into ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 501 * several URBs. Later, priv->sgl is used by stub_complete() and ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 502 * stub_send_ret_submit() to reassemble the divied URBs. ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 503 */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 504 support_sg = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 @505 num_urbs = nents; ^^^^^^^^^^^^^^^^ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 506 priv->completed_urbs = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 507 pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 508 }---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Quoted 71 lines · Suwan Kim
Quoted 63 lines · Dan Carpenter
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks agoIf you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>New smatch warnings:
drivers/usb/usbip/stub_rx.c:505stub_recv_cmd_submit()error: uninitialized symbol 'nents'.Old smatch warnings:
drivers/usb/usbip/stub_rx.c:450stub_recv_xbuff()error: uninitialized symbol 'ret'.# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea44d190764b4422af4d1c29eaeb9e69e353b406
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkoutea44d190764b4422af4d1c29eaeb9e69e353b406
vim +/nents +505drivers/usb/usbip/stub_rx.c4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 453 static void stub_recv_cmd_submit(struct stub_device *sdev, 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 454 struct usbip_header *pdu) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 455 { 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 456 struct stub_priv *priv; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 457 struct usbip_device *ud = &sdev->ud; 2d8f4595d1f275 drivers/staging/usbip/stub_rx.c Max Vozeler 2011-01-12 458 struct usb_device *udev = sdev->udev; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 459 struct scatterlist *sgl = NULL, *sg; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 460 void *buffer = NULL; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 461 unsigned long long buf_len; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 462 int nents; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 463 int num_urbs = 1; c6688ef9f29762 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 464 int pipe = get_pipe(sdev, pdu); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 465 int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 466 int support_sg = 1; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 467 int np = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 468 int ret, i; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 469 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 470 if (pipe == -1) 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 471 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 472 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 473 priv = stub_priv_alloc(sdev, pdu); 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 474 if (!priv) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 475 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 476 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 477 buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 478 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 479 /* allocate urb transfer buffer, if needed */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 480 if (buf_len) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 481 if (use_sg) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 482 sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 483 if (!sgl) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 484 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 485 } else { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 486 buffer = kzalloc(buf_len, GFP_KERNEL); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 487 if (!buffer) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 488 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 489 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 490 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 491 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 492 /* Check if the server's HCD supports SG */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 493 if (use_sg && !udev->bus->sg_tablesize) {Quoted 2 lines
Smatch thinks "
use_sg" can be true when "buf_len" is zero. It's hard to tell if Smatch is right or wrong without more context...This is a bit strange. The meaning of "
use_sg" is that client will use scatter-gather and client'surb->num_sgsis not zero. And buffer length should not be zero.
Quoted 5 lines
usb_sgandbuf_lenare both client-dependent variables, so I think if they have wrong value in the server side, the client must have sentuse_sgandbuf_lenwith incorrect values.Did this error occur when compiling?
Smatch is doing static analysis, yes.
Quoted 1 line
If then, Did Smatch also consider vhci tx side?
I'm not really sure... I can't reproduce the warning because on my system Smatch doesn't parse usbip_recv() correctly so it ends up silencing that warning. :/
regards,
dan carpenter
shuah
maintainerQuoted 78 lines · Dan Carpenter
Quoted 71 lines · Suwan Kim
Quoted 63 lines · Dan Carpenter
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks agoIf you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>New smatch warnings:
drivers/usb/usbip/stub_rx.c:505stub_recv_cmd_submit()error: uninitialized symbol 'nents'.Old smatch warnings:
drivers/usb/usbip/stub_rx.c:450stub_recv_xbuff()error: uninitialized symbol 'ret'.# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea44d190764b4422af4d1c29eaeb9e69e353b406
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkoutea44d190764b4422af4d1c29eaeb9e69e353b406
vim +/nents +505drivers/usb/usbip/stub_rx.c4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 453 static void stub_recv_cmd_submit(struct stub_device *sdev, 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 454 struct usbip_header *pdu) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 455 { 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 456 struct stub_priv *priv; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 457 struct usbip_device *ud = &sdev->ud; 2d8f4595d1f275 drivers/staging/usbip/stub_rx.c Max Vozeler 2011-01-12 458 struct usb_device *udev = sdev->udev; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 459 struct scatterlist *sgl = NULL, *sg; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 460 void *buffer = NULL; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 461 unsigned long long buf_len; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 462 int nents; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 463 int num_urbs = 1; c6688ef9f29762 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 464 int pipe = get_pipe(sdev, pdu); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 465 int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 466 int support_sg = 1; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 467 int np = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 468 int ret, i; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 469 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 470 if (pipe == -1) 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 471 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 472 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 473 priv = stub_priv_alloc(sdev, pdu); 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 474 if (!priv) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 475 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 476 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 477 buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 478 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 479 /* allocate urb transfer buffer, if needed */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 480 if (buf_len) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 481 if (use_sg) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 482 sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 483 if (!sgl) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 484 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 485 } else { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 486 buffer = kzalloc(buf_len, GFP_KERNEL); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 487 if (!buffer) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 488 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 489 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 490 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 491 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 492 /* Check if the server's HCD supports SG */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 493 if (use_sg && !udev->bus->sg_tablesize) {Quoted 2 lines
Smatch thinks "
use_sg" can be true when "buf_len" is zero. It's hard to tell if Smatch is right or wrong without more context...This is a bit strange. The meaning of "
use_sg" is that client will use scatter-gather and client'surb->num_sgsis not zero. And buffer length should not be zero.Quoted 5 lines
usb_sgandbuf_lenare both client-dependent variables, so I think if they have wrong value in the server side, the client must have sentuse_sgandbuf_lenwith incorrect values.Did this error occur when compiling?
Quoted 8 lines
Smatch is doing static analysis, yes.
Quoted 1 line
If then, Did Smatch also consider vhci tx side?
I'm not really sure... I can't reproduce the warning because on my system Smatch doesn't parse
usbip_recv()correctly so it ends up silencing that warning. :/
Hi Suwan,
This is a problem that needs fixing. nents
/* allocate urb transfer buffer, if needed */
if (buf_len) {
if (use_sg) {
sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);nents gets initialized here by sgl_alloc()
if (!sgl)
goto err_malloc;
} else {
buffer = kzalloc(buf_len, GFP_KERNEL);
if (!buffer)
goto err_malloc;
}
}
/* 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 bystub_complete() and
* stub_send_ret_submit() to reassemble the divied URBs.
*/
support_sg = 0;
num_urbs = nents;I think nents will be valid here. Is there need for this additional check here? You can fold this into the previous use_sg check, right after the sg_alloc() success, I would think.
priv->completed_urbs = 0;
pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG;
}thanks,
-- Shuah
Quoted 12 lines · shuah
Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Suwan Kim
Quoted 9 lines · Dan Carpenter
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks agoIf you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
New smatch warnings:
drivers/usb/usbip/stub_rx.c:505stub_recv_cmd_submit()error: uninitialized symbol 'nents'.Old smatch warnings:
drivers/usb/usbip/stub_rx.c:450stub_recv_xbuff()error: uninitialized symbol 'ret'.
Here, ret is not initialized, meaning priv->num_urbs is 0. priv->urbs must be greater than zero. priv->num_urbs = 0 means nents is 0 (line 505)
Dan, What is the relationship between old and new warnings? priv->num_urbs is set as value of "num_urbs" at stub_recv_cmd_submit() and "num_urbs" is initialized as 1 first. "num_urbs" will be reset only at the place where smatch new warnings happened (line 505).
So, In my opinion, old smatch warnings should occur after the new smatch warnings. Does this look right to you?
Quoted 111 lines
Quoted 61 lines
Quoted 55 lines
Quoted 48 lines
# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea44d190764b4422af4d1c29eaeb9e69e353b406
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkoutea44d190764b4422af4d1c29eaeb9e69e353b406
vim +/nents +505drivers/usb/usbip/stub_rx.c4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 453 static void stub_recv_cmd_submit(struct stub_device *sdev, 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 454 struct usbip_header *pdu) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 455 { 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 456 struct stub_priv *priv; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 457 struct usbip_device *ud = &sdev->ud; 2d8f4595d1f275 drivers/staging/usbip/stub_rx.c Max Vozeler 2011-01-12 458 struct usb_device *udev = sdev->udev; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 459 struct scatterlist *sgl = NULL, *sg; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 460 void *buffer = NULL; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 461 unsigned long long buf_len; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 462 int nents; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 463 int num_urbs = 1; c6688ef9f29762 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 464 int pipe = get_pipe(sdev, pdu); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 465 int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 466 int support_sg = 1; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 467 int np = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 468 int ret, i; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 469 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 470 if (pipe == -1) 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 471 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 472 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 473 priv = stub_priv_alloc(sdev, pdu); 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 474 if (!priv) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 475 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 476 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 477 buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 478 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 479 /* allocate urb transfer buffer, if needed */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 480 if (buf_len) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 481 if (use_sg) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 482 sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 483 if (!sgl) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 484 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 485 } else { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 486 buffer = kzalloc(buf_len, GFP_KERNEL); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 487 if (!buffer) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 488 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 489 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 490 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 491 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 492 /* Check if the server's HCD supports SG */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 493 if (use_sg && !udev->bus->sg_tablesize) {Quoted 2 lines
Smatch thinks "
use_sg" can be true when "buf_len" is zero. It's hard to tell if Smatch is right or wrong without more context...This is a bit strange. The meaning of "
use_sg" is that client will use scatter-gather and client'surb->num_sgsis not zero. And buffer length should not be zero.Quoted 5 lines
usb_sgandbuf_lenare both client-dependent variables, so I think if they have wrong value in the server side, the client must have sentuse_sgandbuf_lenwith incorrect values.Did this error occur when compiling?
Quoted 8 lines
Smatch is doing static analysis, yes.
Quoted 1 line
If then, Did Smatch also consider vhci tx side?
I'm not really sure... I can't reproduce the warning because on my system Smatch doesn't parse
usbip_recv()correctly so it ends up silencing that warning. :/Hi Suwan,
This is a problem that needs fixing. nents
/* allocate urb transfer buffer, if needed */ if (buf_len) { if (use_sg) { sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);nents gets initialized here by
sgl_alloc()if (!sgl) goto err_malloc; } else { buffer = kzalloc(buf_len, GFP_KERNEL); if (!buffer) goto err_malloc; } } /* 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 supportSG,
* SG list is stored only inurb->sg) and it is used as an
* indicator that the server split single SG request into
* several URBs. Later,priv->sglis used bystub_complete()
and* stub_send_ret_submit() to reassemble the divied URBs. */ support_sg = 0; num_urbs = nents;I think nents will be valid here. Is there need for this additional check here? You can fold this into the previous
use_sgcheck, right after thesg_alloc()success, I would think.
Quoted 7 lines
priv->completed_urbs = 0; pdu->u.cmd_submit.transfer_flags &= ~URB_DMA_MAP_SG; }thanks,
-- Shuah
I agree with you. Is it your intention to check as follows?
/* Check if the server's HCD supports SG */
if (use_sg && !nents && !udev->bus->sg_tablesize) {
^ Additinal check in here?In my opinion, it would be nice to initialize nents to zero first, and then check in the above if statement to see if nents was set in sgl_free().
Regards,
Suwan Kim
Quoted 23 lines · Suwan Kim
Quoted 12 lines · shuah
Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Suwan Kim
Quoted 9 lines · Dan Carpenter
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks agoIf you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
New smatch warnings:
drivers/usb/usbip/stub_rx.c:505stub_recv_cmd_submit()error: uninitialized symbol 'nents'.Old smatch warnings:
drivers/usb/usbip/stub_rx.c:450stub_recv_xbuff()error: uninitialized symbol 'ret'.Here, ret is not initialized, meaning
priv->num_urbsis 0.priv->urbsmust be greater than zero.priv->num_urbs= 0 means nents is 0 (line 505)
Quoted 7 lines
Dan, What is the relationship between old and new warnings?
priv->num_urbsis set as value of "num_urbs" atstub_recv_cmd_submit()and "num_urbs" is initialized as 1 first. "num_urbs" will be reset only at the place where smatch new warnings happened (line 505).So, In my opinion, old smatch warnings should occur after the new smatch warnings. Does this look right to you?
I don't know exactly how the 0day bot runs Smatch. If you have cross function analysis enabled then silencing the uninitialized "nents" warning will also silence the uninitialized "ret" warning.
regards,
dan carpenter
Quoted 120 lines · shuah
Quoted 78 lines · Dan Carpenter
Quoted 71 lines · Suwan Kim
Quoted 63 lines · Dan Carpenter
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks agoIf you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>New smatch warnings:
drivers/usb/usbip/stub_rx.c:505stub_recv_cmd_submit()error: uninitialized symbol 'nents'.Old smatch warnings:
drivers/usb/usbip/stub_rx.c:450stub_recv_xbuff()error: uninitialized symbol 'ret'.# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea44d190764b4422af4d1c29eaeb9e69e353b406
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkoutea44d190764b4422af4d1c29eaeb9e69e353b406
vim +/nents +505drivers/usb/usbip/stub_rx.c4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 453 static void stub_recv_cmd_submit(struct stub_device *sdev, 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 454 struct usbip_header *pdu) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 455 { 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 456 struct stub_priv *priv; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 457 struct usbip_device *ud = &sdev->ud; 2d8f4595d1f275 drivers/staging/usbip/stub_rx.c Max Vozeler 2011-01-12 458 struct usb_device *udev = sdev->udev; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 459 struct scatterlist *sgl = NULL, *sg; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 460 void *buffer = NULL; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 461 unsigned long long buf_len; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 462 int nents; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 463 int num_urbs = 1; c6688ef9f29762 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 464 int pipe = get_pipe(sdev, pdu); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 465 int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 466 int support_sg = 1; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 467 int np = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 468 int ret, i; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 469 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 470 if (pipe == -1) 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 471 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 472 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 473 priv = stub_priv_alloc(sdev, pdu); 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 474 if (!priv) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 475 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 476 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 477 buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 478 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 479 /* allocate urb transfer buffer, if needed */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 480 if (buf_len) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 481 if (use_sg) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 482 sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 483 if (!sgl) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 484 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 485 } else { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 486 buffer = kzalloc(buf_len, GFP_KERNEL); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 487 if (!buffer) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 488 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 489 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 490 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 491 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 492 /* Check if the server's HCD supports SG */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 493 if (use_sg && !udev->bus->sg_tablesize) {Quoted 2 lines
Smatch thinks "
use_sg" can be true when "buf_len" is zero. It's hard to tell if Smatch is right or wrong without more context...This is a bit strange. The meaning of "
use_sg" is that client will use scatter-gather and client'surb->num_sgsis not zero. And buffer length should not be zero.Quoted 5 lines
usb_sgandbuf_lenare both client-dependent variables, so I think if they have wrong value in the server side, the client must have sentuse_sgandbuf_lenwith incorrect values.Did this error occur when compiling?
Quoted 8 lines
Smatch is doing static analysis, yes.
Quoted 1 line
If then, Did Smatch also consider vhci tx side?
I'm not really sure... I can't reproduce the warning because on my system Smatch doesn't parse
usbip_recv()correctly so it ends up silencing that warning. :/Hi Suwan,
This is a problem that needs fixing. nents
/* allocate urb transfer buffer, if needed */ if (buf_len) { if (use_sg) { sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);nents gets initialized here by
sgl_alloc()if (!sgl) goto err_malloc; } else { buffer = kzalloc(buf_len, GFP_KERNEL); if (!buffer) goto err_malloc; } } /* 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 supportSG,
* SG list is stored only inurb->sg) and it is used as an
* indicator that the server split single SG request into
* several URBs. Later,priv->sglis used bystub_complete()
and
Quoted 8 lines
* stub_send_ret_submit() to reassemble the divied URBs. */ support_sg = 0; num_urbs = nents;I think nents will be valid here. Is there need for this additional check here? You can fold this into the previous
use_sgcheck, right after thesg_alloc()success, I would think.
Or Is it your intention to check as follows?
I think this looks good.
...
int nents = 0
...
/* 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;
} else {
buffer = kzalloc(buf_len, GFP_KERNEL);
if (!buffer)
goto err_malloc;
}
}
/* Additional check here */
if (use_sg && !nents) {
err_message;
goto err_urbs;
}
/* Check if the server's HCD supports SG */
if (use_sg && !udev->bus->sg_tablesize) {Regards,
Suwan Kim
shuah
maintainerQuoted 133 lines · Suwan Kim
Quoted 120 lines · shuah
Quoted 78 lines · Dan Carpenter
Quoted 71 lines · Suwan Kim
Quoted 63 lines · Dan Carpenter
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks agoIf you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>New smatch warnings:
drivers/usb/usbip/stub_rx.c:505stub_recv_cmd_submit()error: uninitialized symbol 'nents'.Old smatch warnings:
drivers/usb/usbip/stub_rx.c:450stub_recv_xbuff()error: uninitialized symbol 'ret'.# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea44d190764b4422af4d1c29eaeb9e69e353b406
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkoutea44d190764b4422af4d1c29eaeb9e69e353b406
vim +/nents +505drivers/usb/usbip/stub_rx.c4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 453 static void stub_recv_cmd_submit(struct stub_device *sdev, 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 454 struct usbip_header *pdu) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 455 { 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 456 struct stub_priv *priv; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 457 struct usbip_device *ud = &sdev->ud; 2d8f4595d1f275 drivers/staging/usbip/stub_rx.c Max Vozeler 2011-01-12 458 struct usb_device *udev = sdev->udev; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 459 struct scatterlist *sgl = NULL, *sg; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 460 void *buffer = NULL; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 461 unsigned long long buf_len; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 462 int nents; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 463 int num_urbs = 1; c6688ef9f29762 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 464 int pipe = get_pipe(sdev, pdu); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 465 int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 466 int support_sg = 1; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 467 int np = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 468 int ret, i; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 469 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 470 if (pipe == -1) 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 471 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 472 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 473 priv = stub_priv_alloc(sdev, pdu); 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 474 if (!priv) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 475 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 476 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 477 buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 478 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 479 /* allocate urb transfer buffer, if needed */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 480 if (buf_len) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 481 if (use_sg) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 482 sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 483 if (!sgl) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 484 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 485 } else { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 486 buffer = kzalloc(buf_len, GFP_KERNEL); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 487 if (!buffer) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 488 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 489 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 490 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 491 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 492 /* Check if the server's HCD supports SG */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 493 if (use_sg && !udev->bus->sg_tablesize) {Quoted 2 lines
Smatch thinks "
use_sg" can be true when "buf_len" is zero. It's hard to tell if Smatch is right or wrong without more context...This is a bit strange. The meaning of "
use_sg" is that client will use scatter-gather and client'surb->num_sgsis not zero. And buffer length should not be zero.Quoted 5 lines
usb_sgandbuf_lenare both client-dependent variables, so I think if they have wrong value in the server side, the client must have sentuse_sgandbuf_lenwith incorrect values.Did this error occur when compiling?
Quoted 8 lines
Smatch is doing static analysis, yes.
Quoted 1 line
If then, Did Smatch also consider vhci tx side?
I'm not really sure... I can't reproduce the warning because on my system Smatch doesn't parse
usbip_recv()correctly so it ends up silencing that warning. :/Hi Suwan,
This is a problem that needs fixing. nents
/* allocate urb transfer buffer, if needed */ if (buf_len) { if (use_sg) { sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);nents gets initialized here by
sgl_alloc()if (!sgl) goto err_malloc; } else { buffer = kzalloc(buf_len, GFP_KERNEL); if (!buffer) goto err_malloc; } } /* 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 supportSG,
* SG list is stored only inurb->sg) and it is used as an
* indicator that the server split single SG request into
* several URBs. Later,priv->sglis used bystub_complete()
andQuoted 8 lines
* stub_send_ret_submit() to reassemble the divied URBs. */ support_sg = 0; num_urbs = nents;I think nents will be valid here. Is there need for this additional check here? You can fold this into the previous
use_sgcheck, right after thesg_alloc()success, I would think.Or Is it your intention to check as follows?
I think this looks good.
Quoted 11 lines
...
int nents = 0
...
/* 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;Why can't we move the
Quoted 3 lines
/* Check if the server's HCD supports SG */
if (use_sg && !udev->bus->sg_tablesize) {under this check?
Quoted 7 lines
} else {
buffer = kzalloc(buf_len, GFP_KERNEL);
if (!buffer)
goto err_malloc;
}
}Quoted 9 lines
/* Additional check here */
if (use_sg && !nents) {
err_message;
goto err_urbs;
}
/* Check if the server's HCD supports SG */
if (use_sg && !udev->bus->sg_tablesize) {thanks,
-- Shuah
Quoted 146 lines · shuah
Quoted 133 lines · Suwan Kim
Quoted 120 lines · shuah
Quoted 78 lines · Dan Carpenter
Quoted 71 lines · Suwan Kim
Quoted 63 lines · Dan Carpenter
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 7d194c2100ad2a6dded545887d02754948ca5241 commit: ea44d190764b4422af4d1c29eaeb9e69e353b406 usbip: Implement SG support to vhci-hcd and stub driver date: 7 weeks agoIf you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>New smatch warnings:
drivers/usb/usbip/stub_rx.c:505stub_recv_cmd_submit()error: uninitialized symbol 'nents'.Old smatch warnings:
drivers/usb/usbip/stub_rx.c:450stub_recv_xbuff()error: uninitialized symbol 'ret'.# https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ea44d190764b4422af4d1c29eaeb9e69e353b406
git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git remote update linus
git checkoutea44d190764b4422af4d1c29eaeb9e69e353b406
vim +/nents +505drivers/usb/usbip/stub_rx.c4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 453 static void stub_recv_cmd_submit(struct stub_device *sdev, 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 454 struct usbip_header *pdu) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 455 { 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 456 struct stub_priv *priv; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 457 struct usbip_device *ud = &sdev->ud; 2d8f4595d1f275 drivers/staging/usbip/stub_rx.c Max Vozeler 2011-01-12 458 struct usb_device *udev = sdev->udev; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 459 struct scatterlist *sgl = NULL, *sg; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 460 void *buffer = NULL; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 461 unsigned long long buf_len; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 462 int nents; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 463 int num_urbs = 1; c6688ef9f29762 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 464 int pipe = get_pipe(sdev, pdu); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 465 int use_sg = pdu->u.cmd_submit.transfer_flags & URB_DMA_MAP_SG; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 466 int support_sg = 1; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 467 int np = 0; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 468 int ret, i; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 469 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 470 if (pipe == -1) 635f545a7e8be7 drivers/usb/usbip/stub_rx.c Shuah Khan 2017-12-07 471 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 472 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 473 priv = stub_priv_alloc(sdev, pdu); 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 474 if (!priv) 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 475 return; 4d7b5c7f8ad49b drivers/staging/usbip/stub_rx.c Takahiro Hirofuchi 2008-07-09 476 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 477 buf_len = (unsigned long long)pdu->u.cmd_submit.transfer_buffer_length; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 478 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 479 /* allocate urb transfer buffer, if needed */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 480 if (buf_len) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 481 if (use_sg) { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 482 sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 483 if (!sgl) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 484 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 485 } else { ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 486 buffer = kzalloc(buf_len, GFP_KERNEL); ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 487 if (!buffer) ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 488 goto err_malloc; ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 489 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 490 } ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 491 ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 492 /* Check if the server's HCD supports SG */ ea44d190764b44 drivers/usb/usbip/stub_rx.c Suwan Kim 2019-08-28 493 if (use_sg && !udev->bus->sg_tablesize) {Quoted 2 lines
Smatch thinks "
use_sg" can be true when "buf_len" is zero. It's hard to tell if Smatch is right or wrong without more context...This is a bit strange. The meaning of "
use_sg" is that client will use scatter-gather and client'surb->num_sgsis not zero. And buffer length should not be zero.Quoted 5 lines
usb_sgandbuf_lenare both client-dependent variables, so I think if they have wrong value in the server side, the client must have sentuse_sgandbuf_lenwith incorrect values.Did this error occur when compiling?
Quoted 8 lines
Smatch is doing static analysis, yes.
Quoted 1 line
If then, Did Smatch also consider vhci tx side?
I'm not really sure... I can't reproduce the warning because on my system Smatch doesn't parse
usbip_recv()correctly so it ends up silencing that warning. :/Hi Suwan,
This is a problem that needs fixing. nents
/* allocate urb transfer buffer, if needed */ if (buf_len) { if (use_sg) { sgl = sgl_alloc(buf_len, GFP_KERNEL, &nents);nents gets initialized here by
sgl_alloc()if (!sgl) goto err_malloc; } else { buffer = kzalloc(buf_len, GFP_KERNEL); if (!buffer) goto err_malloc; } } /* 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 supportSG,
* SG list is stored only inurb->sg) and it is used as an
* indicator that the server split single SG request into
* several URBs. Later,priv->sglis used bystub_complete()
andQuoted 8 lines
* stub_send_ret_submit() to reassemble the divied URBs. */ support_sg = 0; num_urbs = nents;I think nents will be valid here. Is there need for this additional check here? You can fold this into the previous
use_sgcheck, right after thesg_alloc()success, I would think.Or Is it your intention to check as follows?
I think this looks good.Quoted 11 lines
... int nents = 0 ... /* 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;
Quoted 6 lines
Why can't we move the
Quoted 3 lines
/* Check if the server's HCD supports SG */ if (use_sg && !udev->bus->sg_tablesize) {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.
I attached the code. If you are okay, I will send a patch.
Regards,
Suwan Kim
---
drivers/usb/usbip/stub_rx.c +27 -18
@@ -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; + } +/* 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 );
shuah
maintainerQuoted 5 lines · Suwan Kim
Quoted 3 lines · shuah
On 10/25/19 9:40 PM, Suwan Kim wrote:
under this check?
Quoted 8 lines
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_sgis true andbuf_lenis 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.
Quoted 3 lines
I attached the code. If you are okay, I will send a patch.
This code looks good. Couple of comments.
drivers/usb/usbip/stub_rx.c static void stub_recv_cmd_submit(struct stub_device *sdev, · line 481
Quoted 12 lines
---
drivers/usb/usbip/stub_rx.c +3
-0@@ -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?
Quoted 49 lines
drivers/usb/usbip/stub_rx.c +24
-11+ } +/* 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.
drivers/usb/usbip/stub_rx.c static void stub_recv_cmd_submit(struct stub_device *sdev, · line 521
Quoted 10 lines
drivers/usb/usbip/stub_rx.c +0
-7-*/ - 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
Quoted 30 lines · shuah
Quoted 5 lines · Suwan Kim
Quoted 3 lines · shuah
On 10/25/19 9:40 PM, Suwan Kim wrote:
under this check?
Quoted 8 lines
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_sgis true andbuf_lenis 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_lenneeds checking.Quoted 3 lines
I attached the code. If you are okay, I will send a patch.
This code looks good. Couple of comments.
Quoted 7 lines
---
drivers/usb/usbip/stub_rx.c +0
-0@@ -476,12 +476,39 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
Quoted 8 lines
drivers/usb/usbip/stub_rx.c static void stub_recv_cmd_submit(struct stub_device *sdev, · line 481
Quoted 5 lines
drivers/usb/usbip/stub_rx.c +3
-0buf_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?
Hi, Shuah, sorry about the late reply.
We don't need to release stub_priv in this error path. If the malloc error occurs and SDEV_EVENT_ERROR_MALLOC is inserted, event handler is called and releases the priv list of stub_device. And then, event handler shutdown the connection. (stub_shutdown_connection)
Quoted 2 lines
Can you add a comment above
stub_priv_alloc()indicating that it addsSDEV_EVENT_ERROR_MALLOC?
Yes, I will.
Regards,
Suwan Kim
Quoted 62 lines
Quoted 49 lines
drivers/usb/usbip/stub_rx.c +24
-11+ } +/* 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. drivers/usb/usbip/stub_rx.c static void stub_recv_cmd_submit(struct stub_device *sdev, · line 521
Quoted 10 lines
drivers/usb/usbip/stub_rx.c +0
-7-*/ - 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