On 03-May-19 15:18, Jason Gunthorpe wrote:
> On Fri, May 03, 2019 at 12:48:44PM +0300, Gal Pressman wrote:
>> On 02-May-19 21:02, Jason Gunthorpe wrote:
>>>
>>> On Wed, May 01, 2019 at 01:48:22PM +0300, Gal Pressman wrote:
>>>> +static int mmap_entry_insert(struct efa_dev *dev,
>>>> + struct efa_ucontext *ucontext,
>>>> + struct efa_mmap_entry *entry,
>>>> + u8 mmap_flag)
>>>> +{
>>>> + u32 mmap_page;
>>>> + int err;
>>>> +
>>>> + err = xa_alloc(&ucontext->mmap_xa, &mmap_page, entry, xa_limit_32b,
>>>> + GFP_KERNEL);
>>>> + if (err) {
>>>> + ibdev_dbg(&dev->ibdev, "mmap xarray full %d\n", err);
>>>> + return err;
>>>> + }
>>>> +
>>>> + entry->key = (u64)mmap_page << PAGE_SHIFT;
>>>> + set_mmap_flag(&entry->key, mmap_flag);
>>>
>>> This doesn't look like it is in the right order.. There is no locking
>>> here so the xa_alloc should only be called on a fully intialized entry
>>>
>>> And because there is no locking you also can't really have a
>>> mmap_obj_entries_remove..
>>>
>>> I think this needs a mutex lock also head across mmap_get to be correct..
>>
>> What needs to be atomic here is the "mmap_page" allocations, which is guaranteed
>> by xa_alloc.
>> A unique page is allocated for each insertion and then a key is generated for
>> the entry. The key needs the mmap page hence the order, a lock would be
>> redundant as the sequence does not require it.
>>
>> There are no concurrent gets (to other operations) as the key will only be
>> accessed once the insertion is done and the response is returned (at this point
>> the entry is fully initialized).
>
> nonsense, a hostile userspace can call parallel mmap to trigger races
>
>> There are no concurrent removes as they only happen in error flow which happens
>> after all the relevant insertions are done (and won't be accessed) or when the
>> ucontext is being deallocated which cannot happen simultaneously with anything else.
>
> Nope, also can be done in parallel with a hostile userspace
Thanks, will add a lock.
