Re: [PATCH v9] fs: Add VirtualBox guest shared folder (vboxsf) support

[Hans de Goede <hdegoede@redhat.com>]

Hi,

<big snip>

>>> +    if (err)
>>> +        return err;
>>> +
>>> +    *off += nwritten;
>>> +    if (*off > inode->i_size)
>>> +        i_size_write(inode, *off);
>>> +
>>> +    /* Invalidate page-cache so that mmap using apps see the changes too */
>>> +    invalidate_mapping_pages(inode->i_mapping, pos >> PAGE_SHIFT,
>>> +                 *off >> PAGE_SHIFT);
>>> +
>>> +    /* mtime changed */
>>> +    sf_i->force_restat = 1;
>>> +    return nwritten;
>>> +}
>>
>>> +    /* Already open? */
>>> +    if (sf_i->handle != SHFL_HANDLE_NIL) {
>>> +        sf_r->handle = sf_i->handle;
>>> +        sf_i->handle = SHFL_HANDLE_NIL;
>>> +        sf_i->file = file;
>>> +        file->private_data = sf_r;
>>
>> Obviously racy.
> 
> Ok, so how do we fix this? Add a mutex to the private
> struct sf_inode_info and take that here and in other
> relevant places I guess?
> 
> 
>>> +    /* the host may have given us different attr then requested */
>>> +    sf_i->force_restat = 1;
>>> +    sf_r->handle = params.handle;
>>> +    sf_i->file = file;
>>
>> Ditto.  Two opens in parallel and you are in trouble.
> 
> I assume what you fall over there is the "sf_i->file = file" which
> is a hack which is purely implemented to have a handle for writepage.
> 
> Looking at this closer, I think the right thing todo might be to
> implement our own .mmap callback + vma_close callback and then
> refcount the handle and store it in vm_private_data so that we
> can get it from there rather then having this hack of storing
> a struct file * in the inode which as you rightly point out is
> obviously buggy.
> 
> What do you think of using the vm_private_data approach?

Ok this is of course nonsense as we do not have access to the vma
from the writepage callback. So instead I plan to store a list
of writeable handles in the inode combined with a custom vma_close
like fuse is doing to make sure we dump any changes before loosing
the last handle.

Does that sound ok?

Regards,

Hans