Skip to content

Commit

Permalink
scsi: scsi_debug: Parser tables and code interaction
Browse files Browse the repository at this point in the history 
This patch is in response to a static analyser report from Dan Carpenter
titled: "[bug report] scsi: scsi_debug: Add per_host_store option".  This
code may not clear the static analyzer reports, but may shed light on why
they occur. Amongst other things this driver has a table driven SCSI
command parser which also involves some C code. There are some invariants
between the table entries and the corresponding C code (i.e. the resp_*()
functions) that, if broken, may lead to a NULL dereference.  And the report
is valid, at least in the case of the PRE-FETCH command.  Alas, that is not
one of the cases that the static analyzer reported.

In this particular corner case: when the fake_rw flag is set and the table
entry for a "store"-accessing command does not have the required F_FAKE_RW
flag set, do the following. Call BUG_ON() in the devip2sip() very close to
a comment block explaining why it was called and how to fix it.
checkpatch.pl complains about the BUG_ON() but there is no reasonable
remedial action that can be taken at run time.

This change allows the code reported by the static analyzer to be
simplified. Comments were also added to the table flags (e.g.  F_FAKE_RW)
so developers who add commands might be more inclined to use them
(properly).

Link: https://lore.kernel.org/r/20200513013943.25285-1-dgilbert@interlog.com
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
  • Loading branch information
Douglas Gilbert authored and Martin K. Petersen committed May 20, 2020
1 parent 840e1b5 commit b6ff8ca
Showing 1 changed file with 63 additions and 50 deletions.
113 changes: 63 additions & 50 deletions drivers/scsi/scsi_debug.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,21 +236,23 @@ static const char *sdebug_version_date = "20200421";
#define SDEBUG_CANQUEUE (SDEBUG_CANQUEUE_WORDS * BITS_PER_LONG)
#define DEF_CMD_PER_LUN 255

#define F_D_IN 1
#define F_D_OUT 2
/* UA - Unit Attention; SA - Service Action; SSU - Start Stop Unit */
#define F_D_IN 1 /* Data-in command (e.g. READ) */
#define F_D_OUT 2 /* Data-out command (e.g. WRITE) */
#define F_D_OUT_MAYBE 4 /* WRITE SAME, NDOB bit */
#define F_D_UNKN 8
#define F_RL_WLUN_OK 0x10
#define F_SKIP_UA 0x20
#define F_DELAY_OVERR 0x40
#define F_SA_LOW 0x80 /* cdb byte 1, bits 4 to 0 */
#define F_SA_HIGH 0x100 /* as used by variable length cdbs */
#define F_INV_OP 0x200
#define F_FAKE_RW 0x400
#define F_M_ACCESS 0x800 /* media access */
#define F_SSU_DELAY 0x1000
#define F_SYNC_DELAY 0x2000

#define F_RL_WLUN_OK 0x10 /* allowed with REPORT LUNS W-LUN */
#define F_SKIP_UA 0x20 /* bypass UAs (e.g. INQUIRY command) */
#define F_DELAY_OVERR 0x40 /* for commands like INQUIRY */
#define F_SA_LOW 0x80 /* SA is in cdb byte 1, bits 4 to 0 */
#define F_SA_HIGH 0x100 /* SA is in cdb bytes 8 and 9 */
#define F_INV_OP 0x200 /* invalid opcode (not supported) */
#define F_FAKE_RW 0x400 /* bypass resp_*() when fake_rw set */
#define F_M_ACCESS 0x800 /* media access, reacts to SSU state */
#define F_SSU_DELAY 0x1000 /* SSU command delay (long-ish) */
#define F_SYNC_DELAY 0x2000 /* SYNCHRONIZE CACHE delay */

/* Useful combinations of the above flags */
#define FF_RESPOND (F_RL_WLUN_OK | F_SKIP_UA | F_DELAY_OVERR)
#define FF_MEDIA_IO (F_M_ACCESS | F_FAKE_RW)
#define FF_SA (F_SA_HIGH | F_SA_LOW)
Expand Down Expand Up @@ -607,25 +609,25 @@ static const struct opcode_info_t sync_cache_iarr[] = {
};

