On 07.11.2018 17:45, Miklos Szeredi wrote:
> On Wed, Nov 7, 2018 at 3:25 PM, Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
>> On 07.11.2018 16:55, Miklos Szeredi wrote:
>>> On Tue, Nov 6, 2018 at 10:31 AM, Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> wrote:
>>>> When queue_interrupt() is called from fuse_dev_do_write(),
>>>> it came from userspace directly. Userspace may pass any
>>>> request id, even the request's we have not interrupted
>>>> (or even background's request). This patch adds sanity
>>>> check to make kernel safe against that.
>>>
>>> Okay, I understand this far.
>>>
>>>> The problem is real interrupt may be queued and requeued
>>>> by two tasks on two cpus. This case, the requeuer has not
>>>> guarantees it sees FR_INTERRUPTED bit on its cpu (since
>>>> we know nothing about the way userspace manages requests
>>>> between its threads and whether it uses smp barriers).
>>>
>>> This sounds like BS. There's an explicit smp_mb__after_atomic()
>>> after the set_bit(FR_INTERRUPTED,...). Which means FR_INTERRUPTED is
>>> going to be visible on all CPU's after this, no need to fool around
>>> with setting FR_INTERRUPTED again, etc...
>>
>> Hm, but how does it make the bit visible on all CPUS?
>>
>> The problem is that smp_mb_xxx() barrier on a cpu has a sense
>> only in pair with the appropriate barrier on the second cpu.
>> There is no guarantee for visibility, if second cpu does not
>> have a barrier:
>>
>> CPU 1 CPU2 CPU3 CPU4 CPU5
>>
>> set FR_INTERRUPTED set FR_SENT
>> <smp mb> <smp mb>
>> test FR_SENT (== 0) test FR_INTERRUPTED (==1)
>> list_add[&req->intr_entry] read[req by intr_entry]
>> <place to insert a barrier>
>> goto userspace
>> write in userspace buffer
>> read from userspace buffer
>> write to userspace buffer
>> read from userspace buffer
>> enter kernel
>> <place to insert a barrier>
>> test FR_INTERRUPTED <- Not visible
>>
>> The sequence:
>>
>> set_bit(FR_INTERRUPTED, ...)
>> smp_mb__after_atomic();
>> test_bit(FR_SENT, &req->flags)
>>
>> just guarantees the expected order on CPU2, which uses <smp mb>,
>> but CPU5 does not have any guarantees.
>
> What you are missing is that there are other things that synchronize
> memory access besides the memory barrier. In your example the
> ordering will be guaranteed by the fiq->waitq.lock in
> queue_interrupt() on CPU2: the set_bit() cannot move past the one-way
> barrier provided by the spin_unlock().
I thought, RELEASE is related to memory operations, which is made on the same cpu.
Strange thing, but the below memory-model test says, it's not true. Ok, I'll change
the patch, thanks for pointing this.
===== tools/memory-model/litmus-tests/test.litmus =====
C SB+test
{}
P0(atomic_t *flags)
{
int r0;
atomic_add(1, flags);
smp_mb__after_atomic();
r0 = (atomic_read(flags) & 2);
}
P1(atomic_t *flags, int *linked)
{
int r0;
atomic_add(2, flags);
smp_mb__after_atomic();
r0 = (atomic_read(flags) & 1);
if (r0) {
spin_lock(mylock);
*linked = 1;
spin_unlock(mylock);
}
}
P2(atomic_t *flags, int *linked)
{
int r0;
spin_lock(mylock);
if (*linked) {
r0 = atomic_read(flags);
} else
r0 = 0;
spin_unlock(mylock);
}
exists (0:r0=0 /\ 1:r0=1 /\ 2:r0=2)
===================================
kirill@pro:~/linux-next/tools/memory-model$ ~/.opam/system/bin/herd7 -conf linux-kernel.cfg litmus-tests/test.litmus
Test SB+test Allowed
States 5
0:r0=0; 1:r0=1; 2:r0=0;
0:r0=0; 1:r0=1; 2:r0=3;
0:r0=2; 1:r0=0; 2:r0=0;
0:r0=2; 1:r0=1; 2:r0=0;
0:r0=2; 1:r0=1; 2:r0=3;
No
Witnesses
Positive: 0 Negative: 7
Condition exists (0:r0=0 /\ 1:r0=1 /\ 2:r0=2)
Observation SB+test Never 0 7
Time SB+test 0.09
Hash=0213fd54f80c511af2326e1bd120a96b
