Re: [PATCH] ext4: fix race between write and fcntl(F_SETFL)

Re: [PATCH] ext4: fix race between write and fcntl(F_SETFL)

[Jan Kara]

On Fri 10-10-14 11:48:30, Dmitry Monakhov wrote:
> Andreas Dilger <adilger@dilger.ca> writes:
> 
> > On Oct 9, 2014, at 5:14 AM, Dmitry Monakhov <dmonakhov@openvz.org> wrote:
> >> O_DIRECT flags can be toggeled via fcntl(F_SETFL).
> >> But this value checked twice inside ext4_file_write_iter() and __generic_file_write()
> >> which result in BUG_ON (see typical stack trace below)
> >> In order to fix this we have to use our own copy of __generic_file_write
> >> and pass o_direct status explicitly.
> >
> > This seems like a generic problem that would be better served by fixing
> > the core code instead of making a private copy of such a large function
> > for ext4.  I expect other filesystems might have similar issues if the
> > O_DIRECT state is changed in the middle of IO?
> >
> > One option is to flush pending IO on the file if the O_DIRECT flag is
> > changed in setfl(). This is a bit heavyweight but I can't imagine any
> > sane app that is changing the O_DIRECT state on a file repeatedly.
> Agree. In fact there are a lot of other issues there fcntl(F_SETFL)
> works incorrectly. For example it does not works on stack-fs
> (ecrypt,unionfs) if you try to add O_APPEND flags it will be directly
> assigned to virtual file (not lower one). For that reason OpenVZ introduced
> f_op->set_flags in order to support our stackfs (vefs)
> This is reasonable solution in general so I'll prepare patches for mainstream
> But this require reasonable time for negotiations with vfs people.
  Agreed. ->set_flags callback looks reasonable.

> At the same time currently all versions are affected since 69c499d1/2012-11-29
> So we need quick and simple fix for stable releases.
> From first glance the best fix is to simply deny toggle O_DIRECT on files.
> and f_op->check_flags(new_flags) looks like reasonable candidate, *but*
> this interface is 100% useless because it has no access to file_pointer
> so we do not know current ->f_flags :)
  [I've CCed linux-fsdevel since it may indeed affect other filesystems]
  I don't think you want to just deny toggling O_DIRECT. That's certainly
going to break some application (however stupid it may be). Also flushing
the IO as Andreas suggests doesn't seem easy because to solve the problem
with changing flags in general, you would have to wait for both reads and
writes, buffered & direct for the struct file and that's not easily doable.
What currently seems as the best solution to me is to store f_flags in the
iocb and then use that for decisions... However it still seems a bit
fragile since people modifying the would would have to have in mind they
have to use iocb->f_flags instead of iocb->ki_filp->f_flags

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] ext4: fix race between write and fcntl(F_SETFL)

[Dmitry Monakhov]

[-- Attachment #1: Type: text/plain, Size: 3294 bytes --]

Jan Kara <jack@suse.cz> writes:

> On Fri 10-10-14 11:48:30, Dmitry Monakhov wrote:
>> Andreas Dilger <adilger@dilger.ca> writes:
>> 
>> > On Oct 9, 2014, at 5:14 AM, Dmitry Monakhov <dmonakhov@openvz.org> wrote:
>> >> O_DIRECT flags can be toggeled via fcntl(F_SETFL).
>> >> But this value checked twice inside ext4_file_write_iter() and __generic_file_write()
>> >> which result in BUG_ON (see typical stack trace below)
>> >> In order to fix this we have to use our own copy of __generic_file_write
>> >> and pass o_direct status explicitly.
>> >
>> > This seems like a generic problem that would be better served by fixing
>> > the core code instead of making a private copy of such a large function
>> > for ext4.  I expect other filesystems might have similar issues if the
>> > O_DIRECT state is changed in the middle of IO?
>> >
>> > One option is to flush pending IO on the file if the O_DIRECT flag is
>> > changed in setfl(). This is a bit heavyweight but I can't imagine any
>> > sane app that is changing the O_DIRECT state on a file repeatedly.
>> Agree. In fact there are a lot of other issues there fcntl(F_SETFL)
>> works incorrectly. For example it does not works on stack-fs
>> (ecrypt,unionfs) if you try to add O_APPEND flags it will be directly
>> assigned to virtual file (not lower one). For that reason OpenVZ introduced
>> f_op->set_flags in order to support our stackfs (vefs)
>> This is reasonable solution in general so I'll prepare patches for mainstream
>> But this require reasonable time for negotiations with vfs people.
>   Agreed. ->set_flags callback looks reasonable.
BTW: same crap with fadvise.
>
>> At the same time currently all versions are affected since 69c499d1/2012-11-29
>> So we need quick and simple fix for stable releases.
>> From first glance the best fix is to simply deny toggle O_DIRECT on files.
>> and f_op->check_flags(new_flags) looks like reasonable candidate, *but*
>> this interface is 100% useless because it has no access to file_pointer
>> so we do not know current ->f_flags :)
>   [I've CCed linux-fsdevel since it may indeed affect other filesystems]
>   I don't think you want to just deny toggling O_DIRECT. That's certainly
> going to break some application (however stupid it may be). Also flushing
> the IO as Andreas suggests doesn't seem easy because to solve the problem
> with changing flags in general, you would have to wait for both reads and
> writes, buffered & direct for the struct file and that's not easily doable.
> What currently seems as the best solution to me is to store f_flags in the
> iocb and then use that for decisions... However it still seems a bit
> fragile since people modifying the would would have to have in mind they
> have to use iocb->f_flags instead of iocb->ki_filp->f_flags
We can easily serialize AIO,DIO and buf-writers via i_mutex and i_dio_count
the only non-serialized callers are buffered reads and mmaped files.
So tend to agree with iocb->f_flags
>
> 								Honza
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Type: application/pgp-signature, Size: 818 bytes --]