linux-kernel - Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for read-only access using rcu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Tue, 20 May 2014 17:32:54 +0200
From:	Maarten Lankhorst <maarten.lankhorst@...onical.com>
To:	Thomas Hellstrom <thellstrom@...are.com>
CC:	linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
	ccross@...gle.com, linux-media@...r.kernel.org
Subject: Re: [RFC PATCH 2/2 with seqcount v3] reservation: add suppport for
 read-only access using rcu

op 20-05-14 17:13, Thomas Hellstrom schreef:
> On 05/19/2014 03:13 PM, Maarten Lankhorst wrote:
>> op 19-05-14 15:42, Thomas Hellstrom schreef:
>>> Hi, Maarten!
>>>
>>> Some nitpicks, and that krealloc within rcu lock still worries me.
>>> Otherwise looks good.
>>>
>>> /Thomas
>>>
>>>
>>>
>>> On 04/23/2014 12:15 PM, Maarten Lankhorst wrote:
>>>> @@ -55,8 +60,8 @@ int reservation_object_reserve_shared(struct
>>>> reservation_object *obj)
>>>>                kfree(obj->staged);
>>>>                obj->staged = NULL;
>>>>                return 0;
>>>> -        }
>>>> -        max = old->shared_max * 2;
>>>> +        } else
>>>> +            max = old->shared_max * 2;
>>> Perhaps as a separate reformatting patch?
>> I'll fold it in to the patch that added
>> reservation_object_reserve_shared.
>>>> +
>>>> +int reservation_object_get_fences_rcu(struct reservation_object *obj,
>>>> +                      struct fence **pfence_excl,
>>>> +                      unsigned *pshared_count,
>>>> +                      struct fence ***pshared)
>>>> +{
>>>> +    unsigned shared_count = 0;
>>>> +    unsigned retry = 1;
>>>> +    struct fence **shared = NULL, *fence_excl = NULL;
>>>> +    int ret = 0;
>>>> +
>>>> +    while (retry) {
>>>> +        struct reservation_object_list *fobj;
>>>> +        unsigned seq;
>>>> +
>>>> +        seq = read_seqcount_begin(&obj->seq);
>>>> +
>>>> +        rcu_read_lock();
>>>> +
>>>> +        fobj = rcu_dereference(obj->fence);
>>>> +        if (fobj) {
>>>> +            struct fence **nshared;
>>>> +
>>>> +            shared_count = ACCESS_ONCE(fobj->shared_count);
>>> ACCESS_ONCE() shouldn't be needed inside the seqlock?
>> Yes it is, shared_count may be increased, leading to potential
>> different sizes for krealloc and memcpy
>> if the ACCESS_ONCE is removed. I could use shared_max here instead,
>> which stays the same,
>> but it would waste more memory.
> Maarten, Another perhaps ignorant question WRT this,
> Does ACCESS_ONCE() guarantee that the value accessed is read atomically?
Well I've reworked the code to use shared_max, so this point is moot. :-)

On any archs I'm aware of it would work, either the old or new value would be visible, as long as natural alignment is used.
rcu uses the same trick in the rcu_dereference macro, so if this didn't work rcu wouldn't work either.

~Maarten
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/