static const struct opcode_info_t pre_fetch_iarr[] = {
{0, 0x90, 0, F_SYNC_DELAY | F_M_ACCESS, resp_pre_fetch, NULL,
{0, 0x90, 0, F_SYNC_DELAY | FF_MEDIA_IO, resp_pre_fetch, NULL,
{16, 0x2, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0x3f, 0xc7} }, /* PRE-FETCH (16) */
};

static const struct opcode_info_t zone_out_iarr[] = { /* ZONE OUT(16) */
{0, 0x94, 0x1, F_SA_LOW, resp_close_zone, NULL,
{0, 0x94, 0x1, F_SA_LOW | F_M_ACCESS, resp_close_zone, NULL,
{16, 0x1, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0, 0, 0xff, 0xff, 0x1, 0xc7} }, /* CLOSE ZONE */
{0, 0x94, 0x2, F_SA_LOW, resp_finish_zone, NULL,
{0, 0x94, 0x2, F_SA_LOW | F_M_ACCESS, resp_finish_zone, NULL,
{16, 0x2, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0, 0, 0xff, 0xff, 0x1, 0xc7} }, /* FINISH ZONE */
{0, 0x94, 0x4, F_SA_LOW, resp_rwp_zone, NULL,
{0, 0x94, 0x4, F_SA_LOW | F_M_ACCESS, resp_rwp_zone, NULL,
{16, 0x4, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0, 0, 0xff, 0xff, 0x1, 0xc7} }, /* RESET WRITE POINTER */
};

static const struct opcode_info_t zone_in_iarr[] = { /* ZONE IN(16) */
{0, 0x95, 0x6, F_SA_LOW | F_D_IN, NULL, NULL,
{0, 0x95, 0x6, F_SA_LOW | F_D_IN | F_M_ACCESS, NULL, NULL,
{16, 0x6, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0x3f, 0xc7} }, /* REPORT ZONES */
};
Expand Down Expand Up @@ -726,17 +728,17 @@ static const struct opcode_info_t opcode_info_arr[SDEB_I_LAST_ELEM_P1 + 1] = {
{0, 0x89, 0, F_D_OUT | FF_MEDIA_IO, resp_comp_write, NULL,
{16, 0xf8, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0, 0,
0, 0xff, 0x3f, 0xc7} }, /* COMPARE AND WRITE */
{ARRAY_SIZE(pre_fetch_iarr), 0x34, 0, F_SYNC_DELAY | F_M_ACCESS,
{ARRAY_SIZE(pre_fetch_iarr), 0x34, 0, F_SYNC_DELAY | FF_MEDIA_IO,
resp_pre_fetch, pre_fetch_iarr,
{10, 0x2, 0xff, 0xff, 0xff, 0xff, 0x3f, 0xff, 0xff, 0xc7, 0, 0,
0, 0, 0, 0} }, /* PRE-FETCH (10) */

/* 30 */
{ARRAY_SIZE(zone_out_iarr), 0x94, 0x3, F_SA_LOW,
{ARRAY_SIZE(zone_out_iarr), 0x94, 0x3, F_SA_LOW | F_M_ACCESS,
resp_open_zone, zone_out_iarr, /* ZONE_OUT(16), OPEN ZONE) */
{16, 0x3 /* SA */, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0x0, 0x0, 0xff, 0xff, 0x1, 0xc7} },
{ARRAY_SIZE(zone_in_iarr), 0x95, 0x0, F_SA_LOW | F_D_IN,
{ARRAY_SIZE(zone_in_iarr), 0x95, 0x0, F_SA_LOW | F_M_ACCESS,
resp_report_zones, zone_in_iarr, /* ZONE_IN(16), REPORT ZONES) */
{16, 0x0 /* SA */, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xbf, 0xc7} },
Expand Down Expand Up @@ -2875,10 +2877,20 @@ static inline int check_device_access_params
return 0;
}

static inline struct sdeb_store_info *devip2sip(struct sdebug_dev_info *devip)
/*
* 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)
{
return sdebug_fake_rw ?
NULL : xa_load(per_store_ap, devip->sdbg_host->si_idx);
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);
}

/* Returns number of bytes copied or -1 if error. */
Expand Down Expand Up @@ -3015,7 +3027,7 @@ static void dif_copy_prot(struct scsi_cmnd *scp, sector_t sector,
size_t resid;
void *paddr;
struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
scp->device->hostdata);
scp->device->hostdata, true);
struct t10_pi_tuple *dif_storep = sip->dif_storep;
const void *dif_store_end = dif_storep + sdebug_store_sectors;
struct sg_mapping_iter miter;
Expand Down Expand Up @@ -3061,7 +3073,7 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec,
unsigned int i;
sector_t sector;
struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
scp->device->hostdata);
scp->device->hostdata, true);
struct t10_pi_tuple *sdt;

for (i = 0; i < sectors; i++, ei_lba++) {
Expand Down Expand Up @@ -3094,7 +3106,7 @@ static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
u32 ei_lba;
int ret;
u64 lba;
struct sdeb_store_info *sip = devip2sip(devip);
struct sdeb_store_info *sip = devip2sip(devip, true);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
u8 *cmd = scp->cmnd;
struct sdebug_queued_cmd *sqcp;
Expand Down Expand Up @@ -3403,8 +3415,8 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
u32 ei_lba;
int ret;
u64 lba;
struct sdeb_store_info *sip = devip2sip(devip);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
struct sdeb_store_info *sip = devip2sip(devip, true);
rwlock_t *macc_lckp = &sip->macc_lck;
u8 *cmd = scp->cmnd;

switch (cmd[0]) {
Expand Down Expand Up @@ -3524,8 +3536,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
u8 *cmd = scp->cmnd;
u8 *lrdp = NULL;
u8 *up;
struct sdeb_store_info *sip = devip2sip(devip);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
struct sdeb_store_info *sip = devip2sip(devip, true);
rwlock_t *macc_lckp = &sip->macc_lck;
u8 wrprotect;
u16 lbdof, num_lrd, k;
u32 num, num_by, bt_len, lbdof_blen, sg_off, cum_lb;
Expand Down Expand Up @@ -3695,8 +3707,8 @@ static int resp_write_same(struct scsi_cmnd *scp, u64 lba, u32 num,
u32 lb_size = sdebug_sector_size;
int ret;
struct sdeb_store_info *sip = devip2sip((struct sdebug_dev_info *)
scp->device->hostdata);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
scp->device->hostdata, true);
rwlock_t *macc_lckp = &sip->macc_lck;
u8 *fs1p;
u8 *fsp;

Expand Down Expand Up @@ -3855,8 +3867,8 @@ static int resp_comp_write(struct scsi_cmnd *scp,
{
u8 *cmd = scp->cmnd;
u8 *arr;
struct sdeb_store_info *sip = devip2sip(devip);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
struct sdeb_store_info *sip = devip2sip(devip, true);
rwlock_t *macc_lckp = &sip->macc_lck;
u64 lba;
u32 dnum;
u32 lb_size = sdebug_sector_size;
Expand Down Expand Up @@ -3922,8 +3934,8 @@ static int resp_unmap(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
{
unsigned char *buf;
struct unmap_block_desc *desc;
struct sdeb_store_info *sip = devip2sip(devip);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
struct sdeb_store_info *sip = devip2sip(devip, true);
rwlock_t *macc_lckp = &sip->macc_lck;
unsigned int i, payload_len, descriptors;
int ret;

Expand Down Expand Up @@ -3980,7 +3992,6 @@ static int resp_get_lba_status(struct scsi_cmnd *scp,
struct sdebug_dev_info *devip)
{
u8 *cmd = scp->cmnd;
struct sdeb_store_info *sip = devip2sip(devip);
u64 lba;
u32 alloc_len, mapped, num;
int ret;
Expand All @@ -3996,9 +4007,11 @@ static int resp_get_lba_status(struct scsi_cmnd *scp,
if (ret)
return ret;

if (scsi_debug_lbp())
if (scsi_debug_lbp()) {
struct sdeb_store_info *sip = devip2sip(devip, true);

mapped = map_state(sip, lba, &num);
else {
} else {
mapped = 1;
/* following just in case virtual_gb changed */
sdebug_capacity = get_sdebug_capacity();
Expand Down Expand Up @@ -4058,9 +4071,9 @@ static int resp_pre_fetch(struct scsi_cmnd *scp,
u64 block, rest = 0;
u32 nblks;
u8 *cmd = scp->cmnd;
struct sdeb_store_info *sip = devip2sip(devip);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
u8 *fsp = sip ? sip->storep : NULL;
struct sdeb_store_info *sip = devip2sip(devip, true);
rwlock_t *macc_lckp = &sip->macc_lck;
u8 *fsp = sip->storep;

if (cmd[0] == PRE_FETCH) { /* 10 byte cdb */
lba = get_unaligned_be32(cmd + 2);
Expand Down Expand Up @@ -4204,8 +4217,8 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
u64 lba;
u8 *arr;
u8 *cmd = scp->cmnd;
struct sdeb_store_info *sip = devip2sip(devip);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;
struct sdeb_store_info *sip = devip2sip(devip, true);
rwlock_t *macc_lckp = &sip->macc_lck;

bytchk = (cmd[1] >> 1) & 0x3;
if (bytchk == 0) {
Expand Down Expand Up @@ -4283,7 +4296,7 @@ static int resp_report_zones(struct scsi_cmnd *scp,
u8 *arr = NULL, *desc;
u8 *cmd = scp->cmnd;
struct sdeb_zone_state *zsp;
struct sdeb_store_info *sip = devip2sip(devip);
struct sdeb_store_info *sip = devip2sip(devip, false);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;

if (!sdebug_dev_is_zoned(devip)) {
Expand Down Expand Up @@ -4424,7 +4437,7 @@ static int resp_open_zone(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
u8 *cmd = scp->cmnd;
struct sdeb_zone_state *zsp;
bool all = cmd[14] & 0x01;
struct sdeb_store_info *sip = devip2sip(devip);
struct sdeb_store_info *sip = devip2sip(devip, false);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;

if (!sdebug_dev_is_zoned(devip)) {
Expand Down Expand Up @@ -4503,7 +4516,7 @@ static int resp_close_zone(struct scsi_cmnd *scp,
u8 *cmd = scp->cmnd;
struct sdeb_zone_state *zsp;
bool all = cmd[14] & 0x01;
struct sdeb_store_info *sip = devip2sip(devip);
struct sdeb_store_info *sip = devip2sip(devip, false);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;

if (!sdebug_dev_is_zoned(devip)) {
Expand Down Expand Up @@ -4576,7 +4589,7 @@ static int resp_finish_zone(struct scsi_cmnd *scp,
u64 z_id;
u8 *cmd = scp->cmnd;
bool all = cmd[14] & 0x01;
struct sdeb_store_info *sip = devip2sip(devip);
struct sdeb_store_info *sip = devip2sip(devip, false);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;

if (!sdebug_dev_is_zoned(devip)) {
Expand Down Expand Up @@ -4652,7 +4665,7 @@ static int resp_rwp_zone(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
u64 z_id;
u8 *cmd = scp->cmnd;
bool all = cmd[14] & 0x01;
struct sdeb_store_info *sip = devip2sip(devip);
struct sdeb_store_info *sip = devip2sip(devip, false);
rwlock_t *macc_lckp = sip ? &sip->macc_lck : &sdeb_fake_rw_lck;

if (!sdebug_dev_is_zoned(devip)) {
Expand Down

0 comments on commit b6ff8ca

Please sign in to comment.