by Suwan Kim

[permalink] [raw]

Subject: Re: drivers/usb/usbip/stub_rx.c:505 stub_recv_cmd_submit() error: uninitialized symbol 'nents'.

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