* [bug report] scsi: scsi_debug: Add per_host_store option
@ 2020-05-12 10:31 Dan Carpenter
2020-05-12 18:50 ` Douglas Gilbert
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2020-05-12 10:31 UTC (permalink / raw)
To: dgilbert; +Cc: linux-scsi
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)
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] scsi: scsi_debug: Add per_host_store option
2020-05-12 10:31 [bug report] scsi: scsi_debug: Add per_host_store option Dan Carpenter
@ 2020-05-12 18:50 ` Douglas Gilbert
0 siblings, 0 replies; 2+ messages in thread
From: Douglas Gilbert @ 2020-05-12 18:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-scsi
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-05-12 18:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-12 10:31 [bug report] scsi: scsi_debug: Add per_host_store option Dan Carpenter
2020-05-12 18:50 ` Douglas Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.