On 02/01/2017 02:12 PM, Christoph Hellwig wrote: > On Wed, Feb 01, 2017 at 12:22:15PM +0100, Hannes Reinecke wrote: >> The '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 we need to protect it against concurrent accesses. >> >> Cc: stable@xxxxxxxxxxxxxxx >> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx> >> Link: http://www.spinics.net/lists/linux-scsi/msg104326.html >> Signed-off-by: Hannes Reinecke <hare@xxxxxxxx> >> Tested-by: Johannes Thumshirn <jth@xxxxxxxxxx> >> --- >> drivers/scsi/sg.c | 30 ++++++++++++------------------ >> 1 file changed, 12 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >> index 652b934..6a8601c 100644 >> --- a/drivers/scsi/sg.c >> +++ b/drivers/scsi/sg.c >> @@ -155,6 +155,8 @@ >> 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 */ >> + unsigned long flags; >> +#define SG_RESERVED_IN_USE 1 >> struct kref f_ref; >> struct execute_work ew; >> } Sg_fd; >> @@ -198,7 +200,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); >> >> @@ -721,7 +722,7 @@ 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)) { >> + if (test_bit(SG_RESERVED_IN_USE, &sfp->flags)) { >> sg_remove_request(sfp, srp); >> return -EBUSY; /* reserve buffer already being used */ >> } >> @@ -963,10 +964,14 @@ static int max_sectors_bytes(struct request_queue *q) >> val = min_t(int, val, >> max_sectors_bytes(sdp->device->request_queue)); >> if (val != sfp->reserve.bufflen) { >> - if (sg_res_in_use(sfp) || sfp->mmap_called) >> + if (sfp->mmap_called) >> + return -EBUSY; >> + if (test_and_set_bit(SG_RESERVED_IN_USE, &sfp->flags)) >> return -EBUSY; >> + >> sg_remove_scat(sfp, &sfp->reserve); >> sg_build_reserve(sfp, val); >> + clear_bit(SG_RESERVED_IN_USE, &sfp->flags); > > > This seems to be abusing an atomic bitflag as a lock. Hmm. I wouldn't call it 'abusing'; the driver can proceed quite happily without the 'reserved' buffer, so taking a lock would be overkill. I could modify it to use a mutex if you insist ... > And I think > in general we have two different things here that this patch conflates: > > a) a lock to protect building and using the reserve lists > b) a flag is a reservations is in use > No. This is not about reservations, this is about the internal 'reserved' page buffer array. (Just in case to avoid any misunderstandings). We need to have an atomic / protected check in the 'sfp' structure if the 'reserved' page buffer array is in use; there's an additional check in the 'sg_request' structure (res_in_use) telling us which of the requests is using it. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare@xxxxxxx +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)
