On Wed, 30 Aug 2023 at 16:38, Bernd Schubert <aakef@xxxxxxxxxxx> wrote:
>
>
>
> On 8/30/23 15:28, Miklos Szeredi wrote:
> > On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@xxxxxxx> wrote:
> >>
> >> Take a shared lock in fuse_cache_write_iter. This was already
> >> done for FOPEN_DIRECT_IO in
> >>
> >> commit 153524053bbb ("fuse: allow non-extending parallel direct
> >> writes on the same file")
> >>
> >> but so far missing for plain O_DIRECT. Server side needs
> >> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that
> >> it supports parallel dio writes.
> >
> > Hmm, I think file_remove_privs() needs exclusive lock (due to calling
> > notify_change()). And the fallback case also.
> >
> > Need to be careful with such locking changes...
>
> Hrmm, yeah, I missed that :( Really sorry and thanks!
>
> I guess I can fix it if by exporting dentry_needs_remove_privs. I hope
> that is acceptable. Interesting is that btrfs_direct_write seems to have
> the same issue.
>
> btrfs_direct_write
> if (iocb->ki_pos + iov_iter_count(from) <= i_size_read(inode))
> ilock_flags |= BTRFS_ILOCK_SHARED;
> ...
> err = btrfs_inode_lock(BTRFS_I(inode), ilock_flags);
> ...
> err = btrfs_write_check(iocb, from, err);
> ...
> ret = file_remove_privs(file);
>
>
> I think that can be fixed as well after exporting
> dentry_needs_remove_privs().
Interesting. Would be worth asking the btrfs devs what they think of this.
>
>
> Btw, why is fuse_direct_write_iter not dropping privileges? That is
> another change I need to document...
Generally read and write can't fail due to lack of privileges, so that
should be okay. But it would be more consistent to drop privs during
I/O as well. But this is something that needs some thought as well,
because there could be non-obvious consequences.
> Another issue I just see is that it needs to check file size again after
> taking the lock.
Yes.
> Thanks a lot for your review,
Thanks for doing the actual work, that's the harder one.
Thanks,
Miklos
