2018-07-31 19:54:43

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 0/9] block: Consolidate scsi sense buffer usage

This is a follow-up to commit f7068114d45e ("sr: pass down correctly
sized SCSI sense buffer") which further cleans up and removes needless
sense character array buffers and "struct request_sense" usage in favor
of the common "struct scsi_sense_hdr".

First, drop a bunch of unused sense buffers:
[PATCH 1/9] ide-cd: Drop unused sense buffers
[PATCH 2/9] scsi: cxlflash: Drop unused sense buffers

Next, allow the sense buffer to be usable outside SCSI tree:
[PATCH 3/9] scsi: build scsi_common.o for all scsi passthrough request users
[PATCH 4/9] target: don't depend on SCSI

Then move all request_sense usage to scsi_sense_hdr:
[PATCH 5/9] block: Switch struct packet_command to use struct scsi_sense_hdr

And do some further cleanups for scsi_sense_hdr now that we can:
[PATCH 6/9] ide-cd: Remove redundant sense buffer
[PATCH 7/9] cdrom: Use struct scsi_sense_hdr internally

Finally add a build-time check to make sure we don't pass bad buffer sizes:
[PATCH 8/9] libata-scsi: Move sense buffers onto stack
[PATCH 9/9] scsi: Check sense buffer size at build time


-Kees

Christoph Hellwig (2):
scsi: build scsi_common.o for all scsi passthrough request users
target: don't depend on SCSI

Kees Cook (7):
ide-cd: Drop unused sense buffers
scsi: cxlflash: Drop unused sense buffers
block: Switch struct packet_command to use struct scsi_sense_hdr
ide-cd: Remove redundant sense buffer
cdrom: Use struct scsi_sense_hdr internally
libata-scsi: Move sense buffers onto stack
scsi: Check sense buffer size at build time

drivers/Makefile | 2 +-
drivers/ata/libata-scsi.c | 18 +++------
drivers/block/Kconfig | 2 +-
drivers/block/pktcdvd.c | 36 +++++++++---------
drivers/cdrom/cdrom.c | 30 ++++++++-------
drivers/ide/ide-cd.c | 58 ++++++++++++++---------------
drivers/ide/ide-cd.h | 6 +--
drivers/ide/ide-cd_ioctl.c | 62 +++++++++++++------------------
drivers/scsi/Makefile | 2 +-
drivers/scsi/cxlflash/superpipe.c | 8 +---
drivers/scsi/cxlflash/vlun.c | 7 +---
drivers/scsi/scsi_lib.c | 6 +--
drivers/scsi/sr_ioctl.c | 22 +++++------
drivers/target/Kconfig | 5 ++-
include/linux/cdrom.h | 3 +-
include/scsi/scsi_cmnd.h | 6 +--
include/scsi/scsi_device.h | 14 ++++++-
17 files changed, 136 insertions(+), 151 deletions(-)

--
2.17.1



2018-07-31 19:53:33

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 1/9] ide-cd: Drop unused sense buffers

This drops unused sense buffers from:

cdrom_eject()
cdrom_read_capacity()
cdrom_read_tocentry()
ide_cd_lockdoor()
ide_cd_read_toc()

Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
drivers/ide/ide-cd.c | 36 +++++++++++++++---------------------
drivers/ide/ide-cd.h | 2 +-
drivers/ide/ide-cd_ioctl.c | 34 ++++++++++++----------------------
3 files changed, 28 insertions(+), 44 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 5a8e8e3c22cd..1d5b35312e33 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -890,8 +890,7 @@ int cdrom_check_status(ide_drive_t *drive, struct request_sense *sense)
}

static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
- unsigned long *sectors_per_frame,
- struct request_sense *sense)
+ unsigned long *sectors_per_frame)
{
struct {
__be32 lba;
@@ -908,7 +907,7 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
memset(cmd, 0, BLK_MAX_CDB);
cmd[0] = GPCMD_READ_CDVD_CAPACITY;

- stat = ide_cd_queue_pc(drive, cmd, 0, &capbuf, &len, sense, 0,
+ stat = ide_cd_queue_pc(drive, cmd, 0, &capbuf, &len, NULL, 0,
RQF_QUIET);
if (stat)
return stat;
@@ -944,8 +943,7 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
}

static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
- int format, char *buf, int buflen,
- struct request_sense *sense)
+ int format, char *buf, int buflen)
{
unsigned char cmd[BLK_MAX_CDB];

@@ -962,11 +960,11 @@ static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
if (msf_flag)
cmd[1] = 2;

- return ide_cd_queue_pc(drive, cmd, 0, buf, &buflen, sense, 0, RQF_QUIET);
+ return ide_cd_queue_pc(drive, cmd, 0, buf, &buflen, NULL, 0, RQF_QUIET);
}

/* Try to read the entire TOC for the disk into our internal buffer. */
-int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
+int ide_cd_read_toc(ide_drive_t *drive)
{
int stat, ntracks, i;
struct cdrom_info *info = drive->driver_data;
@@ -996,14 +994,13 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
* Check to see if the existing data is still valid. If it is,
* just return.
*/
- (void) cdrom_check_status(drive, sense);
+ (void) cdrom_check_status(drive, NULL);

if (drive->atapi_flags & IDE_AFLAG_TOC_VALID)
return 0;

/* try to get the total cdrom capacity and sector size */
- stat = cdrom_read_capacity(drive, &toc->capacity, &sectors_per_frame,
- sense);
+ stat = cdrom_read_capacity(drive, &toc->capacity, &sectors_per_frame);
if (stat)
toc->capacity = 0x1fffff;

@@ -1016,7 +1013,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)

/* first read just the header, so we know how long the TOC is */
stat = cdrom_read_tocentry(drive, 0, 1, 0, (char *) &toc->hdr,
- sizeof(struct atapi_toc_header), sense);
+ sizeof(struct atapi_toc_header));
if (stat)
return stat;

@@ -1036,7 +1033,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
(char *)&toc->hdr,
sizeof(struct atapi_toc_header) +
(ntracks + 1) *
- sizeof(struct atapi_toc_entry), sense);
+ sizeof(struct atapi_toc_entry));

if (stat && toc->hdr.first_track > 1) {
/*
@@ -1056,8 +1053,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
(char *)&toc->hdr,
sizeof(struct atapi_toc_header) +
(ntracks + 1) *
- sizeof(struct atapi_toc_entry),
- sense);
+ sizeof(struct atapi_toc_entry));
if (stat)
return stat;

@@ -1094,7 +1090,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
if (toc->hdr.first_track != CDROM_LEADOUT) {
/* read the multisession information */
stat = cdrom_read_tocentry(drive, 0, 0, 1, (char *)&ms_tmp,
- sizeof(ms_tmp), sense);
+ sizeof(ms_tmp));
if (stat)
return stat;

@@ -1108,7 +1104,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
if (drive->atapi_flags & IDE_AFLAG_TOCADDR_AS_BCD) {
/* re-read multisession information using MSF format */
stat = cdrom_read_tocentry(drive, 0, 1, 1, (char *)&ms_tmp,
- sizeof(ms_tmp), sense);
+ sizeof(ms_tmp));
if (stat)
return stat;

