bsg-lib interface cleanup V2
Subsystems (5)
Hi all,
this series cleans up various abuses of the bsg interfaces, and then splits bsg for SCSI passthrough from bsg for arbitrary transport passthrough. This removes the scsi_request abuse in bsg-lib that is very confusing, and also makes sure we can sanity check the requests we get. The current code will happily execute scsi commands against bsg-lib queues, and transport pass through against scsi nodes, without any indication to the driver that we are doing the wrong thing.
Changes since V1:
- dropped various patches merged already
- use job to get the payload length in bsg_transport_complete_rq
- renamed the ptr64 macro to uptr64
- make bsg_scsi_complete_rq and bsg_transport_complete_rq look more similar
- trivial cleanup in bsg_map_hdr
PATCH 1/3 bsg-lib: introduce a timeout field in struct bsg_job
The zfcp driver wants to know the timeout for a bsg job, so add a field to struct bsg_job for it in preparation of not exposing the request to the bsg-lib users.
Signed-off-by Christoph Hellwig <[email protected]>
Reviewed-by Benjamin Block <[email protected]>
Reviewed-by Hannes Reinecke <[email protected]>
Reviewed-by Johannes Thumshirn <[email protected]>
--- block/bsg-lib.c | 1 + drivers/s390/scsi/zfcp_fc.c | 4 ++-- include/linux/bsg-lib.h | 2 ++ 3 files changed, 5 insertions(+), 2 deletions(-)
block/bsg-lib.c +1 -0
@@ -132,6 +132,7 @@ static int bsg_prepare_job(struct device *dev, struct request *req)struct bsg_job *job =blk_mq_rq_to_pdu (req);int ret; + job->timeout = req->timeout; job->request = rq->cmd; job->request_len = rq->cmd_len;
drivers/s390/scsi/zfcp_fc.c +2 -2
@@ -961,7 +961,7 @@ static int zfcp_fc_exec_els_job(struct bsg_job *job, d_id =ntoh24 (bsg_request->rqst_data.h_els.port_id); els->handler = zfcp_fc_ct_els_job_handler; -return zfcp_fsf_send_els (adapter, d_id, els, job->req->timeout /HZ ); +return zfcp_fsf_send_els (adapter, d_id, els, job->timeout /HZ ); } static int zfcp_fc_exec_ct_job(struct bsg_job *job,
@@ -980,7 +980,7 @@ static int zfcp_fc_exec_ct_job(struct bsg_job *job,return ret; ct->handler = zfcp_fc_ct_job_handler; - ret =zfcp_fsf_send_ct (wka_port, ct,NULL , job->req->timeout /HZ ); + ret =zfcp_fsf_send_ct (wka_port, ct,NULL , job->timeout /HZ );if (ret)zfcp_fc_wka_port_put (wka_port);
include/linux/bsg-lib.h +2 -0
@@ -44,6 +44,8 @@ struct bsg_job {struct kref kref; +unsigned int timeout; +/* Transport/driver specific request/reply structs */ void *request;void *reply;
Signature · 1 line
2.14.2
PATCH 2/3 bsg-lib: remove bsg_job.req
In reply to Christoph HellwigUsers of the bsg-lib interface should only use the bsg_job data structure and not know about implementation details of it.
Signed-off-by Christoph Hellwig <[email protected]>
Reviewed-by Benjamin Block <[email protected]>
Reviewed-by Hannes Reinecke <[email protected]>
Reviewed-by Johannes Thumshirn <[email protected]>
--- block/bsg-lib.c | 14 ++++++-------- include/linux/bsg-lib.h | 1 - 2 files changed, 6 insertions(+), 9 deletions(-)
block/bsg-lib.c +6 -8
@@ -35,7 +35,7 @@static void bsg_teardown_job (struct kref *kref) { struct bsg_job *job = container_of(kref, struct bsg_job, kref); -struct request *rq = job->req; +struct request *rq =blk_mq_rq_from_pdu (job);put_device (job->dev);/* release reference for the request */
@@ -68,19 +68,18 @@ EXPORT_SYMBOL_GPL(bsg_job_get);void bsg_job_done (struct bsg_job *job,int result,unsigned int reply_payload_rcv_len) { -struct request *req = job->req; +struct request *req =blk_mq_rq_from_pdu (job);struct request *rsp = req->next_rq; -struct scsi_request *rq =scsi_req (req);int err; - err =scsi_req (job->req)->result = result; + err = job->sreq.result = result;if (err <0 )/* we're only returning the result field in the reply */ - rq->sense_len =sizeof (u32); + job->sreq.sense_len =sizeof (u32);else - rq->sense_len = job->reply_len; + job->sreq.sense_len = job->reply_len;/* we assume all request payload was transferred, residual == 0 */ - rq->resid_len =0 ; + job->sreq.resid_len =0 ;if (rsp) {WARN_ON (reply_payload_rcv_len >scsi_req (rsp)->resid_len);
@@ -232,7 +231,6 @@ static void bsg_initialize_rq(struct request *req) sreq->sense = sense; sreq->sense_len =SCSI_SENSE_BUFFERSIZE ; - job->req = req; job->reply = sense; job->reply_len = sreq->sense_len; job->dd_data = job +1 ;
include/linux/bsg-lib.h +0 -1
@@ -40,7 +40,6 @@ struct bsg_buffer {struct bsg_job {struct scsi_request sreq;struct device *dev; -struct request *req;struct kref kref;
Signature · 1 line
2.14.2
PATCH 3/3 bsg: split handling of SCSI CDBs vs transport requeues
In reply to Christoph HellwigThe current BSG design tries to shoe-horn the transport-specific passthrough commands into the overall framework for SCSI passthrough requests. This has a couple problems:
- each passthrough queue has to set the QUEUE_FLAG_SCSI_PASSTHROUGH flag despite not dealing with SCSI commands at all. Because of that these queues could also incorrectly accept SCSI commands from in-kernel users or through the legacy SCSI_IOCTL_SEND_COMMAND ioctl.
- the real SCSI bsg queues also incorrectly accept bsg requests of the BSG_SUB_PROTOCOL_SCSI_TRANSPORT type
- the bsg transport code is almost unredable because it tries to reuse different SCSI concepts for its own purpose.
This patch instead adds a new bsg_ops structure to handle the two cases differently, and thus solves all of the above problems. Another side effect is that the bsg-lib queues also don't need to embedd a struct scsi_request anymore.
Signed-off-by Christoph Hellwig <[email protected]>
Reviewed-by Hannes Reinecke <[email protected]>
Reviewed-by Johannes Thumshirn <[email protected]>
--- block/bsg-lib.c | 158 +++++++++++++++-------- block/bsg.c | 262 +++++++++++++++++--------------------- drivers/scsi/scsi_lib.c | 4 +- drivers/scsi/scsi_sysfs.c | 3 +- drivers/scsi/scsi_transport_sas.c | 1 - include/linux/bsg-lib.h | 4 +- include/linux/bsg.h | 35 +++-- 7 files changed, 250 insertions(+), 217 deletions(-)
block/bsg-lib.c +105 -53
@@ -27,6 +27,94 @@#include <linux/bsg-lib.h> #include <linux/export.h> #include <scsi/scsi_cmnd.h> +#include <scsi/sg.h> + +#define uptr64 (val) ((void __user *)(uintptr_t)(val)) + +static int bsg_transport_check_proto (struct sg_io_v4 *hdr) +{ +if (hdr->protocol !=BSG_PROTOCOL_SCSI || + hdr->subprotocol !=BSG_SUB_PROTOCOL_SCSI_TRANSPORT ) +return -EINVAL ; +if (!capable (CAP_SYS_RAWIO )) +return -EPERM ; +return 0 ; +} + +static int bsg_transport_fill_hdr (struct request *rq,struct sg_io_v4 *hdr, +fmode_t mode) +{ +struct bsg_job *job =blk_mq_rq_to_pdu (rq); + + job->request_len = hdr->request_len; + job->request =memdup_user (uptr64 (hdr->request), hdr->request_len); +if (IS_ERR (job->request)) +return PTR_ERR (job->request); +return 0 ; +} + +static int bsg_transport_complete_rq (struct request *rq,struct sg_io_v4 *hdr) +{ +struct bsg_job *job =blk_mq_rq_to_pdu (rq); +int ret =0 ; + +/* +* The assignments below don't make much sense, but are kept for +* bug by bug backwards compatibility: +*/ + hdr->device_status = job->result &0xff ; + hdr->transport_status =host_byte (job->result); + hdr->driver_status =driver_byte (job->result); + hdr->info =0 ; +if (hdr->device_status || hdr->transport_status || hdr->driver_status) + hdr->info |=SG_INFO_CHECK ; + hdr->response_len =0 ; + +if (job->result <0 ) { +/* we're only returning the result field in the reply */ + job->reply_len =sizeof (u32); + ret = job->result; + } + +if (job->reply_len && hdr->response) { +int len =min (hdr->max_response_len, job->reply_len); + +if (copy_to_user (uptr64 (hdr->response), job->reply, len)) + ret = -EFAULT ; +else + hdr->response_len = len; + } + +/* we assume all request payload was transferred, residual == 0 */ + hdr->dout_resid =0 ; + +if (rq->next_rq) { +unsigned int rsp_len = job->reply_payload.payload_len; + +if (WARN_ON (job->reply_payload_rcv_len > rsp_len)) + hdr->din_resid =0 ; +else + hdr->din_resid = rsp_len - job->reply_payload_rcv_len; + }else { + hdr->din_resid =0 ; + } + +return ret; +} + +static void bsg_transport_free_rq (struct request *rq) +{ +struct bsg_job *job =blk_mq_rq_to_pdu (rq); + +kfree (job->request); +} + +static const struct bsg_ops bsg_transport_ops = { + .check_proto = bsg_transport_check_proto, + .fill_hdr = bsg_transport_fill_hdr, + .complete_rq = bsg_transport_complete_rq, + .free_rq = bsg_transport_free_rq, +};/** * bsg_teardown_job - routine to teardown a bsg job
@@ -68,27 +156,9 @@ EXPORT_SYMBOL_GPL(bsg_job_get);void bsg_job_done (struct bsg_job *job,int result,unsigned int reply_payload_rcv_len) { -struct request *req =blk_mq_rq_from_pdu (job); -struct request *rsp = req->next_rq; -int err; - - err = job->sreq.result = result; -if (err <0 ) -/* we're only returning the result field in the reply */ - job->sreq.sense_len =sizeof (u32); -else - job->sreq.sense_len = job->reply_len; -/* we assume all request payload was transferred, residual == 0 */ - job->sreq.resid_len =0 ; - -if (rsp) { -WARN_ON (reply_payload_rcv_len >scsi_req (rsp)->resid_len); - -/* set reply (bidi) residual */ -scsi_req (rsp)->resid_len -= -min (reply_payload_rcv_len,scsi_req (rsp)->resid_len); - } -blk_complete_request (req); + job->result = result; + job->reply_payload_rcv_len = reply_payload_rcv_len; +blk_complete_request (blk_mq_rq_from_pdu (job)); }EXPORT_SYMBOL_GPL (bsg_job_done);
@@ -113,7 +183,6 @@ static int bsg_map_buffer(struct bsg_buffer *buf, struct request *req)if (!buf->sg_list)return -ENOMEM ;sg_init_table (buf->sg_list, req->nr_phys_segments); -scsi_req (req)->resid_len =blk_rq_bytes (req); buf->sg_cnt =blk_rq_map_sg (req->q, req, buf->sg_list); buf->payload_len =blk_rq_bytes (req);return 0 ;
@@ -124,16 +193,13 @@ static int bsg_map_buffer(struct bsg_buffer *buf, struct request *req)* @dev: device that is being sent the bsg request * @req: BSG request that needs a job structure */ -static int bsg_prepare_job (struct device *dev,struct request *req) +static bool bsg_prepare_job (struct device *dev,struct request *req) {struct request *rsp = req->next_rq; -struct scsi_request *rq =scsi_req (req);struct bsg_job *job =blk_mq_rq_to_pdu (req);int ret; job->timeout = req->timeout; - job->request = rq->cmd; - job->request_len = rq->cmd_len;if (req->bio) { ret =bsg_map_buffer (&job->request_payload, req);
@@ -149,12 +215,13 @@ static int bsg_prepare_job(struct device *dev, struct request *req)/* take a reference for the request */ get_device (job->dev);kref_init (&job->kref); -return 0 ; +return true; failjob_rls_rqst_payload:kfree (job->request_payload.sg_list); failjob_rls_job: -return -ENOMEM ; + job->result = -ENOMEM ; +return false; }/**
@@ -183,9 +250,7 @@ static void bsg_request_fn(struct request_queue *q)break ;spin_unlock_irq (q->queue_lock); - ret =bsg_prepare_job (dev, req); -if (ret) { -scsi_req (req)->result = ret; +if (!bsg_prepare_job (dev, req)) {blk_end_request_all (req,BLK_STS_OK );spin_lock_irq (q->queue_lock);continue ;
@@ -202,46 +267,34 @@ static void bsg_request_fn(struct request_queue *q)spin_lock_irq (q->queue_lock); } +/* called right after the request is allocated for the request_queue */ static int bsg_init_rq (struct request_queue *q,struct request *req,gfp_t gfp) {struct bsg_job *job =blk_mq_rq_to_pdu (req); -struct scsi_request *sreq = &job->sreq; - -/* called right after the request is allocated for the request_queue */ - sreq->sense =kzalloc (SCSI_SENSE_BUFFERSIZE , gfp); -if (!sreq->sense) + job->reply =kzalloc (SCSI_SENSE_BUFFERSIZE , gfp); +if (!job->reply)return -ENOMEM ; -return 0 ; } +/* called right before the request is given to the request_queue user */ static void bsg_initialize_rq (struct request *req) {struct bsg_job *job =blk_mq_rq_to_pdu (req); -struct scsi_request *sreq = &job->sreq; -void *sense = sreq->sense; - -/* called right before the request is given to the request_queue user */ +void *reply = job->reply;memset (job,0 ,sizeof (*job)); - -scsi_req_init (sreq); - - sreq->sense = sense; - sreq->sense_len =SCSI_SENSE_BUFFERSIZE ; - - job->reply = sense; - job->reply_len = sreq->sense_len; + job->reply = reply; + job->reply_len =SCSI_SENSE_BUFFERSIZE ; job->dd_data = job +1 ; }static void bsg_exit_rq (struct request_queue *q,struct request *req) {struct bsg_job *job =blk_mq_rq_to_pdu (req); -struct scsi_request *sreq = &job->sreq; -kfree (sreq->sense); +kfree (job->reply); }/**
@@ -275,11 +328,10 @@ struct request_queue *bsg_setup_queue(struct device *dev, const char *name, q->queuedata = dev; q->bsg_job_fn = job_fn;blk_queue_flag_set (QUEUE_FLAG_BIDI , q); -blk_queue_flag_set (QUEUE_FLAG_SCSI_PASSTHROUGH , q);blk_queue_softirq_done (q, bsg_softirq_done);blk_queue_rq_timeout (q,BLK_DEFAULT_SG_TIMEOUT ); - ret =bsg_register_queue (q, dev, name, release); + ret =bsg_register_queue (q, dev, name, &bsg_transport_ops, release);if (ret) {printk (KERN_ERR "%s: bsg interface failed to " "initialize - register queue\n" , dev->kobj.name);
block/bsg.c +117 -145
374 lines not on this page · raw message
drivers/scsi/scsi_lib.c +2 -2
@@ -2140,8 +2140,6 @@ void __scsi_init_queue(struct Scsi_Host *shost, struct request_queue *q) {struct device *dev = shost->dma_dev; -blk_queue_flag_set (QUEUE_FLAG_SCSI_PASSTHROUGH , q); -/* * this limit is imposed by hardware restrictions */
@@ -2239,6 +2237,7 @@ struct request_queue *scsi_old_alloc_queue(struct scsi_device *sdev) }__scsi_init_queue (shost, q); +blk_queue_flag_set (QUEUE_FLAG_SCSI_PASSTHROUGH , q);blk_queue_prep_rq (q, scsi_prep_fn);blk_queue_unprep_rq (q, scsi_unprep_fn);blk_queue_softirq_done (q, scsi_softirq_done);
@@ -2270,6 +2269,7 @@ struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev) sdev->request_queue->queuedata = sdev;__scsi_init_queue (sdev->host, sdev->request_queue); +blk_queue_flag_set (QUEUE_FLAG_SCSI_PASSTHROUGH , sdev->request_queue);return sdev->request_queue; }
drivers/scsi/scsi_sysfs.c +1 -2
@@ -1292,8 +1292,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)transport_add_device (&sdev->sdev_gendev); sdev->is_visible =1 ; - error =bsg_register_queue (rq, &sdev->sdev_gendev,NULL ,NULL ); - + error =bsg_scsi_register_queue (rq, &sdev->sdev_gendev);if (error)/* we're treating error on bsg register as non-fatal, * so pretend nothing went wrong */
drivers/scsi/scsi_transport_sas.c +0 -1
@@ -228,7 +228,6 @@ static int sas_bsg_initialize(struct Scsi_Host *shost, struct sas_rphy *rphy)*/ blk_queue_bounce_limit (q,BLK_BOUNCE_HIGH );blk_queue_flag_set (QUEUE_FLAG_BIDI , q); -blk_queue_flag_set (QUEUE_FLAG_SCSI_PASSTHROUGH , q);return 0 ; }
include/linux/bsg-lib.h +3 -1
@@ -38,7 +38,6 @@ struct bsg_buffer { };struct bsg_job { -struct scsi_request sreq;struct device *dev;struct kref kref;
@@ -64,6 +63,9 @@ struct bsg_job {struct bsg_buffer request_payload;struct bsg_buffer reply_payload; +int result; +unsigned int reply_payload_rcv_len; +void *dd_data;/* Used for driver-specific storage */ };
include/linux/bsg.h +22 -13
@@ -1,34 +1,43 @@/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef BSG_H -#define BSG_H +#ifndef _LINUX_BSG_H +#define _LINUX_BSG_H#include <uapi/linux/bsg.h> +struct request ; + +#ifdef CONFIG_BLK_DEV_BSG +struct bsg_ops { +int (*check_proto)(struct sg_io_v4 *hdr); +int (*fill_hdr)(struct request *rq,struct sg_io_v4 *hdr, +fmode_t mode); +int (*complete_rq)(struct request *rq,struct sg_io_v4 *hdr); +void (*free_rq)(struct request *rq); +}; -#if defined(CONFIG_BLK_DEV_BSG )struct bsg_class_device {struct device *class_dev;struct device *parent;int minor;struct request_queue *queue;struct kref ref; +const struct bsg_ops *ops;void (*release)(struct device *); }; -extern int bsg_register_queue (struct request_queue *q, -struct device *parent,const char *name, -void (*release)(struct device *)); -extern void bsg_unregister_queue (struct request_queue *); +int bsg_register_queue (struct request_queue *q,struct device *parent, +const char *name,const struct bsg_ops *ops, +void (*release)(struct device *)); +int bsg_scsi_register_queue (struct request_queue *q,struct device *parent); +void bsg_unregister_queue (struct request_queue *q);#else -static inline int bsg_register_queue (struct request_queue *q, -struct device *parent,const char *name, -void (*release)(struct device *)) +static inline int bsg_scsi_register_queue (struct request_queue *q, +struct device *parent) {return 0 ; }static inline void bsg_unregister_queue (struct request_queue *q) { } -#endif - -#endif +#endif /* CONFIG_BLK_DEV_BSG */ +#endif /* _LINUX_BSG_H */
Signature · 1 line
2.14.2
Jens Axboe
maintainerQuoted 9 lines · Christoph Hellwig
Hi all,
this series cleans up various abuses of the bsg interfaces, and then splits bsg for SCSI passthrough from bsg for arbitrary transport passthrough. This removes the
scsi_requestabuse in bsg-lib that is very confusing, and also makes sure we can sanity check the requests we get. The current code will happily execute scsi commands against bsg-lib queues, and transport pass through against scsi nodes, without any indication to the driver that we are doing the wrong thing.
Applied for 4.17, thanks.
Signature · 1 line
Jens Axboe