linux-ext4 - Re: [PATCH] ext4: fix race between write and fcntl(F

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 10 Oct 2014 11:39:34 +0200
From:	Jan Kara <jack@...e.cz>
To:	Dmitry Monakhov <dmonakhov@...nvz.org>
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)

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.

> 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@...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