Re: [bug report] scsi: scsi_debug: Add per_host_store option

[Douglas Gilbert <dgilbert@interlog.com>]

On 2020-05-12 6:31 a.m., Dan Carpenter wrote:
> Hello Douglas Gilbert,
> 
> The patch 87c715dcde63: "scsi: scsi_debug: Add per_host_store option"
> from Apr 21, 2020, leads to the following static checker warning:
> 
> drivers/scsi/scsi_debug.c:3748 resp_write_same() warn: inconsistent returns '*macc_lckp'.
>    Locked on  : 3728
>    Unlocked on: 3708,3748
> drivers/scsi/scsi_debug.c:3712 resp_write_same() error: we previously assumed 'sip' could be null (see line 3699)
> drivers/scsi/scsi_debug.c:3902 resp_comp_write() error: we previously assumed 'sip' could be null (see line 3859)
> drivers/scsi/scsi_debug.c:3965 resp_unmap() error: we previously assumed 'sip' could be null (see line 3926)
> drivers/scsi/scsi_debug.c:4261 resp_verify() error: we previously assumed 'sip' could be null (see line 4208)

Dan,
It is probably a bit much to expect a static analyzer to follow a table
driven parser. Before any resp_*() functions are invoked there is this
code in scsi_debug_queuecommand() . It starts with pfp=NULL :

         if (sdebug_fake_rw && (F_FAKE_RW & flags))
                 goto fini;
         if (unlikely(sdebug_every_nth)) {
                 if (fake_timeout(scp))
                         return 0;       /* ignore command: make trouble */
         }
         if (likely(oip->pfp))
                 pfp = oip->pfp; /* calls a resp_* function */
         else
                 pfp = r_pfp;    /* if leaf function ptr NULL, try the root's */
fini:

So iff those tables are properly set up then any media-touching (i.e.
store touching) SCSI command will have the F_FAKE_RW flag set and pfp
will reach the 'fini:' label still set to NULL. In that case the
corresponding resp_*() function will _not_ be called and this code's
static analysis becomes moot.

That said, a quick audit of those tables finds that some recently added
commands break that invariant so a new patch is coming. The static
analyzer may still complain, so can it be told to STFU ?

The code in that area is going to get the tripwire shown below.
check_patch.pl warns against BUG_ON() but there seems to be no simple
way to enforce these relationships.

/*
  * Note: if BUG_ON() fires it usually indicates a problem with the parser
  * tables. Perhaps a missing F_FAKE_RW or FF_MEDIA_IO flag. Response functions
  * that access any of the "stores" in struct sdeb_store_info should call this
  * function with bug_if_fake_rw set to true.
  */
static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip,
                                                 bool bug_if_fake_rw)
{
         if (sdebug_fake_rw) {
                 BUG_ON(bug_if_fake_rw); /* See note above */
                 return NULL;
         }
         return xa_load(per_store_ap, devip->sdbg_host->si_idx);
}


Doug Gilbert


> drivers/scsi/scsi_debug.c
>    3688  static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
>    3689                             u32 ei_lba, bool unmap, bool ndob)
>    3690  {
>    3691          struct scsi_device *sdp = scp->device;
>    3692          struct sdebug_dev_info *devip = (struct sdebug_dev_info *)sdp->hostdata;
>    3693          unsigned long long i;
>    3694          u64 block, lbaa;
>    3695          u32 lb_size = sdebug_sector_size;
>    3696          int ret;
>    3697          struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
>    3698                                                  scp->device->hostdata);
>    3699          rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
>                            ^^^^^^^^^
> If "sip" is non-NULL we use that lock.
> 
>    3700          u8 *fs1p;
>    3701          u8 *fsp;
>    3702
>    3703          write_lock(macc_lckp);
>    3704
>    3705          ret = check_device_access_params(scp, lba, num, true);
>    3706          if (ret) {
>    3707                  write_unlock(macc_lckp);
>    3708                  return ret;
>    3709          }
>    3710
>    3711          if (unmap && scsi_debug_lbp()) {
>    3712                  unmap_region(sip, lba, num);
> 
> How do we know "sip" is non-NULL?
> 
>    3713                  goto out;
>    3714          }
>    3715          lbaa = lba;
>    3716          block = do_div(lbaa, sdebug_store_sectors);
>    3717          /* if ndob then zero 1 logical block, else fetch 1 logical block */
>    3718          fsp = sip->storep;
>                        ^^^^^^^^^^^
> Same.
> 
>    3719          fs1p = fsp + (block * lb_size);
>    3720          if (ndob) {
>    3721                  memset(fs1p, 0, lb_size);
>    3722                  ret = 0;
>    3723          } else
>    3724                  ret = fetch_to_dev_buffer(scp, fs1p, lb_size);
>    3725
>    3726          if (-1 == ret) {
>    3727                  write_unlock(&sip->macc_lck);
> 
> If we know that "sip" is non-NULL then this is fine, but it is probably
> less confusing to use write_unlock(macc_lckp); consistently everywhere.
> 
>    3728                  return DID_ERROR << 16;
>    3729          } else if (sdebug_verbose && !ndob && (ret < lb_size))
>    3730                  sdev_printk(KERN_INFO, scp->device,
>    3731                              "%s: %s: lb size=%u, IO sent=%d bytes\n",
>    3732                              my_name, "write same", lb_size, ret);
> 
> regards,
> dan carpenter
>