Re: [PATCH v8 04/12] xen: add basic hypervisor filesystem support

[Jan Beulich <jbeulich@suse.com>]

On 15.05.2020 07:33, Jürgen Groß wrote:
> On 14.05.20 11:50, Jürgen Groß wrote:
>> On 14.05.20 09:59, Jan Beulich wrote:
>>> On 08.05.2020 17:34, Juergen Gross wrote:
>>>> +int hypfs_read_dir(const struct hypfs_entry *entry,
>>>> +                   XEN_GUEST_HANDLE_PARAM(void) uaddr)
>>>> +{
>>>> +    const struct hypfs_entry_dir *d;
>>>> +    const struct hypfs_entry *e;
>>>> +    unsigned int size = entry->size;
>>>> +
>>>> +    d = container_of(entry, const struct hypfs_entry_dir, e);
>>>> +
>>>> +    list_for_each_entry ( e, &d->dirlist, list )
>>>
>>> This function, in particular because of being non-static, makes
>>> me wonder how, with add_entry() taking a lock, it can be safe
>>> without any locking. Initially I thought the justification might
>>> be because all adding of entries is an init-time-only thing, but
>>> various involved functions aren't marked __init (and it is at
>>> least not implausible that down the road we might see new
>>> entries getting added during certain hotplug operations).
>>>
>>> I do realize that do_hypfs_op() takes the necessary read lock,
>>> but then you're still building on the assumption that the
>>> function is reachable through only that code path, despite
>>> being non-static. An ASSERT() here would be the minimum I guess,
>>> but with read locks now being recursive I don't see why you
>>> couldn't read-lock here again.
>>
>> Right, will add the read-lock.
>>
>>>
>>> The same goes for other non-static functions, albeit things may
>>> become more interesting for functions living on the
>>> XEN_HYPFS_OP_write_contents path (because write locks aren't
>>
>> Adding an ASSERT() in this regard should be rather easy.
> 
> As the type specific read- and write-functions should only be called
> through the generic read/write functions I think it is better to have
> a percpu variable holding the current locking state and ASSERT() that
> to match. This will be cheaper than nesting locks and I don't have to
> worry about either write_lock nesting or making _is_write_locked_by_me()
> an official interface.

Ah yes, this should do.

Jan