Date:	Fri, 10 Oct 2014 14:09:44 +0400
From:	Dmitry Monakhov <dmonakhov@...nvz.org>
To:	Jan Kara <jack@...e.cz>
Cc:	Andreas Dilger <adilger@...ger.ca>, linux-ext4@...r.kernel.org,
	sasha.levin@...cle.com, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] ext4: fix race between write and fcntl(F_SETFL)

Jan Kara <jack@...e.cz> writes:

> On Fri 10-10-14 11:48:30, Dmitry Monakhov wrote:
>> Andreas Dilger <adilger@...ger.ca> writes:
>> 
>> > On Oct 9, 2014, at 5:14 AM, Dmitry Monakhov <dmonakhov@...nvz.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@...e.cz>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists