Re: [bug report] scsi: aacraid: Added support for hotplug

[Dan Carpenter <dan.carpenter@oracle.com>]

On Mon, Feb 13, 2017 at 07:39:15PM +0000, Raghava Aditya Renukunta wrote:
> Hi Don,
> 
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Monday, February 13, 2017 10:47 AM
> > To: Raghava Aditya Renukunta
> > <RaghavaAditya.Renukunta@microsemi.com>
> > Cc: linux-scsi@vger.kernel.org
> > Subject: [bug report] scsi: aacraid: Added support for hotplug
> > 
> > EXTERNAL EMAIL
> > 
> > 
> > Hello Raghava Aditya Renukunta,
> > 
> > The patch 6223a39fe6fb: "scsi: aacraid: Added support for hotplug"
> > from Feb 2, 2017, leads to the following static checker warning:
> > 
> >         drivers/scsi/aacraid/commsup.c:2243 aac_process_events()
> >         error: double unlock 'spin_lock:t_lock'
> > 
> > drivers/scsi/aacraid/commsup.c
> >   2130          spin_lock_irqsave(t_lock, flags);
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   2131
> >   2132          while (!list_empty(&(dev->queues-
> > >queue[HostNormCmdQueue].cmdq))) {
> >   2133                  struct list_head *entry;
> >   2134                  struct aac_aifcmd *aifcmd;
> >   2135                  unsigned int  num;
> >   2136                  struct hw_fib **hw_fib_pool, **hw_fib_p;
> >   2137                  struct fib **fib_pool, **fib_p;
> >   2138
> >   2139                  set_current_state(TASK_RUNNING);
> >   2140
> >   2141                  entry = dev->queues-
> > >queue[HostNormCmdQueue].cmdq.next;
> >   2142                  list_del(entry);
> >   2143
> >   2144                  t_lock = dev->queues->queue[HostNormCmdQueue].lock;
> >   2145                  spin_unlock_irqrestore(t_lock, flags);
> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >   2146
> >   2147                  fib = list_entry(entry, struct fib, fiblink);
> >   2148                  hw_fib = fib->hw_fib_va;
> >   2149                  if (dev->sa_firmware) {
> >   2150                          /* Thor AIF */
> >   2151                          aac_handle_sa_aif(dev, fib);
> >   2152                          aac_fib_adapter_complete(fib, (u16)sizeof(u32));
> >   2153                          continue;
> > 
> > The locking isn't right here.  We should re-take the spinlock before
> > continuing.
> 
> The intention here is to protect the retrieval of entry from the queues.
> Or do you mean that we should just protect the whole while loop with one spin_lock (t_lock)?
> 

This is a static checker warning that says we call
spin_unlock_irqrestore(t_lock, flags); at the end of the loop but
sometimes we're not holding the lock.

This is a Smatch warning and it doesn't handle loops correctly.  It
should also warn that on line 2145 we might not be holding the lock
either but it misses that bug.

There is no way this continue is correct with regards to locking.

regards,
dan carpenter

> Thank you,
> Raghava Aditya
> 
> >   2154                  }
> >   2155                  /*
> >   2156                   *      We will process the FIB here or pass it to a
> >   2157                   *      worker thread that is TBD. We Really can't
> >   2158                   *      do anything at this point since we don't have
> >   2159                   *      anything defined for this thread to do.
> >   2160                   */
> > 
> >         [ snip ]
> > 
> >   2221  free_mem:
> >   2222                  /* Free up the remaining resources */
> >   2223                  hw_fib_p = hw_fib_pool;
> >   2224                  fib_p = fib_pool;
> >   2225                  while (hw_fib_p < &hw_fib_pool[num]) {
> >   2226                          kfree(*hw_fib_p);
> >   2227                          kfree(*fib_p);
> >   2228                          ++fib_p;
> >   2229                          ++hw_fib_p;
> >   2230                  }
> >   2231                  kfree(fib_pool);
> >   2232  free_hw_fib_pool:
> >   2233                  kfree(hw_fib_pool);
> >   2234  free_fib:
> >   2235                  kfree(fib);
> >   2236                  t_lock = dev->queues->queue[HostNormCmdQueue].lock;
> >   2237                  spin_lock_irqsave(t_lock, flags);
> >   2238          }
> >   2239          /*
> >   2240           *      There are no more AIF's
> >   2241           */
> >   2242          t_lock = dev->queues->queue[HostNormCmdQueue].lock;
> >   2243          spin_unlock_irqrestore(t_lock, flags);
> > 
> > Otherwise it is a double unlock bug.
> > 
> >   2244  }
> > 
> > 
> > regards,
> > dan carpenter