@@ -1412,7 +1408,7 @@ static sector_t ide_cdrom_capacity(ide_drive_t *drive)
{
unsigned long capacity, sectors_per_frame;

- if (cdrom_read_capacity(drive, &capacity, &sectors_per_frame, NULL))
+ if (cdrom_read_capacity(drive, &capacity, &sectors_per_frame))
return 0;

return capacity * sectors_per_frame;
@@ -1723,9 +1719,8 @@ static unsigned int idecd_check_events(struct gendisk *disk,
static int idecd_revalidate_disk(struct gendisk *disk)
{
struct cdrom_info *info = ide_drv_g(disk, cdrom_info);
- struct request_sense sense;

- ide_cd_read_toc(info->drive, &sense);
+ ide_cd_read_toc(info->drive);

return 0;
}
@@ -1749,7 +1744,6 @@ static int ide_cd_probe(ide_drive_t *drive)
{
struct cdrom_info *info;
struct gendisk *g;
- struct request_sense sense;

ide_debug_log(IDE_DBG_PROBE, "driver_req: %s, media: 0x%x",
drive->driver_req, drive->media);
@@ -1798,7 +1792,7 @@ static int ide_cd_probe(ide_drive_t *drive)
goto failed;
}

- ide_cd_read_toc(drive, &sense);
+ ide_cd_read_toc(drive);
g->fops = &idecd_ops;
g->flags |= GENHD_FL_REMOVABLE | GENHD_FL_BLOCK_EVENTS_ON_EXCL_WRITE;
device_add_disk(&drive->gendev, g);
diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
index 04f0f310a856..fc162fbb6629 100644
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -99,7 +99,7 @@ void ide_cd_log_error(const char *, struct request *, struct request_sense *);
/* ide-cd.c functions used by ide-cd_ioctl.c */
int ide_cd_queue_pc(ide_drive_t *, const unsigned char *, int, void *,
unsigned *, struct request_sense *, int, req_flags_t);
-int ide_cd_read_toc(ide_drive_t *, struct request_sense *);
+int ide_cd_read_toc(ide_drive_t *);
int ide_cdrom_get_capabilities(ide_drive_t *, u8 *);
void ide_cdrom_update_speed(ide_drive_t *, u8 *);
int cdrom_check_status(ide_drive_t *, struct request_sense *);
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index 2acca12b9c94..c709f4eed0e9 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -105,8 +105,7 @@ unsigned int ide_cdrom_check_events_real(struct cdrom_device_info *cdi,
/* Eject the disk if EJECTFLAG is 0.
If EJECTFLAG is 1, try to reload the disk. */
static
-int cdrom_eject(ide_drive_t *drive, int ejectflag,
- struct request_sense *sense)
+int cdrom_eject(ide_drive_t *drive, int ejectflag)
{
struct cdrom_info *cd = drive->driver_data;
struct cdrom_device_info *cdi = &cd->devinfo;
@@ -129,20 +128,16 @@ int cdrom_eject(ide_drive_t *drive, int ejectflag,
cmd[0] = GPCMD_START_STOP_UNIT;
cmd[4] = loej | (ejectflag != 0);

- return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, sense, 0, 0);
+ return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, NULL, 0, 0);
}

/* Lock the door if LOCKFLAG is nonzero; unlock it otherwise. */
static
-int ide_cd_lockdoor(ide_drive_t *drive, int lockflag,
- struct request_sense *sense)
+int ide_cd_lockdoor(ide_drive_t *drive, int lockflag)
{
- struct request_sense my_sense;
+ struct request_sense my_sense, *sense = &my_sense;
int stat;

- if (sense == NULL)
- sense = &my_sense;
-
/* If the drive cannot lock the door, just pretend. */
if ((drive->dev_flags & IDE_DFLAG_DOORLOCKING) == 0) {
stat = 0;
@@ -186,23 +181,22 @@ int ide_cd_lockdoor(ide_drive_t *drive, int lockflag,
int ide_cdrom_tray_move(struct cdrom_device_info *cdi, int position)
{
ide_drive_t *drive = cdi->handle;
- struct request_sense sense;

if (position) {
- int stat = ide_cd_lockdoor(drive, 0, &sense);
+ int stat = ide_cd_lockdoor(drive, 0);

if (stat)
return stat;
}

- return cdrom_eject(drive, !position, &sense);
+ return cdrom_eject(drive, !position);
}

int ide_cdrom_lock_door(struct cdrom_device_info *cdi, int lock)
{
ide_drive_t *drive = cdi->handle;

- return ide_cd_lockdoor(drive, lock, NULL);
+ return ide_cd_lockdoor(drive, lock);
}

/*
@@ -213,7 +207,6 @@ int ide_cdrom_select_speed(struct cdrom_device_info *cdi, int speed)
{
ide_drive_t *drive = cdi->handle;
struct cdrom_info *cd = drive->driver_data;
- struct request_sense sense;
u8 buf[ATAPI_CAPABILITIES_PAGE_SIZE];
int stat;
unsigned char cmd[BLK_MAX_CDB];
@@ -236,7 +229,7 @@ int ide_cdrom_select_speed(struct cdrom_device_info *cdi, int speed)
cmd[5] = speed & 0xff;
}

- stat = ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, &sense, 0, 0);
+ stat = ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, NULL, 0, 0);

if (!ide_cdrom_get_capabilities(drive, buf)) {
ide_cdrom_update_speed(drive, buf);
@@ -252,11 +245,10 @@ int ide_cdrom_get_last_session(struct cdrom_device_info *cdi,
struct atapi_toc *toc;
ide_drive_t *drive = cdi->handle;
struct cdrom_info *info = drive->driver_data;
- struct request_sense sense;
int ret;

if ((drive->atapi_flags & IDE_AFLAG_TOC_VALID) == 0 || !info->toc) {
- ret = ide_cd_read_toc(drive, &sense);
+ ret = ide_cd_read_toc(drive);
if (ret)
return ret;
}
@@ -300,7 +292,6 @@ int ide_cdrom_reset(struct cdrom_device_info *cdi)
{
ide_drive_t *drive = cdi->handle;
struct cdrom_info *cd = drive->driver_data;
- struct request_sense sense;
struct request *rq;
int ret;

@@ -315,7 +306,7 @@ int ide_cdrom_reset(struct cdrom_device_info *cdi)
* lock it again.
*/
if (drive->atapi_flags & IDE_AFLAG_DOOR_LOCKED)
- (void)ide_cd_lockdoor(drive, 1, &sense);
+ (void)ide_cd_lockdoor(drive, 1);

return ret;
}
@@ -355,7 +346,6 @@ static int ide_cd_fake_play_trkind(ide_drive_t *drive, void *arg)
struct atapi_toc_entry *first_toc, *last_toc;
unsigned long lba_start, lba_end;
int stat;
- struct request_sense sense;
unsigned char cmd[BLK_MAX_CDB];

stat = ide_cd_get_toc_entry(drive, ti->cdti_trk0, &first_toc);
@@ -380,7 +370,7 @@ static int ide_cd_fake_play_trkind(ide_drive_t *drive, void *arg)
lba_to_msf(lba_start, &cmd[3], &cmd[4], &cmd[5]);
lba_to_msf(lba_end - 1, &cmd[6], &cmd[7], &cmd[8]);

- return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, &sense, 0, 0);
+ return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, NULL, 0, 0);
}

static int ide_cd_read_tochdr(ide_drive_t *drive, void *arg)
@@ -391,7 +381,7 @@ static int ide_cd_read_tochdr(ide_drive_t *drive, void *arg)
int stat;

/* Make sure our saved TOC is valid. */
- stat = ide_cd_read_toc(drive, NULL);
+ stat = ide_cd_read_toc(drive);
if (stat)
return stat;

--
2.17.1


2018-07-31 19:53:46

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 9/9] scsi: Check sense buffer size at build time

To avoid introducing problems like those fixed in commit f7068114d45e
("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
wrapper for scsi_execute() that verifies the size of the sense buffer
similar to what was done for command string sizes in commit 3756f6401c30
("exec: avoid gcc-8 warning for get_task_comm").

Another solution could be to add a length argument to scsi_execute(),
but this function already takes a lot of arguments and Jens was not fond
of that approach.

Additionally, this moves the SCSI_SENSE_BUFFERSIZE definition into
scsi_device.h, and removes a redundant include for scsi_device.h from
scsi_cmnd.h.

Signed-off-by: Kees Cook <[email protected]>
---
drivers/scsi/scsi_lib.c | 6 +++---
include/scsi/scsi_cmnd.h | 6 ++----
include/scsi/scsi_device.h | 14 +++++++++++++-
3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9b4f279d29c..718c2bec4516 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -238,7 +238,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)


/**
- * scsi_execute - insert request and wait for the result
+ * __scsi_execute - insert request and wait for the result
* @sdev: scsi device
* @cmd: scsi command
* @data_direction: data direction
@@ -255,7 +255,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
* Returns the scsi_cmnd result field if a command was executed, or a negative
* Linux error code if we didn't get that far.
*/
-int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags, req_flags_t rq_flags,
@@ -309,7 +309,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,

return ret;
}
-EXPORT_SYMBOL(scsi_execute);
+EXPORT_SYMBOL(__scsi_execute);

/*
* Function: scsi_init_cmd_errh()
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index aaf1e971c6a3..7bf043a66e10 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -14,8 +14,6 @@
struct Scsi_Host;
struct scsi_driver;

-#include <scsi/scsi_device.h>
-
/*
* MAX_COMMAND_SIZE is:
* The longest fixed-length SCSI CDB as per the SCSI standard.
@@ -120,11 +118,11 @@ struct scsi_cmnd {
struct request *request; /* The command we are
working on */

