linux-kernel - Re: [PATCH] fs/ext{3,4}: fix potential race when setversion ioctl updates inode

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 4 Jan 2012 18:34:36 +0100
From:	Jan Kara <jack@...e.cz>
To:	Djalal Harouni <tixxdz@...ndz.org>
Cc:	Jan Kara <jack@...e.cz>, Andrew Morton <akpm@...ux-foundation.org>,
	Andreas Dilger <adilger.kernel@...ger.ca>,
	Theodore Ts'o <tytso@....edu>,
	Yongqiang Yang <xiaoqiangnk@...il.com>,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] fs/ext{3,4}: fix potential race when setversion ioctl
 updates inode

On Wed 04-01-12 00:14:32, Djalal Harouni wrote:
> On Tue, Jan 03, 2012 at 01:46:24PM +0100, Jan Kara wrote:
> > On Tue 03-01-12 02:31:52, Djalal Harouni wrote:
> > > 
> > > The EXT{3,4}_IOC_SETVERSION ioctl() updates the inode without i_mutex,
> > > this can lead to a race with the other operations that update the same
> > > inode.
> > > 
> > > Patch tested.
> >   Thanks for the patch but I don't quite understand the problem.
> > i_generation is set when:
> >   a) inode is loaded from disk
> >   b) inode is allocated
> >   c) in SETVERSION ioctl
> > 
> >   The only thing that can race here seems to be c) against c) and that is
> > racy with i_mutex as well. So what problems do you exactly observe without
> > the patch?
> Right, but what about the related i_ctime change ? (i_ctime is updated in
> other places...)
> 
> The i_ctime update must reflect the _appropriate_ inode modification
> operation. This is why IMHO we should protect them to avoid a lost update.
  Yes, but realistically even if we race with someone else updating
i_ctime, the times will be rather similar so there's hardly a real
difference.

Anyway, using i_mutex is consistent with how we handle permission changes
or timestamp changes and the ioctl isn't performance critical so I'll take
your patch. I was just wondering whether you observed a real problem or
whether it's more or less a theoretical concern.

> BTW the i_generation which is used by NFS and fuse filesystems is updated
> even if the inode is marked immutable, is this the intended behaviour?
  Well, I'd say it's unexpected that generation can be changed for
immutable inode so I'd be inclined to take a patch that would make ext3
refuse to do that. But it's a change in how the ioctl behaves so I'd like
to hear opinion of Ted or Andrew on this.

								Honza
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/