lkml.org 
[lkml]   [2013]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] sg: atomize check and set sdp->exclude in sg_open
于 2013年06月05日 23:41, Jörn Engel 写道:
> On Thu, 6 June 2013 00:16:45 +0800, vaughan wrote:
>> 于 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...
>
> Sure. What I meant by line-wrapped is that your mailer mangled the
> patch. Those two lines should have been one:
>>>> - ((!sfds_list_empty(sdp) || get_exclude(sdp))
>>>> ? 0 : set_exclude(sdp, 1)));
>
>>> 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.
>
> Makes sense. And reading the code again, I have to wonder what monkey
> came up with the get_exclude/set_exclude functions.
>
> Can I sucker you into a slightly larger cleanup? I think the entire
> "get_exclude(sdp)) ? 0 : set_exclude(sdp, 1)" should be simplified.
> And once you add the try_set_exclude(), set_exclude will only ever do
> clear_exclude, so you might as well rename and simplify that as well.
I find my patch is not enough to avoid this race condition said above.
Since sg_add_sfp() just do an add_to_list without check and wait_event
check don't set a sign to announce a future add_to_list is on going, the
time window between wait_event and sg_add_sfp gives others to open sg
before the prechecked sg_add_sfp() called.

The same case also happens when one shared and one exclude open occur
simultaneously. If the shared open pass the precheck stage and ready to
sg_add_sfp(). At this time another exclude open will also pass the check:
((!sfds_list_empty(sdp) || get_exclude(sdp)) ? 0 :
try_set_exclude(sdp)));
Then, both open can succeed.

I think the point is we separate the check&add routine and haven't set
an sign to let others wait until the whole actions complete. I suppose
we may change the steps a bit to avoid trouble like this. If we can
malloc&initialize sfp at first, and then check&add sfp under the
protection of sg_index_lock, everything seems to be quite simple.


Regards,
Vaughan

>
> Let no good deed go unpunished.
>
> Jörn
>
> --
> It's just what we asked for, but not what we want!
> -- anonymous
>
--
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/

\
 
 \ /
  Last update: 2013-06-06 10:21    [W:0.140 / U:0.376 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site