Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Suwan Kim <suwan.kim027@gmail.com>
To: shuah <shuah@kernel.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>,
	kbuild@lists.01.org, kbuild-all@lists.01.org,
	linux-kernel@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org
Subject: Re: drivers/usb/usbip/stub_rx.c:505 stub_recv_cmd_submit() error: uninitialized symbol 'nents'.
Date: Mon, 11 Nov 2019 11:45:32 +0900
Message-ID: <20191111024532.GA3424@localhost.localdomain> (raw)
In-Reply-To: <82478914-2bed-d8d8-0ee2-0460081434db@kernel.org>

On Mon, Nov 04, 2019 at 10:07:28AM -0700, shuah wrote:
> 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?

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)

> Can you add a comment above stub_priv_alloc() indicating that it adds
> SDEV_EVENT_ERROR_MALLOC?

Yes, I will.

Regards,
Suwan Kim

> > +       }
> > +
> >          /* 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

      reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20191022092839.GD10833@kadam>
2019-10-23  7:11 ` Suwan Kim
2019-10-24 19:45   ` Dan Carpenter
2019-10-24 22:52     ` shuah
2019-10-26  2:41       ` Suwan Kim
2019-10-30  8:19         ` Dan Carpenter
2019-10-26  3:40       ` Suwan Kim
2019-10-29 11:07         ` shuah
2019-11-01 14:34           ` Suwan Kim
2019-11-04 17:07             ` shuah
2019-11-11  2:45               ` Suwan Kim [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191111024532.GA3424@localhost.localdomain \
    --to=suwan.kim027@gmail.com \
    --cc=dan.carpenter@oracle.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kbuild-all@lists.01.org \
    --cc=kbuild@lists.01.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=shuah@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git