linux-kernel - Re: [PATCH 0/7] overlay filesystem: request for inclusion

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, 15 Jun 2011 17:49:05 +0200
From:	Miklos Szeredi <miklos@...redi.hu>
To:	"J. R. Okajima" <hooanon05@...oo.co.jp>
Cc:	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Valerie Aurora <val@...consulting.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	NeilBrown <neilb@...e.de>, viro@...IV.linux.org.uk,
	torvalds@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, apw@...onical.com, nbd@...nwrt.org,
	hramrach@...trum.cz, jordipujolp@...il.com, ezk@....cs.sunysb.edu
Subject: Re: [PATCH 0/7] overlay filesystem: request for inclusion

"J. R. Okajima" <hooanon05@...oo.co.jp> writes:

> Miklos Szeredi:
>> For example "rmdir /ovl/a/b" will do the following:
>>
>>   1. find the underlying dentry for "a" -> upper-a
>>   2. lock upper-a
>>   3. find the underlying dentry for "b" -> upper-b
>>   4. verify that upper-b is a child of upper-a
>>   5. remove upper-b
>
> It is good to verify in step 4.
> Essentially (or ideally) this verification should be equivalent to all
> of what VFS does before vfs_rmdir(). I know overlayfs makes the upper
> mnt_want_write()-ed in early stage and keeps it. So it might be better
> to lookup again (as step 3 and 4) instead of comparing d_parent
> simply. If you think it is unnecessary to lookup here, then I'd suggest
> you to make it option (choosable by user).

The parent verification is only to make sure the locking is correct.
It's not to make sure that modifications of underlying filesystems will
have sane semantics.

Until someone comes up with a sane use case for allowing modification of
underlying filesystem I won't bother with that.

> I see ovl_rmdir() does,
> - lookup and unlink all whiteouts
> - rmdir the target dir
> - create a whiteout for the target
> Right?

Not quite.

 - checks if directory is empty (all lower entries are whiteouted)
 - marks directory opaque
 - unlinks whiteouts
 - rmdir
 - create whiteout

> But I am afraid that any error can happen in every step on the upper
> dir. And if it happens, then ovl_rmdir() returns the error but the dir
> left in incomplete status. It may be one of these.
> - some whiteouts are unlinked but others are left
> - all whiteouts are gone but the target dir remains
> - the target dir is removed but the whiteout is not created
> Of course, it is bad and makes users really confused, since it will show
> users things which should not be. At the same time, I don't know how
> possible it can happen.

Atomic whiteout and atomic copy-up would be nice, that's one feature I'm
willing to think about.

> Anyway if you have read aufs, then you would know how aufs solves these
> problems. I don't think the approaches in aufs is best or one and
> only. I just could not get another good idea.

Rollback on failure is an incomplete solution, rollback itself can fail.
And it doesn't protect against machine crashing in the middle of
operation.

Thanks,
Miklos
--
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/