Re: [media] gspca: Fix locking issues related to suspend/resume

[Hans de Goede <hdegoede@redhat.com>]

Hi,

Thanks for the report, it turns out that the checking for gspca_dev->dev,
rather then gspca_dev->present (which is a mod I made on top of
Hans Verkuil's orginal patch), turns out to be a bad idea in general
as its racy, this race has been fixed by this commit:
http://git.linuxtv.org/hgoede/gspca.git/commitdiff/a454f0811950742475b76dbf5ac10876e48ffaaa

Which should also appease the static checker warnings.

Thanks & Regards,

Hans


On 07/19/2012 02:13 PM, Dan Carpenter wrote:
> Hello Hans Verkuil,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 254902b01d2a: "[media] gspca: Fix locking issues related to
> suspend/resume" from May 6, 2012, leads to the following Smatch
> complaint:
>
> drivers/media/video/gspca/sq905c.c:176 sq905c_dostream()
> 	 warn: variable dereferenced before check 'gspca_dev->dev' (see line 180)
>
> drivers/media/video/gspca/sq905c.c
>     158                  /* Request the header, which tells the size to download */
>     159                  ret = usb_bulk_msg(gspca_dev->dev,
>     160                                  usb_rcvbulkpipe(gspca_dev->dev, 0x81),
>                                                          ^^^^^^^^^^^^^^
> Derereference inside the calls to usb_bulk_msg() and usb_rcvbulkpipe().
>
>     161                                  buffer, FRAME_HEADER_LEN, &act_len,
>     162                                  SQ905C_DATA_TIMEOUT);
>     163                  PDEBUG(D_STREAM,
>     164                          "Got %d bytes out of %d for header",
>     165                          act_len, FRAME_HEADER_LEN);
>     166                  if (ret < 0 || act_len < FRAME_HEADER_LEN)
>     167                          goto quit_stream;
>     168                  /* size is read from 4 bytes starting 0x40, little endian */
>     169                  bytes_left = buffer[0x40]|(buffer[0x41]<<8)|(buffer[0x42]<<16)
>     170                                          |(buffer[0x43]<<24);
>     171                  PDEBUG(D_STREAM, "bytes_left = 0x%x", bytes_left);
>     172                  /* We keep the header. It has other information, too. */
>     173                  packet_type = FIRST_PACKET;
>     174                  gspca_frame_add(gspca_dev, packet_type,
>     175                                  buffer, FRAME_HEADER_LEN);
>     176			while (bytes_left > 0 && gspca_dev->dev) {
>                                                   ^^^^^^^^^^^^^^
> New check.
>
>     177				data_len = bytes_left > SQ905C_MAX_TRANSFER ?
>     178					SQ905C_MAX_TRANSFER : bytes_left;
>     179				ret = usb_bulk_msg(gspca_dev->dev,
>     180					usb_rcvbulkpipe(gspca_dev->dev, 0x81),
>                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Some more dereferences here.
>
>     181					buffer, data_len, &act_len,
>     182					SQ905C_DATA_TIMEOUT);
>     183                          if (ret < 0 || act_len < data_len)
>     184                                  goto quit_stream;
>     185                          PDEBUG(D_STREAM,
>     186                                  "Got %d bytes out of %d for frame",
>     187                                  data_len, bytes_left);
>     188                          bytes_left -= data_len;
>     189                          if (bytes_left == 0)
>     190                                  packet_type = LAST_PACKET;
>     191                          else
>     192                                  packet_type = INTER_PACKET;
>     193                          gspca_frame_add(gspca_dev, packet_type,
>     194                                          buffer, data_len);
>     195                  }
>
> regards,
> dan carpenter
>
> --
> 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
>