On 7/27/19 5:37 AM, Douglas Gilbert wrote:
> The biggest single item in the sg_request object is the sense
> buffer array which is SCSI_SENSE_BUFFERSIZE bytes long. That
> constant started out at 18 bytes 20 years ago and is 96 bytes
> now and might grow in the future. On the other hand the sense
> buffer is only used by a small number of SCSI commands: those
> that fail and those that want to return more information
> other than a SCSI status of GOOD.
>
> Allocate sense buffer as needed on the heap and delete as soon as
> possible.
>
> Signed-off-by: Douglas Gilbert <dgilb...@interlog.com>
> ---
> drivers/scsi/sg.c | 47 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 00b8553e0038..4d6635af7da7 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -176,7 +176,6 @@ struct sg_request { /* active SCSI command or
> inactive on free list (fl) */
> spinlock_t req_lck;
> struct sg_scatter_hold sgat_h; /* hold buffer, perhaps scatter list */
> struct sg_slice_hdr3 s_hdr3; /* subset of sg_io_hdr */
> - u8 sense_b[SCSI_SENSE_BUFFERSIZE];
> u32 duration; /* cmd duration in milliseconds */
> u32 rq_flags; /* hold user supplied flags */
> u32 rq_info; /* info supplied by v3 and v4 interfaces */
> @@ -188,6 +187,7 @@ struct sg_request { /* active SCSI command or
> inactive on free list (fl) */
> u8 cmd_opcode; /* first byte of SCSI cdb */
> u64 start_ns; /* starting point of command duration calc */
> unsigned long frq_bm[1]; /* see SG_FRQ_* defines above */
> + u8 *sense_bp; /* alloc-ed sense buffer, as needed */
> struct sg_fd *parentfp; /* pointer to owning fd, even when on fl */
> struct request *rq; /* released in sg_rq_end_io(), bio kept */
> struct bio *bio; /* kept until this req -->SG_RS_INACTIVE */
> @@ -845,18 +845,21 @@ sg_copy_sense(struct sg_request *srp)
> (driver_byte(srp->rq_result) & DRIVER_SENSE)) {
> int sb_len = min_t(int, SCSI_SENSE_BUFFERSIZE, srp->sense_len);
> int mx_sb_len;
> + u8 *sbp = srp->sense_bp;
> void __user *up;
>
> + srp->sense_bp = NULL;
> up = (void __user *)srp->s_hdr3.sbp;
> mx_sb_len = srp->s_hdr3.mx_sb_len;
> - if (up && mx_sb_len > 0) {
> + if (up && mx_sb_len > 0 && sbp) {
> sb_len = min_t(int, sb_len, mx_sb_len);
> /* Additional sense length field */
> - sb_len_wr = 8 + (int)srp->sense_b[7];
> + sb_len_wr = 8 + (int)sbp[7];
> sb_len_wr = min_t(int, sb_len, sb_len_wr);
> - if (copy_to_user(up, srp->sense_b, sb_len_wr))
> + if (copy_to_user(up, sbp, sb_len_wr))
> sb_len_wr = -EFAULT;
> }
> + kfree(sbp);
> }
> return sb_len_wr;
> }
> @@ -963,8 +966,9 @@ sg_rd_v1v2(void __user *buf, int count, struct sg_fd *sfp,
> h2p->driver_status = driver_byte(rq_result);
> if ((CHECK_CONDITION & status_byte(rq_result)) ||
> (DRIVER_SENSE & driver_byte(rq_result))) {
> - memcpy(h2p->sense_buffer, srp->sense_b,
> - sizeof(h2p->sense_buffer));
> + if (srp->sense_bp)
> + memcpy(h2p->sense_buffer, srp->sense_bp,
> + sizeof(h2p->sense_buffer));
> }
> switch (host_byte(rq_result)) {
> /*
> @@ -999,17 +1003,21 @@ sg_rd_v1v2(void __user *buf, int count, struct sg_fd
> *sfp,
>
> /* Now copy the result back to the user buffer. */
> if (count >= SZ_SG_HEADER) {
> - if (copy_to_user(buf, h2p, SZ_SG_HEADER))
> - return -EFAULT;
> + if (copy_to_user(buf, h2p, SZ_SG_HEADER)) {
> + res = -EFAULT;
> + goto fini;
> + }
> buf += SZ_SG_HEADER;
> if (count > h2p->reply_len)
> count = h2p->reply_len;
> if (count > SZ_SG_HEADER) {
> - if (sg_rd_append(srp, buf, count - SZ_SG_HEADER))
> - return -EFAULT;
> + res = sg_rd_append(srp, buf, count - SZ_SG_HEADER);
> + if (res)
> + goto fini;
> }
> } else
> res = (h2p->result == 0) ? 0 : -EIO;
> +fini:
> atomic_set(&srp->rq_st, SG_RS_DONE_RD);
> sg_finish_scsi_blk_rq(srp);
> sg_deact_request(sfp, srp);
> @@ -1977,8 +1985,17 @@ sg_rq_end_io(struct request *rq, blk_status_t status)
> srp->duration = sg_calc_rq_dur(srp);
> if (unlikely((srp->rq_result & SG_ML_RESULT_MSK) && slen > 0))
> sg_check_sense(sdp, srp, slen);
> - if (slen > 0)
> - memcpy(srp->sense_b, scsi_rp->sense, slen);
> + if (slen > 0) {
> + if (scsi_rp->sense) {
> + srp->sense_bp = kzalloc(SCSI_SENSE_BUFFERSIZE,
> + GFP_ATOMIC);
> + if (srp->sense_bp)
> + memcpy(srp->sense_bp, scsi_rp->sense, slen);
> + } else {
> + pr_warn("%s: scsi_request::sense==NULL\n", __func__);
> + slen = 0;
> + }
> + }
> srp->sense_len = slen;
> if (unlikely(test_bit(SG_FRQ_IS_ORPHAN, srp->frq_bm))) {
> spin_lock(&srp->req_lck);
> @@ -2940,6 +2957,7 @@ sg_deact_request(struct sg_fd *sfp, struct sg_request
> *srp)
> bool on_fl = false;
> int dlen, buflen;
> unsigned long iflags;
> + u8 *sbp;
> struct sg_request *t_srp;
> struct sg_scatter_hold *schp;
> const char *cp = "head";
> @@ -2948,8 +2966,11 @@ sg_deact_request(struct sg_fd *sfp, struct sg_request
> *srp)
> return;
> schp = &srp->sgat_h; /* make sure it is own data buffer */
> spin_lock_irqsave(&sfp->rq_list_lock, iflags);
> + sbp = srp->sense_bp;
> + srp->sense_bp = NULL;
> atomic_set(&srp->rq_st, SG_RS_BUSY);
> list_del_rcu(&srp->rq_entry);
> + kfree(sbp); /* maybe orphaned req, thus never read */
> /*
> * N.B. sg_request object is not de-allocated (freed). The contents
> * of the rq_list and rq_fl lists are de-allocated (freed) when
> @@ -3092,6 +3113,7 @@ sg_remove_sfp_usercontext(struct work_struct *work)
> list_del(&srp->rq_entry);
> if (srp->sgat_h.buflen > 0)
> sg_remove_sgat(srp);
> + kfree(srp->sense_bp); /* abnormal close: device detached */
> SG_LOG(6, sfp, "%s:%s%p --\n", __func__, cp, srp);
> kfree(srp);
> }
> @@ -3103,6 +3125,7 @@ sg_remove_sfp_usercontext(struct work_struct *work)
> list_del(&srp->fl_entry);
> if (srp->sgat_h.buflen > 0)
> sg_remove_sgat(srp);
> + kfree(srp->sense_bp);
> SG_LOG(6, sfp, "%s: fl%s%p --\n", __func__, cp, srp);
> kfree(srp);
> }
>
Maybe it's worthwhile using a mempool here for sense buffers; frequest
kmalloc()/kfree() really should be avoided.
Also the allocation at end_io time is slightly dodgy; I'd rather have
the sense allocated before setting up the command. Thing is, the end_io
callback might be called at any time and in about every context
(including from an interrupt handler), so I really would avoid having to
do kmalloc() there.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
h...@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