-#define SCSI_SENSE_BUFFERSIZE 96
unsigned char *sense_buffer;
/* obtained by REQUEST SENSE when
* CHECK CONDITION is received on original
- * command (auto-sense) */
+ * command (auto-sense). Length must be
+ * SCSI_SENSE_BUFFERSIZE bytes. */

/* Low-level done function - can be used by low-level driver to point
* to completion function. Not used by mid/upper level code. */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ae177c8e399..96b626ad5fb1 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -17,6 +17,8 @@ struct scsi_sense_hdr;

typedef unsigned int __bitwise blist_flags_t;

+#define SCSI_SENSE_BUFFERSIZE 96
+
struct scsi_mode_data {
__u32 length;
__u16 block_descriptor_length;
@@ -426,11 +428,21 @@ extern const char *scsi_device_state_name(enum scsi_device_state);
extern int scsi_is_sdev_device(const struct device *);
extern int scsi_is_target_device(const struct device *);
extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
-extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags,
req_flags_t rq_flags, int *resid);
+/* Make sure any sense buffer is the correct size. */
+#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense, \
+ sshdr, timeout, retries, flags, rq_flags, resid) \
+({ \
+ BUILD_BUG_ON((sense) != NULL && \
+ sizeof(sense) != SCSI_SENSE_BUFFERSIZE); \
+ __scsi_execute(sdev, cmd, data_direction, buffer, bufflen, \
+ sense, sshdr, timeout, retries, flags, rq_flags, \
+ resid); \
+})
static inline int scsi_execute_req(struct scsi_device *sdev,
const unsigned char *cmd, int data_direction, void *buffer,
unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
--
2.17.1


2018-07-31 19:54:01

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 5/9] block: Switch struct packet_command to use struct scsi_sense_hdr

There is a lot of needless struct request_sense usage in the CDROM
code. These can all be struct scsi_sense_hdr instead, to avoid any
confusion over their respective structure sizes. This patch is a lot
of noise changing "sense" to "sshdr", but the final code is more
readable to distinguish between "sense" meaning "struct request_sense"
and "sshdr" meaning "struct scsi_sense_hdr".

Signed-off-by: Kees Cook <[email protected]>
---
drivers/block/pktcdvd.c | 36 ++++++++++++++++++------------------
drivers/cdrom/cdrom.c | 22 +++++++++++-----------
drivers/ide/ide-cd.c | 11 ++++++-----
drivers/ide/ide-cd.h | 4 ++--
drivers/ide/ide-cd_ioctl.c | 30 +++++++++++++++---------------
drivers/scsi/sr_ioctl.c | 22 +++++++++-------------
include/linux/cdrom.h | 3 ++-
7 files changed, 63 insertions(+), 65 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index c61d20c9f3f8..f91c9f85e29d 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -748,13 +748,13 @@ static const char *sense_key_string(__u8 index)
static void pkt_dump_sense(struct pktcdvd_device *pd,
struct packet_command *cgc)
{
- struct request_sense *sense = cgc->sense;
+ struct scsi_sense_hdr *sshdr = cgc->sshdr;

- if (sense)
+ if (sshdr)
pkt_err(pd, "%*ph - sense %02x.%02x.%02x (%s)\n",
CDROM_PACKET_SIZE, cgc->cmd,
- sense->sense_key, sense->asc, sense->ascq,
- sense_key_string(sense->sense_key));
+ sshdr->sense_key, sshdr->asc, sshdr->ascq,
+ sense_key_string(sshdr->sense_key));
else
pkt_err(pd, "%*ph - no sense\n", CDROM_PACKET_SIZE, cgc->cmd);
}
@@ -787,11 +787,11 @@ static noinline_for_stack int pkt_set_speed(struct pktcdvd_device *pd,
unsigned write_speed, unsigned read_speed)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
int ret;

