Messages in this thread | | | Date | Thu, 06 Jun 2013 00:16:45 +0800 | From | vaughan <> | Subject | Re: [PATCH] sg: atomize check and set sdp->exclude in sg_open |
| |
于 2013年06月05日 21:27, Jörn Engel 写道: > On Wed, 5 June 2013 17:18:33 +0800, vaughan wrote: >> >> Check and set sdp->exclude should be atomic when set in sg_open(). > > The patch is line-wrapped. More importantly, it doesn't seem to do It's shorter than the original line, so I just leave it like this...
> what your description indicates it should do. And lastly, does this > fix a bug, possibly even one you have a testcase for, or was it found > by code inspection? I found it by code inspection. A race condition may happen with the old code if two threads are both trying to open the same sg with O_EXCL simultaneously. It's possible that they both find fsds list is empty and get_exclude(sdp) returns 0, then they both call set_exclude() and break out from wait_event_interruptible and resume open. So it's necessary to check again with sg_open_exclusive_lock held to ensure only one can set sdp->exclude and return >0 to break out from wait_event loop.
> >> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com> >> --- >> drivers/scsi/sg.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >> index 25b5455..0ede08f 100644 >> --- a/drivers/scsi/sg.c >> +++ b/drivers/scsi/sg.c >> @@ -245,6 +245,21 @@ static int set_exclude(Sg_device *sdp, char val) >> return val; >> } >> >> +/* Check if we can set exclude and then set, return 1 if success */ >> +static int try_set_exclude(Sg_device *sdp) >> +{ >> + unsigned long flags; >> + >> + spin_lock_irqsave(&sg_open_exclusive_lock, flags); >> + if (sdp->exclude) { >> + spin_unlock_irqrestore(&sg_open_exclusive_lock, flags); >> + return 0; >> + } >> + sdp->exclude = 1; >> + spin_unlock_irqrestore(&sg_open_exclusive_lock, flags); >> + return 1; >> +} >> + >> static int sfds_list_empty(Sg_device *sdp) >> { >> unsigned long flags; >> @@ -303,7 +318,7 @@ sg_open(struct inode *inode, struct file *filp) >> goto error_out; >> } >> res = wait_event_interruptible(sdp->o_excl_wait, >> - ((!sfds_list_empty(sdp) || get_exclude(sdp)) >> ? 0 : set_exclude(sdp, 1))); >> + ((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 : >> try_set_exclude(sdp))); >> if (res) { >> retval = res; /* -ERESTARTSYS because signal hit process */ >> goto error_out; >> -- >> 1.7.11.7 >> > > Jörn > > -- > Fantasy is more important than knowledge. Knowledge is limited, > while fantasy embraces the whole world. > -- Albert Einstein > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |