sanitize sg
Subsystems (2)
v3 cover sanitize sg
Hi all,
the infamous syzkaller incovered some more issues in the sg driver. This patchset fixes those two issues (and adds a fix for yet another potential issue; checking for a NULL dxferp when dxfer_len is not 0). It also removes handling of the SET_FORCE_LOW_DMA ioctl, which never worked since the initial git checkin. And does some code cleanup by removing the private list implementation, using standard lists instead.
As usual, comments and reviews are welcome.
Changes to v1:
- Include reviews from Christoph
- Add patch to close race condition in sg_remove_sfp_usercontext()
- Remove stale variable 'save_scat_len'
Changes to v2:
- Move misplaced hunk
- Add Reviewed-by: and Tested-by: tags
Hannes Reinecke (5):
sg: disable SET_FORCE_LOW_DMA
sg: remove 'save_scat_len'
sg: protect accesses to 'reserved' page array
sg: use standard lists for sg_requests
sg: close race condition in sg_remove_sfp_usercontext()
Johannes Thumshirn (1):
sg: check for valid direction before starting the request
drivers/scsi/sg.c | 284 +++++++++++++++++++++++++++--------------------------- include/scsi/sg.h | 1 - 2 files changed, 141 insertions(+), 144 deletions(-)
Signature · 1 line
1.8.5.6
PATCH v3 1/6 sg: disable SET_FORCE_LOW_DMA
The ioctl SET_FORCE_LOW_DMA has never worked since the initial git check-in, and the respective setting is nowadays handled correctly.
So disable it entirely.
--- drivers/scsi/sg.c | 30 +++++++++--------------------- include/scsi/sg.h | 1 - 2 files changed, 9 insertions(+), 22 deletions(-)
drivers/scsi/sg.c +9 -21
@@ -149,7 +149,6 @@Sg_request *headrp;/* head of request slist, NULL->empty */ struct fasync_struct *async_qp;/* used by asynchronous notification */ Sg_request req_arr[SG_MAX_QUEUE ];/* used as singly-linked list */ -char low_dma;/* as in parent but possibly overridden to 1 */ char force_packid;/* 1 -> pack_id input to read(), 0 -> ignored */ char cmd_q;/* 1 -> allow command queuing, 0 -> don't */ unsigned char next_cmd_len;/* 0: automatic, >0: use on next write() */
@@ -887,24 +886,14 @@ static int max_sectors_bytes(struct request_queue *q)/* strange ..., for backward compatibility */ return sfp->timeout_user;case SG_SET_FORCE_LOW_DMA : - result =get_user (val, ip); -if (result) -return result; -if (val) { - sfp->low_dma =1 ; -if ((0 == sfp->low_dma) && (0 ==sg_res_in_use (sfp))) { - val = (int ) sfp->reserve.bufflen; -sg_remove_scat (sfp, &sfp->reserve); -sg_build_reserve (sfp, val); - } - }else { -if (atomic_read (&sdp->detaching)) -return -ENODEV ; - sfp->low_dma = sdp->device->host->unchecked_isa_dma; - } +/* +* N.B. This ioctl never worked properly, but failed to +* return an error value. So returning '0' to keep compability +* with legacy applications. +*/ return 0 ;case SG_GET_LOW_DMA : -return put_user ((int ) sfp->low_dma, ip); +return put_user ((int ) sdp->device->host->unchecked_isa_dma, ip);case SG_GET_SCSI_ID :if (!access_ok (VERIFY_WRITE , p,sizeof (sg_scsi_id_t)))return -EFAULT ;
@@ -1821,6 +1810,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonint sg_tablesize = sfp->parentdp->sg_tablesize;int blk_size = buff_size, order;gfp_t gfp_mask =GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN; +struct sg_device *sdp = sfp->parentdp;if (blk_size <0 )return -EFAULT ;
@@ -1846,7 +1836,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon scatter_elem_sz_prev = num; } -if (sfp->low_dma) +if (sdp->device->host->unchecked_isa_dma) gfp_mask |=GFP_DMA ;if (!capable (CAP_SYS_ADMIN ) || !capable (CAP_SYS_RAWIO ))
@@ -2132,8 +2122,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon sfp->timeout =SG_DEFAULT_TIMEOUT ; sfp->timeout_user =SG_DEFAULT_TIMEOUT_USER ; sfp->force_packid =SG_DEF_FORCE_PACK_ID ; - sfp->low_dma = (SG_DEF_FORCE_LOW_DMA ==0 ) ? - sdp->device->host->unchecked_isa_dma :1 ; sfp->cmd_q =SG_DEF_COMMAND_Q ; sfp->keep_orphan =SG_DEF_KEEP_ORPHAN ; sfp->parentdp = sdp;
@@ -2603,7 +2591,7 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)jiffies_to_msecs (fp->timeout), fp->reserve.bufflen, (int ) fp->reserve.k_use_sg, - (int) fp->low_dma); + (int) sdp->device->host->unchecked_isa_dma);seq_printf (s," cmd_q=%d f_packid=%d k_orphan=%d closed=0\n" , (int ) fp->cmd_q, (int ) fp->force_packid, (int ) fp->keep_orphan);
include/scsi/sg.h +0 -1
@@ -197,7 +197,6 @@#define SG_DEFAULT_RETRIES 0/* Defaults, commented if they differ from original sg driver */ -#define SG_DEF_FORCE_LOW_DMA 0/* was 1 -> memory below 16MB on i386 */ #define SG_DEF_FORCE_PACK_ID 0#define SG_DEF_KEEP_ORPHAN 0#define SG_DEF_RESERVED_SIZE SG_SCATTER_SZ/* load time option */
Signature · 1 line
1.8.5.6
Re: [PATCHv3 1/6] sg: disable SET_FORCE_LOW_DMA
Looks fine:
PATCH v3 2/6 sg: remove 'save_scat_len'
In reply to Hannes ReineckeUnused.
--- drivers/scsi/sg.c | 2 -- 1 file changed, 2 deletions(-)
drivers/scsi/sg.c +0 -2
@@ -145,7 +145,6 @@int timeout;/* defaults to SG_DEFAULT_TIMEOUT */ int timeout_user;/* defaults to SG_DEFAULT_TIMEOUT_USER */ Sg_scatter_hold reserve;/* buffer held for this file descriptor */ -unsigned save_scat_len;/* original length of trunc. scat. element */ Sg_request *headrp;/* head of request slist, NULL->empty */ struct fasync_struct *async_qp;/* used by asynchronous notification */ Sg_request req_arr[SG_MAX_QUEUE ];/* used as singly-linked list */
@@ -2006,7 +2005,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon req_schp->pages =NULL ; req_schp->page_order =0 ; req_schp->sglist_len =0 ; - sfp->save_scat_len =0 ; srp->res_used =0 ; }
Signature · 1 line
1.8.5.6
Re: [PATCHv3 2/6] sg: remove 'save_scat_len'
Looks fine,
PATCH v3 3/6 sg: protect accesses to 'reserved' page array
In reply to Hannes ReineckeThe 'reserved' page array is used as a short-cut for mapping data, saving us to allocate pages per request. However, the 'reserved' array is only capable of holding one request, so this patch introduces a mutex for protect 'sg_fd' against concurrent accesses.
--- drivers/scsi/sg.c | 47 +++++++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 20 deletions(-)
drivers/scsi/sg.c +27 -20
@@ -142,6 +142,7 @@struct sg_device *parentdp;/* owning device */ wait_queue_head_t read_wait;/* queue read until command done */ rwlock_t rq_list_lock;/* protect access to list in req_arr */ +struct mutex f_mutex;/* protect against changes in this fd */ int timeout;/* defaults to SG_DEFAULT_TIMEOUT */ int timeout_user;/* defaults to SG_DEFAULT_TIMEOUT_USER */ Sg_scatter_hold reserve;/* buffer held for this file descriptor */
@@ -153,6 +154,7 @@unsigned char next_cmd_len;/* 0: automatic, >0: use on next write() */ char keep_orphan;/* 0 -> drop orphan (def), 1 -> keep for read() */ char mmap_called;/* 0 -> mmap() never called on this fd */ +char res_in_use;/* 1 -> 'reserve' array in use */ struct kref f_ref;struct execute_work ew; } Sg_fd;
@@ -196,7 +198,6 @@ static int sg_common_write(Sg_fd * sfp, Sg_request * srp,static Sg_request *sg_get_rq_mark (Sg_fd * sfp,int pack_id);static Sg_request *sg_add_request (Sg_fd * sfp);static int sg_remove_request (Sg_fd * sfp,Sg_request * srp); -static int sg_res_in_use (Sg_fd * sfp);static Sg_device *sg_get_dev (int dev);static void sg_device_destroy (struct kref *kref);
@@ -612,6 +613,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd) } buf +=SZ_SG_HEADER ;__get_user (opcode, buf); +mutex_lock (&sfp->f_mutex);if (sfp->next_cmd_len >0 ) { cmd_size = sfp->next_cmd_len; sfp->next_cmd_len =0 ;/* reset so only this write() effected */
@@ -620,6 +622,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)if ((opcode >=0xc0 ) && old_hdr.twelve_byte) cmd_size =12 ; } +mutex_unlock (&sfp->f_mutex);SCSI_LOG_TIMEOUT (4 ,sg_printk (KERN_INFO , sdp,"sg_write: scsi opcode=0x%02x, cmd_size=%d\n" , (int ) opcode, cmd_size));/* Determine buffer size. */
@@ -719,10 +722,13 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)sg_remove_request (sfp, srp);return -EINVAL ;/* either MMAP_IO or DIRECT_IO (not both) */ } -if (sg_res_in_use (sfp)) { +mutex_lock (&sfp->f_mutex); +if (sfp->res_in_use) { +mutex_unlock (&sfp->f_mutex);sg_remove_request (sfp, srp);return -EBUSY ;/* reserve buffer already being used */ } +mutex_unlock (&sfp->f_mutex); } ul_timeout =msecs_to_jiffies (srp->header.timeout); timeout = (ul_timeout <INT_MAX ) ? ul_timeout :INT_MAX ;
@@ -955,12 +961,18 @@ static int max_sectors_bytes(struct request_queue *q)return -EINVAL ; val =min_t (int, val,max_sectors_bytes (sdp->device->request_queue)); +mutex_lock (&sfp->f_mutex);if (val != sfp->reserve.bufflen) { -if (sg_res_in_use (sfp) || sfp->mmap_called) +if (sfp->mmap_called || + sfp->res_in_use) { +mutex_unlock (&sfp->f_mutex);return -EBUSY ; + } +sg_remove_scat (sfp, &sfp->reserve);sg_build_reserve (sfp, val); } +mutex_unlock (&sfp->f_mutex);return 0 ;case SG_GET_RESERVED_SIZE : val = min_t(int, sfp->reserve.bufflen,
@@ -986,7 +998,9 @@ static int max_sectors_bytes(struct request_queue *q) result =get_user (val, ip);if (result)return result; +mutex_lock (&sfp->f_mutex); sfp->next_cmd_len = (val >0 ) ? val :0 ; +mutex_unlock (&sfp->f_mutex);return 0 ;case SG_GET_VERSION_NUM :return put_user (sg_version_num, ip);
@@ -1713,13 +1727,19 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon md = &map_data;if (md) { -if (!sg_res_in_use (sfp) && dxfer_len <= rsv_schp->bufflen) +mutex_lock (&sfp->f_mutex); +if (dxfer_len <= rsv_schp->bufflen && + !sfp->res_in_use) { + sfp->res_in_use =1 ;sg_link_reserve (sfp, srp, dxfer_len); -else { + }else { res =sg_build_indirect (req_schp, sfp, dxfer_len); -if (res) +if (res) { +mutex_unlock (&sfp->f_mutex);return res; + } } +mutex_unlock (&sfp->f_mutex); md->pages = req_schp->pages; md->page_order = req_schp->page_order;
@@ -2117,6 +2137,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonrwlock_init (&sfp->rq_list_lock);kref_init (&sfp->f_ref); +mutex_init (&sfp->f_mutex); sfp->timeout =SG_DEFAULT_TIMEOUT ; sfp->timeout_user =SG_DEFAULT_TIMEOUT_USER ; sfp->force_packid =SG_DEF_FORCE_PACK_ID ;
@@ -2190,20 +2211,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonschedule_work (&sfp->ew.work); } -static int -sg_res_in_use (Sg_fd * sfp) -{ -const Sg_request *srp; -unsigned long iflags; - -read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (srp = sfp->headrp; srp; srp = srp->nextrp) -if (srp->res_used) -break ; -read_unlock_irqrestore (&sfp->rq_list_lock, iflags); -return srp ?1 :0 ; -} -#ifdef CONFIG_SCSI_PROC_FS static int sg_idr_max_id (int id,void *p,void *data)
Signature · 1 line
1.8.5.6
Re: [PATCHv3 3/6] sg: protect accesses to 'reserved' page array
drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 731
Quoted 8 lines
drivers/scsi/sg.c +4
-1-if (sg_res_in_use (sfp)) { +mutex_lock (&sfp->f_mutex); +if (sfp->res_in_use) { +mutex_unlock (&sfp->f_mutex);sg_remove_request (sfp, srp);return -EBUSY ;/* reserve buffer already being used */ } +mutex_unlock (&sfp->f_mutex);
Holding a mutex over a the check of a single scalar doesn't make sense.
Re: [PATCHv3 3/6] sg: protect accesses to 'reserved' page array
Quoted 11 lines · Christoph Hellwig
drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 731
Quoted 8 lines
drivers/scsi/sg.c +4
-1-if (sg_res_in_use (sfp)) { +mutex_lock (&sfp->f_mutex); +if (sfp->res_in_use) { +mutex_unlock (&sfp->f_mutex);sg_remove_request (sfp, srp);return -EBUSY ;/* reserve buffer already being used */ } +mutex_unlock (&sfp->f_mutex);Holding a mutex over a the check of a single scalar doesn't make sense.
It's adds a synchronisation point, doesn't it?
Cheers,
Hannes
Signature · 5 lines
Dr. Hannes Reinecke Teamlead Storage & Networking [email protected] +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)
Re: [PATCHv3 3/6] sg: protect accesses to 'reserved' page array
Quoted 12 lines · Hannes Reinecke
Quoted 11 lines · Christoph Hellwig
drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 731
Quoted 8 lines
drivers/scsi/sg.c +4
-1-if (sg_res_in_use (sfp)) { +mutex_lock (&sfp->f_mutex); +if (sfp->res_in_use) { +mutex_unlock (&sfp->f_mutex);sg_remove_request (sfp, srp);return -EBUSY ;/* reserve buffer already being used */ } +mutex_unlock (&sfp->f_mutex);Holding a mutex over a the check of a single scalar doesn't make sense.
Quoted 1 line
It's adds a synchronisation point, doesn't it?
It does, but it doesn't actually protect anything..
Re: [PATCHv3 3/6] sg: protect accesses to 'reserved' page array
Quoted 12 lines · Christoph Hellwig
Quoted 11 lines · Hannes Reinecke
Quoted 10 lines · Christoph Hellwig
drivers/scsi/sg.c static int sg_allow_access(struct file *filp, unsigned char *cmd) · line 731
Quoted 8 lines
drivers/scsi/sg.c +4
-1-if (sg_res_in_use (sfp)) { +mutex_lock (&sfp->f_mutex); +if (sfp->res_in_use) { +mutex_unlock (&sfp->f_mutex);sg_remove_request (sfp, srp);return -EBUSY ;/* reserve buffer already being used */ } +mutex_unlock (&sfp->f_mutex);Holding a mutex over a the check of a single scalar doesn't make sense.
Quoted 2 lines
Quoted 1 line
It's adds a synchronisation point, doesn't it?
It does, but it doesn't actually protect anything..
But all the other mutex_{un,}locks() do (for instance guarding sg_build_indirect()) and this one provides a synchronization point.
Sorry but I really don't get your point here.
The sole purpose is to guard the reserved list from being altered while blk_rq_map_* or similar functions are in progess (that's what the syzcaller reproducer was doing).
Byte,
Johannes
James Bottomley
maintainerRe: [PATCHv3 3/6] sg: protect accesses to 'reserved' page array
Quoted 19 lines · Johannes Thumshirn
Quoted 14 lines · Christoph Hellwig
Quoted 13 lines · Hannes Reinecke
Quoted 12 lines · Christoph Hellwig
Review
Quoted 9 lines
drivers/scsi/sg.c +3
-1-if (sg_res_in_use (sfp)) { +mutex_lock (&sfp->f_mutex); +if (sfp->res_in_use) { +mutex_unlock (&sfp->f_mutex);sg_remove_request (sfp, srp);return -EBUSY ;/* reserve buffer already being used */
}(quoted diff) +1
-0+ mutex_unlock(&sfp->f_mutex);Holding a mutex over a the check of a single scalar doesn't make sense.
Quoted 2 lines
Quoted 1 line
It's adds a synchronisation point, doesn't it?
It does, but it doesn't actually protect anything..
Quoted 8 lines
But all the other mutex_{un,}locks() do (for instance guarding
sg_build_indirect()) and this one provides a synchronization point.Sorry but I really don't get your point here.
The sole purpose is to guard the reserved list from being altered while blk_rq_map_* or similar functions are in progess (that's what the syzcaller reproducer was doing).
What he means is that naturally aligned reads are always atomic, so adding further synchronisation gains nothing (you already atomically get either the prior or next value) and only causes an unnecessary pipeline stall. From a reviewer's perspective, the sequence
lock
read
unlock
is always a red flag because it means the writer may not understand how locking works. Usually because the writer thinks there's some other synchronization they need that this sequence doesn't give.
James
PATCH v3 4/6 sg: check for valid direction before starting the request
In reply to Hannes ReineckeFrom: Johannes Thumshirn <[email protected]>
Check for a valid direction before starting the request, otherwise we risk running into an assertion in the scsi midlayer checking for vaild requests.
--- drivers/scsi/sg.c | 46 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 12 deletions(-)
drivers/scsi/sg.c +34 -12
@@ -663,18 +663,14 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)* is a non-zero input_size, so emit a warning. */ if (hp->dxfer_direction ==SG_DXFER_TO_FROM_DEV ) { -static char cmd[TASK_COMM_LEN ]; -if (strcmp (current->comm, cmd)) { -printk_ratelimited (KERN_WARNING -"sg_write: data in/out %d/%d bytes " -"for SCSI command 0x%x-- guessing " -"data in;\n program %s not setting " -"count and/or reply_len properly\n" , - old_hdr.reply_len - (int )SZ_SG_HEADER , - input_size, (unsigned int ) cmnd[0 ], - current->comm); -strcpy (cmd, current->comm); - } +printk_ratelimited (KERN_WARNING +"sg_write: data in/out %d/%d bytes " +"for SCSI command 0x%x-- guessing " +"data in;\n program %s not setting " +"count and/or reply_len properly\n" , + old_hdr.reply_len - (int )SZ_SG_HEADER , + input_size, (unsigned int ) cmnd[0 ], + current->comm); } k =sg_common_write (sfp, srp, cmnd, sfp->timeout, blocking);return (k <0 ) ? k : count;
@@ -756,6 +752,29 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)return count; } +static bool sg_is_valid_dxfer (sg_io_hdr_t *hp) +{ +switch (dxfer_direction) { +case SG_DXFER_NONE : +if (hp->dxferp || hp->dxfer_len >0 ) +return false; +return true; +case SG_DXFER_TO_DEV : +case SG_DXFER_FROM_DEV : +case SG_DXFER_TO_FROM_DEV : +if (!hp->dxferp || hp->dxfer_len ==0 ) +return false; +return true; +case SG_DXFER_UNKNOWN : +if ((!hp->dxferp && hp->dxfer_len) || + (hp->dxferp && hp->dxfer_len ==0 )) +return false; +return true; +default : +return false; + } +} +static int sg_common_write (Sg_fd * sfp,Sg_request * srp,unsigned char *cmnd,int timeout,int blocking)
@@ -776,6 +795,9 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)"sg_common_write: scsi opcode=0x%02x, cmd_size=%d\n" , (int) cmnd[0], (int) hp->cmd_len)); +if (!sg_is_valid_dxfer (hp)) +return -EINVAL ; + k =sg_start_req (srp, cmnd);if (k) { SCSI_LOG_TIMEOUT(1, sg_printk(KERN_INFO, sfp->parentdp,
Signature · 1 line
1.8.5.6
PATCH v3 5/6 sg: use standard lists for sg_requests
In reply to Hannes Reinecke'Sg_request' is using a private list implementation; convert it to standard lists.
--- drivers/scsi/sg.c | 149 +++++++++++++++++++++++------------------------------- 1 file changed, 62 insertions(+), 87 deletions(-)
drivers/scsi/sg.c +62 -87
@@ -122,7 +122,7 @@struct sg_fd ; typedef struct sg_request { /* SG_MAX_QUEUE requests outstanding per file */ - struct sg_request *nextrp; /* NULL -> tail request (slist) */ + struct list_head entry; /* list entry */ struct sg_fd *parentfp; /* NULL -> not in use */ Sg_scatter_hold data; /* hold buffer, perhaps scatter list */ sg_io_hdr_t header; /* scsi command+info, see <scsi/sg.h> */
@@ -146,7 +146,7 @@int timeout;/* defaults to SG_DEFAULT_TIMEOUT */ int timeout_user;/* defaults to SG_DEFAULT_TIMEOUT_USER */ Sg_scatter_hold reserve;/* buffer held for this file descriptor */ -Sg_request *headrp;/* head of request slist, NULL->empty */ +struct list_head rq_list;/* head of request list */ struct fasync_struct *async_qp;/* used by asynchronous notification */ Sg_request req_arr[SG_MAX_QUEUE ];/* used as singly-linked list */ char force_packid;/* 1 -> pack_id input to read(), 0 -> ignored */
@@ -754,7 +754,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd)static bool sg_is_valid_dxfer (sg_io_hdr_t *hp) { -switch (dxfer_direction) { +switch (hp->dxfer_direction) {case SG_DXFER_NONE :if (hp->dxferp || hp->dxfer_len >0 )return false;
@@ -954,7 +954,7 @@ static int max_sectors_bytes(struct request_queue *q)if (!access_ok (VERIFY_WRITE , ip,sizeof (int )))return -EFAULT ;read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (srp = sfp->headrp; srp; srp = srp->nextrp) { +list_for_each_entry (srp, &sfp->rq_list, entry) {if ((1 == srp->done) && (!srp->sg_io_owned)) {read_unlock_irqrestore (&sfp->rq_list_lock, iflags);
@@ -967,7 +967,8 @@ static int max_sectors_bytes(struct request_queue *q)return 0 ;case SG_GET_NUM_WAITING :read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (val =0 , srp = sfp->headrp; srp; srp = srp->nextrp) { + val =0 ; +list_for_each_entry (srp, &sfp->rq_list, entry) {if ((1 == srp->done) && (!srp->sg_io_owned)) ++val; }
@@ -1042,35 +1043,33 @@ static int max_sectors_bytes(struct request_queue *q)if (!rinfo)return -ENOMEM ;read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (srp = sfp->headrp, val =0 ; val <SG_MAX_QUEUE ; - ++val, srp = srp ? srp->nextrp : srp) { + val =0 ; +list_for_each_entry (srp, &sfp->rq_list, entry) { +if (val >SG_MAX_QUEUE ) +break ;memset (&rinfo[val],0 ,SZ_SG_REQ_INFO ); -if (srp) { - rinfo[val].req_state = srp->done +1 ; - rinfo[val].problem = - srp->header.masked_status & - srp->header.host_status & - srp->header.driver_status; -if (srp->done) - rinfo[val].duration = - srp->header.duration; -else { - ms =jiffies_to_msecs (jiffies); - rinfo[val].duration = - (ms > srp->header.duration) ? - (ms - srp->header.duration) :0 ; - } - rinfo[val].orphan = srp->orphan; - rinfo[val].sg_io_owned = - srp->sg_io_owned; - rinfo[val].pack_id = - srp->header.pack_id; - rinfo[val].usr_ptr = - srp->header.usr_ptr; + rinfo[val].req_state = srp->done +1 ; + rinfo[val].problem = + srp->header.masked_status & + srp->header.host_status & + srp->header.driver_status; +if (srp->done) + rinfo[val].duration = + srp->header.duration; +else { + ms =jiffies_to_msecs (jiffies); + rinfo[val].duration = + (ms > srp->header.duration) ? + (ms - srp->header.duration) :0 ; } + rinfo[val].orphan = srp->orphan; + rinfo[val].sg_io_owned = srp->sg_io_owned; + rinfo[val].pack_id = srp->header.pack_id; + rinfo[val].usr_ptr = srp->header.usr_ptr; + val++; }read_unlock_irqrestore (&sfp->rq_list_lock, iflags); - result =__copy_to_user (p, rinfo, + result =__copy_to_user (p, rinfo,SZ_SG_REQ_INFO *SG_MAX_QUEUE ); result = result ? -EFAULT :0 ;kfree (rinfo);
@@ -1176,7 +1175,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonreturn POLLERR ;poll_wait (filp, &sfp->read_wait, wait);read_lock_irqsave (&sfp->rq_list_lock, iflags); -for (srp = sfp->headrp; srp; srp = srp->nextrp) { +list_for_each_entry (srp, &sfp->rq_list, entry) {/* if any read waiting, flag it */ if ((0 == res) && (1 == srp->done) && (!srp->sg_io_owned)) res =POLLIN |POLLRDNORM ;
@@ -2057,7 +2056,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonunsigned long iflags;write_lock_irqsave (&sfp->rq_list_lock, iflags); -for (resp = sfp->headrp; resp; resp = resp->nextrp) { +list_for_each_entry (resp, &sfp->rq_list, entry) {/* look for requests that are ready + not SG_IO owned */ if ((1 == resp->done) && (!resp->sg_io_owned) && ((-1 == pack_id) || (resp->header.pack_id == pack_id))) {
@@ -2075,70 +2074,45 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon {int k;unsigned long iflags; -Sg_request *resp;Sg_request *rp = sfp->req_arr;write_lock_irqsave (&sfp->rq_list_lock, iflags); - resp = sfp->headrp; -if (!resp) { -memset (rp,0 ,sizeof (Sg_request)); - rp->parentfp = sfp; - resp = rp; - sfp->headrp = resp; - }else { -if (0 == sfp->cmd_q) - resp =NULL ;/* command queuing disallowed */ -else { -for (k =0 ; k <SG_MAX_QUEUE ; ++k, ++rp) { -if (!rp->parentfp) -break ; - } -if (k <SG_MAX_QUEUE ) { -memset (rp,0 ,sizeof (Sg_request)); - rp->parentfp = sfp; -while (resp->nextrp) - resp = resp->nextrp; - resp->nextrp = rp; - resp = rp; - }else - resp =NULL ; +if (!list_empty (&sfp->rq_list)) { +if (!sfp->cmd_q) + goto out_unlock; + +for (k =0 ; k <SG_MAX_QUEUE ; ++k, ++rp) { +if (!rp->parentfp) +break ; } +if (k >=SG_MAX_QUEUE ) + goto out_unlock; } -if (resp) { - resp->nextrp =NULL ; - resp->header.duration =jiffies_to_msecs (jiffies); - } +memset (rp,0 ,sizeof (Sg_request)); + rp->parentfp = sfp; + rp->header.duration =jiffies_to_msecs (jiffies); +list_add_tail (&rp->entry, &sfp->rq_list);write_unlock_irqrestore (&sfp->rq_list_lock, iflags); -return resp; +return rp; +out_unlock: +write_unlock_irqrestore (&sfp->rq_list_lock, iflags); +return NULL ; }/* Return of 1 for found; 0 for not found */ static int sg_remove_request (Sg_fd * sfp,Sg_request * srp) { -Sg_request *prev_rp; -Sg_request *rp;unsigned long iflags;int res =0 ; -if ((!sfp) || (!srp) || (!sfp->headrp)) +if (!sfp || !srp ||list_empty (&sfp->rq_list))return res;write_lock_irqsave (&sfp->rq_list_lock, iflags); - prev_rp = sfp->headrp; -if (srp == prev_rp) { - sfp->headrp = prev_rp->nextrp; - prev_rp->parentfp =NULL ; +if (!list_empty (&srp->entry)) { +list_del (&srp->entry); + srp->parentfp =NULL ; res =1 ; - }else { -while ((rp = prev_rp->nextrp)) { -if (srp == rp) { - prev_rp->nextrp = rp->nextrp; - rp->parentfp =NULL ; - res =1 ; -break ; - } - prev_rp = rp; - } }write_unlock_irqrestore (&sfp->rq_list_lock, iflags);return res;
@@ -2157,7 +2131,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned loninit_waitqueue_head (&sfp->read_wait);rwlock_init (&sfp->rq_list_lock); - +INIT_LIST_HEAD (&sfp->rq_list);kref_init (&sfp->f_ref);mutex_init (&sfp->f_mutex); sfp->timeout =SG_DEFAULT_TIMEOUT ;
@@ -2196,10 +2170,13 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon { struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);struct sg_device *sdp = sfp->parentdp; +Sg_request *srp;/* Cleanup any responses which were never read(). */ -while (sfp->headrp) -sg_finish_rem_req (sfp->headrp); +while (!list_empty (&sfp->rq_list)) { + srp =list_first_entry (&sfp->rq_list, Sg_request, entry); +sg_finish_rem_req (srp); + }if (sfp->reserve.bufflen >0 ) { SCSI_LOG_TIMEOUT(6, sg_printk(KERN_INFO, sdp,
@@ -2602,7 +2579,7 @@ static int sg_proc_seq_show_devstrs(struct seq_file *s, void *v)/* must be called while holding sg_index_lock */ static void sg_proc_debug_helper (struct seq_file *s,Sg_device * sdp) { -int k, m, new_interface, blen, usg; +int k, new_interface, blen, usg;Sg_request *srp;Sg_fd *fp;const sg_io_hdr_t *hp;
@@ -2622,13 +2599,11 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)seq_printf (s," cmd_q=%d f_packid=%d k_orphan=%d closed=0\n" , (int ) fp->cmd_q, (int ) fp->force_packid, (int ) fp->keep_orphan); -for (m =0 , srp = fp->headrp; - srp !=NULL ; - ++m, srp = srp->nextrp) { +list_for_each_entry (srp, &fp->rq_list, entry) { hp = &srp->header; new_interface = (hp->interface_id =='\0' ) ?0 :1 ;if (srp->res_used) { -if (new_interface && +if (new_interface && (SG_FLAG_MMAP_IO & hp->flags)) cp =" mmap>> " ;else
@@ -2659,7 +2634,7 @@ static void sg_proc_debug_helper(struct seq_file *s, Sg_device * sdp)seq_printf (s,"ms sgat=%d op=0x%02x\n" , usg, (int ) srp->data.cmd_opcode); } -if (0 == m) +if (list_empty (&fp->rq_list))seq_puts (s," No requests active\n" );read_unlock (&fp->rq_list_lock); }
Signature · 1 line
1.8.5.6
PATCH v3 6/6 sg: close race condition in sg_remove_sfp_usercontext()
In reply to Hannes Reineckesg_remove_sfp_usercontext() is clearing any sg requests, but needs to take 'rq_list_lock' when modifying the list.
--- drivers/scsi/sg.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/scsi/sg.c +10 -2
@@ -524,6 +524,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd) }else count = (old_hdr->result ==0 ) ?0 : -EIO ;sg_finish_rem_req (srp); +sg_remove_request (sfp, srp); retval = count; free_old_hdr:kfree (old_hdr);
@@ -564,6 +565,7 @@ static int sg_allow_access(struct file *filp, unsigned char *cmd) } err_out: err2 =sg_finish_rem_req (srp); +sg_remove_request (sfp, srp);return err ? : err2 ? : count; }
@@ -803,6 +805,7 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp)SCSI_LOG_TIMEOUT (1 ,sg_printk (KERN_INFO , sfp->parentdp,"sg_common_write: start_req err=%d\n" , k));sg_finish_rem_req (srp); +sg_remove_request (sfp, srp);return k;/* probably out of space --> ENOMEM */ }if (atomic_read (&sdp->detaching)) {
@@ -815,6 +818,7 @@ static bool sg_is_valid_dxfer(sg_io_hdr_t *hp) }sg_finish_rem_req (srp); +sg_remove_request (sfp, srp);return -ENODEV ; }
@@ -1291,6 +1295,7 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonstruct sg_fd *sfp = srp->parentfp;sg_finish_rem_req (srp); +sg_remove_request (sfp, srp);kref_put (&sfp->f_ref, sg_remove_sfp); }
@@ -1825,8 +1830,6 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lonelse sg_remove_scat (sfp, req_schp); -sg_remove_request (sfp, srp); -return ret; }
@@ -2171,12 +2174,17 @@ static long sg_compat_ioctl(struct file *filp, unsigned int cmd_in, unsigned lon struct sg_fd *sfp = container_of(work, struct sg_fd, ew.work);struct sg_device *sdp = sfp->parentdp;Sg_request *srp; +unsigned long iflags;/* Cleanup any responses which were never read(). */ +write_lock_irqsave (&sfp->rq_list_lock, iflags);while (!list_empty (&sfp->rq_list)) { srp =list_first_entry (&sfp->rq_list, Sg_request, entry);sg_finish_rem_req (srp); +list_del (&srp->entry); + srp->parentfp =NULL ; } +write_unlock_irqrestore (&sfp->rq_list_lock, iflags);if (sfp->reserve.bufflen >0 ) { SCSI_LOG_TIMEOUT(6, sg_printk(KERN_INFO, sdp,
Signature · 1 line
1.8.5.6