On 02/14/2017 09:48 PM, Dmitry Vyukov wrote: > On Wed, Feb 1, 2017 at 2:21 PM, Hannes Reinecke <h...@suse.de> wrote: >> 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: sta...@vger.kernel.org >>>> Reported-by: Dmitry Vyukov <dvyu...@google.com> >>>> Link: http://www.spinics.net/lists/linux-scsi/msg104326.html >>>> Signed-off-by: Hannes Reinecke <h...@suse.com> >>>> Tested-by: Johannes Thumshirn <j...@kernel.org> >>>> --- >>>> 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. > > > So, how should we proceed here? > We could use a mutex with only trylock, but it would be effectively the same. > > There is one missed user of sg_res_in_use in "case SG_SET_FORCE_LOW_DMA". > I've played around using a spinlock (can't use a mutex as one access in done from softirq context), but always ended up in lockdep hell.
And in the end, we really only need to have a marker whether the reserved array is in use or not. Which should be atomic, so we _could_ be using an atomic variable here. Which would only ever have a value of '0' or '1', hence the use of a bitfield here. But if that fall foul of some style guide I could modify it to use an atomic counter. Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking h...@suse.de +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)