init_cdrom_command(&cgc, NULL, 0, CGC_DATA_NONE);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
cgc.cmd[0] = GPCMD_SET_SPEED;
cgc.cmd[2] = (read_speed >> 8) & 0xff;
cgc.cmd[3] = read_speed & 0xff;
@@ -1645,7 +1645,7 @@ static noinline_for_stack int pkt_get_last_written(struct pktcdvd_device *pd,
static noinline_for_stack int pkt_set_write_settings(struct pktcdvd_device *pd)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
write_param_page *wp;
char buffer[128];
int ret, size;
@@ -1656,7 +1656,7 @@ static noinline_for_stack int pkt_set_write_settings(struct pktcdvd_device *pd)

memset(buffer, 0, sizeof(buffer));
init_cdrom_command(&cgc, buffer, sizeof(*wp), CGC_DATA_READ);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
if ((ret = pkt_mode_sense(pd, &cgc, GPMODE_WRITE_PARMS_PAGE, 0))) {
pkt_dump_sense(pd, &cgc);
return ret;
@@ -1671,7 +1671,7 @@ static noinline_for_stack int pkt_set_write_settings(struct pktcdvd_device *pd)
* now get it all
*/
init_cdrom_command(&cgc, buffer, size, CGC_DATA_READ);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
if ((ret = pkt_mode_sense(pd, &cgc, GPMODE_WRITE_PARMS_PAGE, 0))) {
pkt_dump_sense(pd, &cgc);
return ret;
@@ -1905,12 +1905,12 @@ static noinline_for_stack int pkt_write_caching(struct pktcdvd_device *pd,
int set)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
unsigned char buf[64];
int ret;

init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_READ);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
cgc.buflen = pd->mode_offset + 12;

/*
@@ -1950,14 +1950,14 @@ static noinline_for_stack int pkt_get_max_speed(struct pktcdvd_device *pd,
unsigned *write_speed)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
unsigned char buf[256+18];
unsigned char *cap_buf;
int ret, offset;

cap_buf = &buf[sizeof(struct mode_page_header) + pd->mode_offset];
init_cdrom_command(&cgc, buf, sizeof(buf), CGC_DATA_UNKNOWN);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;

ret = pkt_mode_sense(pd, &cgc, GPMODE_CAPABILITIES_PAGE, 0);
if (ret) {
@@ -2011,13 +2011,13 @@ static noinline_for_stack int pkt_media_speed(struct pktcdvd_device *pd,
unsigned *speed)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
unsigned char buf[64];
unsigned int size, st, sp;
int ret;

init_cdrom_command(&cgc, buf, 2, CGC_DATA_READ);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
cgc.cmd[0] = GPCMD_READ_TOC_PMA_ATIP;
cgc.cmd[1] = 2;
cgc.cmd[2] = 4; /* READ ATIP */
@@ -2032,7 +2032,7 @@ static noinline_for_stack int pkt_media_speed(struct pktcdvd_device *pd,
size = sizeof(buf);

init_cdrom_command(&cgc, buf, size, CGC_DATA_READ);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
cgc.cmd[0] = GPCMD_READ_TOC_PMA_ATIP;
cgc.cmd[1] = 2;
cgc.cmd[2] = 4;
@@ -2083,13 +2083,13 @@ static noinline_for_stack int pkt_media_speed(struct pktcdvd_device *pd,
static noinline_for_stack int pkt_perform_opc(struct pktcdvd_device *pd)
{
struct packet_command cgc;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
int ret;

pkt_dbg(2, pd, "Performing OPC\n");

init_cdrom_command(&cgc, NULL, 0, CGC_DATA_NONE);
- cgc.sense = &sense;
+ cgc.sshdr = &sshdr;
cgc.timeout = 60*HZ;
cgc.cmd[0] = GPCMD_SEND_OPC;
cgc.cmd[1] = 1;
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index bfc566d3f31a..3522d2cae1b6 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -345,10 +345,10 @@ static LIST_HEAD(cdrom_list);
int cdrom_dummy_generic_packet(struct cdrom_device_info *cdi,
struct packet_command *cgc)
{
- if (cgc->sense) {
- cgc->sense->sense_key = 0x05;
- cgc->sense->asc = 0x20;
- cgc->sense->ascq = 0x00;
+ if (cgc->sshdr) {
+ cgc->sshdr->sense_key = 0x05;
+ cgc->sshdr->asc = 0x20;
+ cgc->sshdr->ascq = 0x00;
}

cgc->stat = -EIO;
@@ -2943,7 +2943,7 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
struct packet_command *cgc,
int cmd)
{
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
struct cdrom_msf msf;
int blocksize = 0, format = 0, lba;
int ret;
@@ -2971,13 +2971,13 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
if (cgc->buffer == NULL)
return -ENOMEM;

- memset(&sense, 0, sizeof(sense));
- cgc->sense = &sense;
+ memset(&sshdr, 0, sizeof(sshdr));
+ cgc->sshdr = &sshdr;
cgc->data_direction = CGC_DATA_READ;
ret = cdrom_read_block(cdi, cgc, lba, 1, format, blocksize);
- if (ret && sense.sense_key == 0x05 &&
- sense.asc == 0x20 &&
- sense.ascq == 0x00) {
+ if (ret && sshdr.sense_key == 0x05 &&
+ sshdr.asc == 0x20 &&
+ sshdr.ascq == 0x00) {
/*
* SCSI-II devices are not required to support
* READ_CD, so let's try switching block size
@@ -2986,7 +2986,7 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
ret = cdrom_switch_blocksize(cdi, blocksize);
if (ret)
goto out;
- cgc->sense = NULL;
+ cgc->sshdr = NULL;
ret = cdrom_read_cd(cdi, cgc, lba, blocksize, 1);
ret |= cdrom_switch_blocksize(cdi, blocksize);
}
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 1d5b35312e33..cb90560acf6e 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -419,7 +419,7 @@ static void ide_cd_request_sense_fixup(ide_drive_t *drive, struct ide_cmd *cmd)

int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
int write, void *buffer, unsigned *bufflen,
- struct request_sense *sense, int timeout,
+ struct scsi_sense_hdr *sshdr, int timeout,
req_flags_t rq_flags)
{
struct cdrom_info *info = drive->driver_data;
@@ -456,8 +456,9 @@ int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,

if (buffer)
*bufflen = scsi_req(rq)->resid_len;
- if (sense)
- memcpy(sense, scsi_req(rq)->sense, sizeof(*sense));
+ if (sshdr)
+ scsi_normalize_sense(scsi_req(rq)->sense,
+ scsi_req(rq)->sense_len, sshdr);

/*
* FIXME: we should probably abort/retry or something in case of
@@ -864,7 +865,7 @@ static void msf_from_bcd(struct atapi_msf *msf)
msf->frame = bcd2bin(msf->frame);
}

-int cdrom_check_status(ide_drive_t *drive, struct request_sense *sense)
+int cdrom_check_status(ide_drive_t *drive, struct scsi_sense_hdr *sshdr)
{
struct cdrom_info *info = drive->driver_data;
struct cdrom_device_info *cdi;
@@ -886,7 +887,7 @@ int cdrom_check_status(ide_drive_t *drive, struct request_sense *sense)
*/
cmd[7] = cdi->sanyo_slot % 3;

- return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, sense, 0, RQF_QUIET);
+ return ide_cd_queue_pc(drive, cmd, 0, NULL, NULL, sshdr, 0, RQF_QUIET);
}

static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
diff --git a/drivers/ide/ide-cd.h b/drivers/ide/ide-cd.h
index fc162fbb6629..a69dc7f61c4d 100644
--- a/drivers/ide/ide-cd.h
+++ b/drivers/ide/ide-cd.h
@@ -98,11 +98,11 @@ void ide_cd_log_error(const char *, struct request *, struct request_sense *);

/* ide-cd.c functions used by ide-cd_ioctl.c */
int ide_cd_queue_pc(ide_drive_t *, const unsigned char *, int, void *,
- unsigned *, struct request_sense *, int, req_flags_t);
+ unsigned *, struct scsi_sense_hdr *, int, req_flags_t);
int ide_cd_read_toc(ide_drive_t *);
int ide_cdrom_get_capabilities(ide_drive_t *, u8 *);
void ide_cdrom_update_speed(ide_drive_t *, u8 *);
-int cdrom_check_status(ide_drive_t *, struct request_sense *);
+int cdrom_check_status(ide_drive_t *, struct scsi_sense_hdr *);

/* ide-cd_ioctl.c */
int ide_cdrom_open_real(struct cdrom_device_info *, int);
diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
index c709f4eed0e9..c945beb9e75a 100644
--- a/drivers/ide/ide-cd_ioctl.c
+++ b/drivers/ide/ide-cd_ioctl.c
@@ -43,14 +43,14 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
{
ide_drive_t *drive = cdi->handle;
struct media_event_desc med;
- struct request_sense sense;
+ struct scsi_sense_hdr sshdr;
int stat;

if (slot_nr != CDSL_CURRENT)
return -EINVAL;

- stat = cdrom_check_status(drive, &sense);
- if (!stat || sense.sense_key == UNIT_ATTENTION)
+ stat = cdrom_check_status(drive, &sshdr);
+ if (!stat || sshdr.sense_key == UNIT_ATTENTION)
return CDS_DISC_OK;

if (!cdrom_get_media_event(cdi, &med)) {
@@ -62,8 +62,8 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
return CDS_NO_DISC;
}

- if (sense.sense_key == NOT_READY && sense.asc == 0x04
- && sense.ascq == 0x04)
+ if (sshdr.sense_key == NOT_READY && sshdr.asc == 0x04
+ && sshdr.ascq == 0x04)
return CDS_DISC_OK;

/*
@@ -71,8 +71,8 @@ int ide_cdrom_drive_status(struct cdrom_device_info *cdi, int slot_nr)
* just return TRAY_OPEN since ATAPI doesn't provide
* any other way to detect this...
*/
- if (sense.sense_key == NOT_READY) {
- if (sense.asc == 0x3a && sense.ascq == 1)
+ if (sshdr.sense_key == NOT_READY) {
+ if (sshdr.asc == 0x3a && sshdr.ascq == 1)
return CDS_NO_DISC;
else
return CDS_TRAY_OPEN;
@@ -135,7 +135,7 @@ int cdrom_eject(ide_drive_t *drive, int ejectflag)
static
int ide_cd_lockdoor(ide_drive_t *drive, int lockflag)
{
- struct request_sense my_sense, *sense = &my_sense;
+ struct scsi_sense_hdr sshdr;
int stat;

/* If the drive cannot lock the door, just pretend. */
@@ -150,14 +150,14 @@ int ide_cd_lockdoor(ide_drive_t *drive, int lockflag)
cmd[4] = lockflag ? 1 : 0;

stat = ide_cd_queue_pc(drive, cmd, 0, NULL, NULL,
- sense, 0, 0);
+ &sshdr, 0, 0);
}

/* If we got an illegal field error, the drive
probably cannot lock the door. */
if (stat != 0 &&
- sense->sense_key == ILLEGAL_REQUEST &&
- (sense->asc == 0x24 || sense->asc == 0x20)) {
+ sshdr.sense_key == ILLEGAL_REQUEST &&
+ (sshdr.asc == 0x24 || sshdr.asc == 0x20)) {
printk(KERN_ERR "%s: door locking not supported\n",
drive->name);
drive->dev_flags &= ~IDE_DFLAG_DOORLOCKING;
@@ -165,7 +165,7 @@ int ide_cd_lockdoor(ide_drive_t *drive, int lockflag)
}

/* no medium, that's alright. */
- if (stat != 0 && sense->sense_key == NOT_READY && sense->asc == 0x3a)
+ if (stat != 0 && sshdr.sense_key == NOT_READY && sshdr.asc == 0x3a)
stat = 0;

if (stat == 0) {
@@ -451,8 +451,8 @@ int ide_cdrom_packet(struct cdrom_device_info *cdi,
layer. the packet must be complete, as we do not
touch it at all. */

- if (cgc->sense)
- memset(cgc->sense, 0, sizeof(struct request_sense));
+ if (cgc->sshdr)
+ memset(cgc->sshdr, 0, sizeof(*cgc->sshdr));

if (cgc->quiet)
flags |= RQF_QUIET;
@@ -460,7 +460,7 @@ int ide_cdrom_packet(struct cdrom_device_info *cdi,
cgc->stat = ide_cd_queue_pc(drive, cgc->cmd,
cgc->data_direction == CGC_DATA_WRITE,
cgc->buffer, &len,
- cgc->sense, cgc->timeout, flags);
+ cgc->sshdr, cgc->timeout, flags);
if (!cgc->stat)
cgc->buflen -= len;
return cgc->stat;
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index 35fab1e18adc..ffcf902da390 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -186,14 +186,13 @@ static int sr_play_trkind(struct cdrom_device_info *cdi,
int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
{
struct scsi_device *SDev;
- struct scsi_sense_hdr sshdr;
+ struct scsi_sense_hdr local_sshdr, *sshdr = &local_sshdr;
int result, err = 0, retries = 0;
- unsigned char sense_buffer[SCSI_SENSE_BUFFERSIZE], *senseptr = NULL;

SDev = cd->device;

- if (cgc->sense)
- senseptr = sense_buffer;
+ if (cgc->sshdr)
+ sshdr = cgc->sshdr;

retry:
if (!scsi_block_when_processing_errors(SDev)) {
@@ -202,15 +201,12 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
}

result = scsi_execute(SDev, cgc->cmd, cgc->data_direction,
- cgc->buffer, cgc->buflen, senseptr, &sshdr,
+ cgc->buffer, cgc->buflen, NULL, sshdr,
cgc->timeout, IOCTL_RETRIES, 0, 0, NULL);

- if (cgc->sense)
- memcpy(cgc->sense, sense_buffer, sizeof(*cgc->sense));
-
/* Minimal error checking. Ignore cases we know about, and report the rest. */
if (driver_byte(result) != 0) {
- switch (sshdr.sense_key) {
+ switch (sshdr->sense_key) {
case UNIT_ATTENTION:
SDev->changed = 1;
if (!cgc->quiet)
@@ -221,8 +217,8 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
err = -ENOMEDIUM;
break;
case NOT_READY: /* This happens if there is no disc in drive */
- if (sshdr.asc == 0x04 &&
- sshdr.ascq == 0x01) {
+ if (sshdr->asc == 0x04 &&
+ sshdr->ascq == 0x01) {
/* sense: Logical unit is in process of becoming ready */
if (!cgc->quiet)
sr_printk(KERN_INFO, cd,
@@ -245,8 +241,8 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
break;
case ILLEGAL_REQUEST:
err = -EIO;
- if (sshdr.asc == 0x20 &&
- sshdr.ascq == 0x00)
+ if (sshdr->asc == 0x20 &&
+ sshdr->ascq == 0x00)
/* sense: Invalid command operation code */
err = -EDRIVE_CANT_DO_THIS;
break;
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index e75dfd1f1dec..528271c60018 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -13,6 +13,7 @@

#include <linux/fs.h> /* not really needed, later.. */
#include <linux/list.h>
+#include <scsi/scsi_common.h>
#include <uapi/linux/cdrom.h>

struct packet_command
@@ -21,7 +22,7 @@ struct packet_command
unsigned char *buffer;
unsigned int buflen;
int stat;
- struct request_sense *sense;
+ struct scsi_sense_hdr *sshdr;
unsigned char data_direction;
int quiet;
int timeout;
--
2.17.1


2018-07-31 19:54:13

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 7/9] cdrom: Use struct scsi_sense_hdr internally

This removes more casts of struct request_sense and uses the standard
struct scsi_sense_hdr instead. This also fixes any possible stale values
since the prior code did not check the sense length.

Signed-off-by: Kees Cook <[email protected]>
---
drivers/block/Kconfig | 2 +-
drivers/cdrom/cdrom.c | 8 ++++++--
2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index ad9b687a236a..d4913516823f 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -74,12 +74,12 @@ config AMIGA_Z2RAM

config CDROM
tristate
+ select BLK_SCSI_REQUEST

config GDROM
tristate "SEGA Dreamcast GD-ROM drive"
depends on SH_DREAMCAST
select CDROM
- select BLK_SCSI_REQUEST # only for the generic cdrom code
help
A standard SEGA Dreamcast comes with a modified CD ROM drive called a
"GD-ROM" by SEGA to signify it is capable of reading special disks
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 3522d2cae1b6..e547213239dc 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -282,6 +282,7 @@
#include <linux/blkdev.h>
#include <linux/times.h>
#include <linux/uaccess.h>
+#include <scsi/scsi_common.h>
#include <scsi/scsi_request.h>

/* used to tell the module to turn on full debugging messages */
@@ -2222,9 +2223,12 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,

blk_execute_rq(q, cdi->disk, rq, 0);
if (scsi_req(rq)->result) {
- struct request_sense *s = req->sense;
+ struct scsi_sense_hdr sshdr;
+
ret = -EIO;
- cdi->last_sense = s->sense_key;
+ scsi_normalize_sense(req->sense, req->sense_len,
+ &sshdr);
+ cdi->last_sense = sshdr.sense_key;
}

if (blk_rq_unmap_user(bio))
--
2.17.1


2018-07-31 19:54:30

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 8/9] libata-scsi: Move sense buffers onto stack

To support future compile-time sizeof() checks that will be able to
validate the length of sense buffers, this removes the only dynamically
allocated sense buffers in the tree by putting the 96 byte sense buffers
on the stack.

Signed-off-by: Kees Cook <[email protected]>
---
drivers/ata/libata-scsi.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 89a9d4a2efc8..3a43d3a1ce2d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -597,8 +597,9 @@ static int ata_get_identity(struct ata_port *ap, struct scsi_device *sdev,
int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
{
int rc = 0;
+ u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
u8 scsi_cmd[MAX_COMMAND_SIZE];
- u8 args[4], *argbuf = NULL, *sensebuf = NULL;
+ u8 args[4], *argbuf = NULL;
int argsize = 0;
enum dma_data_direction data_dir;
struct scsi_sense_hdr sshdr;
@@ -610,10 +611,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
if (copy_from_user(args, arg, sizeof(args)))
return -EFAULT;

- sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
- if (!sensebuf)
- return -ENOMEM;
-
+ memset(sensebuf, 0, sizeof(sensebuf));
memset(scsi_cmd, 0, sizeof(scsi_cmd));

if (args[3]) {
@@ -685,7 +683,6 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
&& copy_to_user(arg + sizeof(args), argbuf, argsize))
rc = -EFAULT;
error:
- kfree(sensebuf);
kfree(argbuf);
return rc;
}
@@ -704,8 +701,9 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
{
int rc = 0;
+ u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
u8 scsi_cmd[MAX_COMMAND_SIZE];
- u8 args[7], *sensebuf = NULL;
+ u8 args[7];
struct scsi_sense_hdr sshdr;
int cmd_result;

@@ -715,10 +713,7 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
if (copy_from_user(args, arg, sizeof(args)))
return -EFAULT;

- sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
- if (!sensebuf)
- return -ENOMEM;
-
+ memset(sensebuf, 0, sizeof(sensebuf));
memset(scsi_cmd, 0, sizeof(scsi_cmd));
scsi_cmd[0] = ATA_16;
scsi_cmd[1] = (3 << 1); /* Non-data */
@@ -769,7 +764,6 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
}

error:
- kfree(sensebuf);
return rc;
}

--
2.17.1


2018-07-31 19:54:31

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 6/9] ide-cd: Remove redundant sense buffer

This is already able to process the sense buffer, so remove the redundant
parsing during the failure path. This also fixes any possible stale values
since the prior code did not check the sense length.

Signed-off-by: Kees Cook <[email protected]>
---
drivers/ide/ide-cd.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index cb90560acf6e..023a7d94eb08 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -423,6 +423,7 @@ int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
req_flags_t rq_flags)
{
struct cdrom_info *info = drive->driver_data;
+ struct scsi_sense_hdr local_sshdr;
int retries = 10;
bool failed;

@@ -430,6 +431,9 @@ int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
"rq_flags: 0x%x",
cmd[0], write, timeout, rq_flags);

+ if (!sshdr)
+ sshdr = &local_sshdr;
+
/* start of retry loop */
do {
struct request *rq;
@@ -456,9 +460,8 @@ int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,

if (buffer)
*bufflen = scsi_req(rq)->resid_len;
- if (sshdr)
- scsi_normalize_sense(scsi_req(rq)->sense,
- scsi_req(rq)->sense_len, sshdr);
+ scsi_normalize_sense(scsi_req(rq)->sense,
+ scsi_req(rq)->sense_len, sshdr);

/*
* FIXME: we should probably abort/retry or something in case of
@@ -470,12 +473,10 @@ int ide_cd_queue_pc(ide_drive_t *drive, const unsigned char *cmd,
* The request failed. Retry if it was due to a unit
* attention status (usually means media was changed).
*/
- struct request_sense *reqbuf = scsi_req(rq)->sense;
-
- if (reqbuf->sense_key == UNIT_ATTENTION)
+ if (sshdr->sense_key == UNIT_ATTENTION)
cdrom_saw_media_change(drive);
- else if (reqbuf->sense_key == NOT_READY &&
- reqbuf->asc == 4 && reqbuf->ascq != 4) {
+ else if (sshdr->sense_key == NOT_READY &&
+ sshdr->asc == 4 && sshdr->ascq != 4) {
/*
* The drive is in the process of loading
* a disk. Retry, but wait a little to give
--
2.17.1


2018-07-31 19:55:49

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 2/9] scsi: cxlflash: Drop unused sense buffers

This removes the unused sense buffer in read_cap16() and write_same16().

Signed-off-by: Kees Cook <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Acked-by: Matthew R. Ochs <[email protected]>
---
drivers/scsi/cxlflash/superpipe.c | 8 ++------
drivers/scsi/cxlflash/vlun.c | 7 ++-----
2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index 2fe79df5c73c..59b9f2023748 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -324,7 +324,6 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
struct scsi_sense_hdr sshdr;
u8 *cmd_buf = NULL;
u8 *scsi_cmd = NULL;
- u8 *sense_buf = NULL;
int rc = 0;
int result = 0;
int retry_cnt = 0;
@@ -333,8 +332,7 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
retry:
cmd_buf = kzalloc(CMD_BUFSIZE, GFP_KERNEL);
scsi_cmd = kzalloc(MAX_COMMAND_SIZE, GFP_KERNEL);
- sense_buf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
- if (unlikely(!cmd_buf || !scsi_cmd || !sense_buf)) {
+ if (unlikely(!cmd_buf || !scsi_cmd)) {
rc = -ENOMEM;
goto out;
}
@@ -349,7 +347,7 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
/* Drop the ioctl read semahpore across lengthy call */
up_read(&cfg->ioctl_rwsem);
result = scsi_execute(sdev, scsi_cmd, DMA_FROM_DEVICE, cmd_buf,
- CMD_BUFSIZE, sense_buf, &sshdr, to, CMD_RETRIES,
+ CMD_BUFSIZE, NULL, &sshdr, to, CMD_RETRIES,
0, 0, NULL);
down_read(&cfg->ioctl_rwsem);
rc = check_state(cfg);
@@ -380,7 +378,6 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
if (retry_cnt++ < 1) {
kfree(cmd_buf);
kfree(scsi_cmd);
- kfree(sense_buf);
goto retry;
}
}
@@ -411,7 +408,6 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
out:
kfree(cmd_buf);
kfree(scsi_cmd);
- kfree(sense_buf);

dev_dbg(dev, "%s: maxlba=%lld blklen=%d rc=%d\n",
__func__, gli->max_lba, gli->blk_len, rc);
diff --git a/drivers/scsi/cxlflash/vlun.c b/drivers/scsi/cxlflash/vlun.c
index 5deef57a7834..e7e9b2f2ad21 100644
--- a/drivers/scsi/cxlflash/vlun.c
+++ b/drivers/scsi/cxlflash/vlun.c
@@ -425,7 +425,6 @@ static int write_same16(struct scsi_device *sdev,
{
u8 *cmd_buf = NULL;
u8 *scsi_cmd = NULL;
- u8 *sense_buf = NULL;
int rc = 0;
int result = 0;
u64 offset = lba;
@@ -439,8 +438,7 @@ static int write_same16(struct scsi_device *sdev,

cmd_buf = kzalloc(CMD_BUFSIZE, GFP_KERNEL);
scsi_cmd = kzalloc(MAX_COMMAND_SIZE, GFP_KERNEL);
- sense_buf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
- if (unlikely(!cmd_buf || !scsi_cmd || !sense_buf)) {
+ if (unlikely(!cmd_buf || !scsi_cmd)) {
rc = -ENOMEM;
goto out;
}
@@ -456,7 +454,7 @@ static int write_same16(struct scsi_device *sdev,
/* Drop the ioctl read semahpore across lengthy call */
up_read(&cfg->ioctl_rwsem);
result = scsi_execute(sdev, scsi_cmd, DMA_TO_DEVICE, cmd_buf,
- CMD_BUFSIZE, sense_buf, NULL, to,
+ CMD_BUFSIZE, NULL, NULL, to,
CMD_RETRIES, 0, 0, NULL);
down_read(&cfg->ioctl_rwsem);
rc = check_state(cfg);
@@ -481,7 +479,6 @@ static int write_same16(struct scsi_device *sdev,
out:
kfree(cmd_buf);
kfree(scsi_cmd);
- kfree(sense_buf);
dev_dbg(dev, "%s: returning rc=%d\n", __func__, rc);
return rc;
}
--
2.17.1


2018-07-31 19:56:10

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 3/9] scsi: build scsi_common.o for all scsi passthrough request users

From: Christoph Hellwig <[email protected]>

Split scsi_common.o out of SCSI so that non-SCSI users can pull it in
easily for future sense buffer helper usage.

Signed-off-by: Christoph Hellwig <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
drivers/Makefile | 2 +-
drivers/scsi/Makefile | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd47014657..a6abd7a856c6 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -76,7 +76,7 @@ obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
obj-$(CONFIG_NUBUS) += nubus/
obj-y += macintosh/
obj-$(CONFIG_IDE) += ide/
-obj-$(CONFIG_SCSI) += scsi/
+obj-y += scsi/
obj-y += nvme/
obj-$(CONFIG_ATA) += ata/
obj-$(CONFIG_TARGET_CORE) += target/
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index e29f9b8fd66d..1f6218b98430 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -21,6 +21,7 @@ CFLAGS_gdth.o = # -DDEBUG_GDTH=2 -D__SERIAL__ -D__COM2__ -DGDTH_STATISTICS
obj-$(CONFIG_PCMCIA) += pcmcia/

obj-$(CONFIG_SCSI) += scsi_mod.o
+obj-$(CONFIG_BLK_SCSI_REQUEST) += scsi_common.o

obj-$(CONFIG_RAID_ATTRS) += raid_class.o

@@ -155,7 +156,6 @@ obj-$(CONFIG_SCSI_HISI_SAS) += hisi_sas/
obj-$(CONFIG_SCSI_DEBUG) += scsi_debug.o
scsi_mod-y += scsi.o hosts.o scsi_ioctl.o \
scsicam.o scsi_error.o scsi_lib.o
-scsi_mod-y += scsi_common.o
scsi_mod-$(CONFIG_SCSI_CONSTANTS) += constants.o
scsi_mod-$(CONFIG_SCSI_DMA) += scsi_lib_dma.o
scsi_mod-y += scsi_scan.o scsi_sysfs.o scsi_devinfo.o
--
2.17.1


2018-07-31 19:56:21

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2 4/9] target: don't depend on SCSI

From: Christoph Hellwig <[email protected]>

The core target code only needs code from scsi_common.c, which is now
separately selectable.

Signed-off-by: Christoph Hellwig <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
drivers/target/Kconfig | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
index 4c44d7bed01a..cb6f32ce7de8 100644
--- a/drivers/target/Kconfig
+++ b/drivers/target/Kconfig
@@ -1,10 +1,10 @@

menuconfig TARGET_CORE
tristate "Generic Target Core Mod (TCM) and ConfigFS Infrastructure"
- depends on SCSI && BLOCK
+ depends on BLOCK
select CONFIGFS_FS
select CRC_T10DIF
- select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
+ select BLK_SCSI_REQUEST
select SGL_ALLOC
default n
help
@@ -29,6 +29,7 @@ config TCM_FILEIO

config TCM_PSCSI
tristate "TCM/pSCSI Subsystem Plugin for Linux/SCSI"
+ depends on SCSI
help
Say Y here to enable the TCM/pSCSI subsystem plugin for non-buffered
passthrough access to Linux/SCSI device
--
2.17.1


2018-07-31 20:02:55

by Bart Van Assche

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] scsi: build scsi_common.o for all scsi passthrough request users

On Tue, 2018-07-31 at 12:51 -0700, Kees Cook wrote:
+AD4- diff --git a/drivers/Makefile b/drivers/Makefile
+AD4- index 24cd47014657..a6abd7a856c6 100644
+AD4- --- a/drivers/Makefile
+AD4- +-+-+- b/drivers/Makefile
+AD4- +AEAAQA- -76,7 +-76,7 +AEAAQA- obj-+ACQ-(CONFIG+AF8-DMA+AF8-SHARED+AF8-BUFFER) +-+AD0- dma-buf/
+AD4- obj-+ACQ-(CONFIG+AF8-NUBUS) +-+AD0- nubus/
+AD4- obj-y +-+AD0- macintosh/
+AD4- obj-+ACQ-(CONFIG+AF8-IDE) +-+AD0- ide/
+AD4- -obj-+ACQ-(CONFIG+AF8-SCSI) +-+AD0- scsi/
+AD4- +-obj-y +-+AD0- scsi/
+AD4- obj-y +-+AD0- nvme/
+AD4- obj-+ACQ-(CONFIG+AF8-ATA) +-+AD0- ata/
+AD4- obj-+ACQ-(CONFIG+AF8-TARGET+AF8-CORE) +-+AD0- target/

The above change not only selects scsi+AF8-common.o but also the following object
files: scsi.o hosts.o scsi+AF8-ioctl.o scsicam.o scsi+AF8-error.o scsi+AF8-lib.o scsi+AF8-scan.o
scsi+AF8-sysfs.o scsi+AF8-devinfo.o scsi+AF8-trace.o scsi+AF8-logging.o. That is a change that
has not been mentioned in the patch description. That makes me wonder whether
this is an intended change? Wouldn't a cleaner solution have been to move
scsi+AF8-common.c into a new directory?

Bart.


2018-07-31 20:15:41

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] scsi: build scsi_common.o for all scsi passthrough request users

On Tue, Jul 31, 2018 at 08:01:16PM +0000, Bart Van Assche wrote:
> On Tue, 2018-07-31 at 12:51 -0700, Kees Cook wrote:
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 24cd47014657..a6abd7a856c6 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -76,7 +76,7 @@ obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
> > obj-$(CONFIG_NUBUS) += nubus/
> > obj-y += macintosh/
> > obj-$(CONFIG_IDE) += ide/
> > -obj-$(CONFIG_SCSI) += scsi/
> > +obj-y += scsi/
> > obj-y += nvme/
> > obj-$(CONFIG_ATA) += ata/
> > obj-$(CONFIG_TARGET_CORE) += target/
>
> The above change not only selects scsi_common.o but also the following object
> files: scsi.o hosts.o scsi_ioctl.o scsicam.o scsi_error.o scsi_lib.o scsi_scan.o
> scsi_sysfs.o scsi_devinfo.o scsi_trace.o scsi_logging.o. That is a change that
> has not been mentioned in the patch description.

It shouldn't. All these are built into scsi_mod.o, which is only built
when CONFIG_SCSI is set. Under what circumstances do you see them built?

2018-07-31 20:19:58

by Bart Van Assche

[permalink] [raw]
Subject: Re: [PATCH v2 3/9] scsi: build scsi_common.o for all scsi passthrough request users

On Tue, 2018-07-31 at 13:12 -0700, hch+AEA-infradead.org wrote:
+AD4- It shouldn't. All these are built into scsi+AF8-mod.o, which is only built
+AD4- when CONFIG+AF8-SCSI is set. Under what circumstances do you see them built?

Although I think you are right, I still prefer that the scsi+AF8-common.c file
would be moved to a new directory. That will prevent confusion later on for
people who want to add additional code. This patch makes it nontrivial to
figure out which code is built when SCSI target functionality is enabled but
SCSI initiator functionality is not selected. I think moving scsi+AF8-common.c
into a new directory would make it much easier to figure out which code is
built depending on the kernel configuration.

Bart.


2018-07-31 21:32:08

by David Miller

[permalink] [raw]
Subject: Re: [PATCH v2 1/9] ide-cd: Drop unused sense buffers

From: Kees Cook <[email protected]>
Date: Tue, 31 Jul 2018 12:51:46 -0700

> This drops unused sense buffers from:
>
> cdrom_eject()
> cdrom_read_capacity()
> cdrom_read_tocentry()
> ide_cd_lockdoor()
> ide_cd_read_toc()
>
> Signed-off-by: Kees Cook <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>

Acked-by: David S. Miller <[email protected]>

2018-07-31 21:32:08

by David Miller

[permalink] [raw]
Subject: Re: [PATCH v2 6/9] ide-cd: Remove redundant sense buffer

From: Kees Cook <[email protected]>
Date: Tue, 31 Jul 2018 12:51:51 -0700

> This is already able to process the sense buffer, so remove the redundant
> parsing during the failure path. This also fixes any possible stale values
> since the prior code did not check the sense length.
>
> Signed-off-by: Kees Cook <[email protected]>

Acked-by: David S. Miller <[email protected]>

2018-08-01 08:23:53

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 5/9] block: Switch struct packet_command to use struct scsi_sense_hdr

On Tue, Jul 31, 2018 at 12:51:50PM -0700, Kees Cook wrote:
> There is a lot of needless struct request_sense usage in the CDROM
> code. These can all be struct scsi_sense_hdr instead, to avoid any
> confusion over their respective structure sizes. This patch is a lot
> of noise changing "sense" to "sshdr", but the final code is more
> readable to distinguish between "sense" meaning "struct request_sense"
> and "sshdr" meaning "struct scsi_sense_hdr".

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2018-08-01 08:24:52

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 6/9] ide-cd: Remove redundant sense buffer

On Tue, Jul 31, 2018 at 12:51:51PM -0700, Kees Cook wrote:
> This is already able to process the sense buffer, so remove the redundant
> parsing during the failure path. This also fixes any possible stale values
> since the prior code did not check the sense length.
>
> Signed-off-by: Kees Cook <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2018-08-01 08:26:01

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 7/9] cdrom: Use struct scsi_sense_hdr internally

On Tue, Jul 31, 2018 at 12:51:52PM -0700, Kees Cook wrote:
> This removes more casts of struct request_sense and uses the standard
> struct scsi_sense_hdr instead. This also fixes any possible stale values
> since the prior code did not check the sense length.
>
> Signed-off-by: Kees Cook <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2018-08-01 08:26:23

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] libata-scsi: Move sense buffers onto stack

On Tue, Jul 31, 2018 at 12:51:53PM -0700, Kees Cook wrote:
> To support future compile-time sizeof() checks that will be able to
> validate the length of sense buffers, this removes the only dynamically
> allocated sense buffers in the tree by putting the 96 byte sense buffers
> on the stack.
>
> Signed-off-by: Kees Cook <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2018-08-01 08:27:20

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 9/9] scsi: Check sense buffer size at build time

On Tue, Jul 31, 2018 at 12:51:54PM -0700, Kees Cook wrote:
> To avoid introducing problems like those fixed in commit f7068114d45e
> ("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
> wrapper for scsi_execute() that verifies the size of the sense buffer
> similar to what was done for command string sizes in commit 3756f6401c30
> ("exec: avoid gcc-8 warning for get_task_comm").
>
> Another solution could be to add a length argument to scsi_execute(),
> but this function already takes a lot of arguments and Jens was not fond
> of that approach.
>
> Additionally, this moves the SCSI_SENSE_BUFFERSIZE definition into
> scsi_device.h, and removes a redundant include for scsi_device.h from
> scsi_cmnd.h.
>
> Signed-off-by: Kees Cook <[email protected]>

Looks good,

Reviewed-by: Christoph Hellwig <[email protected]>

2018-08-01 15:46:39

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH v2 8/9] libata-scsi: Move sense buffers onto stack

On Tue, Jul 31, 2018 at 12:51:53PM -0700, Kees Cook wrote:
> To support future compile-time sizeof() checks that will be able to
> validate the length of sense buffers, this removes the only dynamically
> allocated sense buffers in the tree by putting the 96 byte sense buffers
> on the stack.

Acked-by: Tejun Heo <[email protected]>

Thanks.

--
tejun

2018-08-02 20:23:52

by Martin K. Petersen

[permalink] [raw]
Subject: Re: [PATCH v2 0/9] block: Consolidate scsi sense buffer usage


Kees,

> This is a follow-up to commit f7068114d45e ("sr: pass down correctly
> sized SCSI sense buffer") which further cleans up and removes needless
> sense character array buffers and "struct request_sense" usage in favor
> of the common "struct scsi_sense_hdr".

This looks good to me.

Acked-by: Martin K. Petersen <[email protected]>

--
Martin K. Petersen Oracle Linux Engineering

2018-08-02 21:26:40

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH v2 0/9] block: Consolidate scsi sense buffer usage

On 7/31/18 1:51 PM, Kees Cook wrote:
> This is a follow-up to commit f7068114d45e ("sr: pass down correctly
> sized SCSI sense buffer") which further cleans up and removes needless
> sense character array buffers and "struct request_sense" usage in favor
> of the common "struct scsi_sense_hdr".
>
> First, drop a bunch of unused sense buffers:
> [PATCH 1/9] ide-cd: Drop unused sense buffers
> [PATCH 2/9] scsi: cxlflash: Drop unused sense buffers
>
> Next, allow the sense buffer to be usable outside SCSI tree:
> [PATCH 3/9] scsi: build scsi_common.o for all scsi passthrough request users
> [PATCH 4/9] target: don't depend on SCSI
>
> Then move all request_sense usage to scsi_sense_hdr:
> [PATCH 5/9] block: Switch struct packet_command to use struct scsi_sense_hdr
>
> And do some further cleanups for scsi_sense_hdr now that we can:
> [PATCH 6/9] ide-cd: Remove redundant sense buffer
> [PATCH 7/9] cdrom: Use struct scsi_sense_hdr internally
>
> Finally add a build-time check to make sure we don't pass bad buffer sizes:
> [PATCH 8/9] libata-scsi: Move sense buffers onto stack
> [PATCH 9/9] scsi: Check sense buffer size at build time

Thanks Kees, applied for 4.19. Note that I hand applied patch #5 and #9,
since they did not apply directly to the block tree.

--
Jens Axboe


2018-08-07 00:02:22

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] target: don't depend on SCSI

On 07/31/2018 12:51 PM, Kees Cook wrote:
> From: Christoph Hellwig <[email protected]>
>
> The core target code only needs code from scsi_common.c, which is now
> separately selectable.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> drivers/target/Kconfig | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
> index 4c44d7bed01a..cb6f32ce7de8 100644
> --- a/drivers/target/Kconfig
> +++ b/drivers/target/Kconfig
> @@ -1,10 +1,10 @@
>
> menuconfig TARGET_CORE
> tristate "Generic Target Core Mod (TCM) and ConfigFS Infrastructure"
> - depends on SCSI && BLOCK
> + depends on BLOCK
> select CONFIGFS_FS
> select CRC_T10DIF
> - select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
> + select BLK_SCSI_REQUEST
> select SGL_ALLOC
> default n
> help
> @@ -29,6 +29,7 @@ config TCM_FILEIO
>
> config TCM_PSCSI
> tristate "TCM/pSCSI Subsystem Plugin for Linux/SCSI"
> + depends on SCSI
> help
> Say Y here to enable the TCM/pSCSI subsystem plugin for non-buffered
> passthrough access to Linux/SCSI device
>

Hi,

This patch causes build errors in linux-next-20180806 when SCSI=m and
LOOPBACK_TARGET=y.

drivers/target/loopback/tcm_loop.o: In function `tcm_loop_port_link':
tcm_loop.c:(.text+0x445): undefined reference to `scsi_add_device'
drivers/target/loopback/tcm_loop.o: In function `tcm_loop_driver_remove':
tcm_loop.c:(.text+0x55c): undefined reference to `scsi_remove_host'
tcm_loop.c:(.text+0x564): undefined reference to `scsi_host_put'
drivers/target/loopback/tcm_loop.o: In function `tcm_loop_submission_work':
tcm_loop.c:(.text+0x7c4): undefined reference to `scmd_printk'
drivers/target/loopback/tcm_loop.o: In function `tcm_loop_driver_probe':
tcm_loop.c:(.text+0x7fb): undefined reference to `scsi_host_alloc'
tcm_loop.c:(.text+0x85b): undefined reference to `scsi_add_host_with_dma'
tcm_loop.c:(.text+0x896): undefined reference to `scsi_host_put'
drivers/target/loopback/tcm_loop.o: In function `tcm_loop_port_unlink':
tcm_loop.c:(.text+0x962): undefined reference to `scsi_device_lookup'
tcm_loop.c:(.text+0x972): undefined reference to `scsi_remove_device'
tcm_loop.c:(.text+0x97a): undefined reference to `scsi_device_put'
drivers/target/loopback/tcm_loop.o:(.data+0x210): undefined reference to `scsi_change_queue_depth'


--
~Randy

2018-08-07 00:53:51

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] target: don't depend on SCSI

On 08/06/2018 04:59 PM, Kees Cook wrote:
> On Mon, Aug 6, 2018 at 4:38 PM, Randy Dunlap <[email protected]> wrote:
>> On 07/31/2018 12:51 PM, Kees Cook wrote:
>>> From: Christoph Hellwig <[email protected]>
>>>
>>> The core target code only needs code from scsi_common.c, which is now
>>> separately selectable.
>>>
>>> Signed-off-by: Christoph Hellwig <[email protected]>
>>> Signed-off-by: Kees Cook <[email protected]>
>>> ---
>>> drivers/target/Kconfig | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
>>> index 4c44d7bed01a..cb6f32ce7de8 100644
>>> --- a/drivers/target/Kconfig
>>> +++ b/drivers/target/Kconfig
>>> @@ -1,10 +1,10 @@
>>>
>>> menuconfig TARGET_CORE
>>> tristate "Generic Target Core Mod (TCM) and ConfigFS Infrastructure"
>>> - depends on SCSI && BLOCK
>>> + depends on BLOCK
>>> select CONFIGFS_FS
>>> select CRC_T10DIF
>>> - select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
>>> + select BLK_SCSI_REQUEST
>>> select SGL_ALLOC
>>> default n
>>> help
>>> @@ -29,6 +29,7 @@ config TCM_FILEIO
>>>
>>> config TCM_PSCSI
>>> tristate "TCM/pSCSI Subsystem Plugin for Linux/SCSI"
>>> + depends on SCSI
>>> help
>>> Say Y here to enable the TCM/pSCSI subsystem plugin for non-buffered
>>> passthrough access to Linux/SCSI device
>>>
>>
>> Hi,
>>
>> This patch causes build errors in linux-next-20180806 when SCSI=m and
>> LOOPBACK_TARGET=y.
>>
>> drivers/target/loopback/tcm_loop.o: In function `tcm_loop_port_link':
>> tcm_loop.c:(.text+0x445): undefined reference to `scsi_add_device'
>> drivers/target/loopback/tcm_loop.o: In function `tcm_loop_driver_remove':
>> tcm_loop.c:(.text+0x55c): undefined reference to `scsi_remove_host'
>> tcm_loop.c:(.text+0x564): undefined reference to `scsi_host_put'
>> drivers/target/loopback/tcm_loop.o: In function `tcm_loop_submission_work':
>> tcm_loop.c:(.text+0x7c4): undefined reference to `scmd_printk'
>> drivers/target/loopback/tcm_loop.o: In function `tcm_loop_driver_probe':
>> tcm_loop.c:(.text+0x7fb): undefined reference to `scsi_host_alloc'
>> tcm_loop.c:(.text+0x85b): undefined reference to `scsi_add_host_with_dma'
>> tcm_loop.c:(.text+0x896): undefined reference to `scsi_host_put'
>> drivers/target/loopback/tcm_loop.o: In function `tcm_loop_port_unlink':
>> tcm_loop.c:(.text+0x962): undefined reference to `scsi_device_lookup'
>> tcm_loop.c:(.text+0x972): undefined reference to `scsi_remove_device'
>> tcm_loop.c:(.text+0x97a): undefined reference to `scsi_device_put'
>> drivers/target/loopback/tcm_loop.o:(.data+0x210): undefined reference to `scsi_change_queue_depth'
>
> Can you send your .config? I'm struggling to get a build with SCSI=m. :P

Sure, it's attached.


> I wonder if LOOPBACK_TARGET is just missing a "depends on SCSI" as was
> added for TCM_PSCSI?

possibly.


--
~Randy


Attachments:
config-r1153 (158.20 kB)

2018-08-07 05:41:22

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] target: don't depend on SCSI

On Mon, Aug 06, 2018 at 04:59:04PM -0700, Kees Cook wrote:
> I wonder if LOOPBACK_TARGET is just missing a "depends on SCSI" as was
> added for TCM_PSCSI?

That is eactly the case. LOOPBACK_TARGET is a scsi driver that
feeds commands into the target layer, so it needs a depends on SCSI.

Sorry for missing that earlier.