Re: [PATCH 00/14] xfs: Support for interacting with multiple user namespaces

[Dave Chinner <david@fromorbit.com>]

On Wed, Mar 13, 2013 at 03:21:11PM -0700, Eric W. Biederman wrote:
> 
> Or replace uids, gids, and projids with kuids, kgids, and kprojids
> 
> In support of user namespaces I have introduced into the kernel kuids,
> kgids, and kprojids.  The most important characteristic of these ids is
> that they are not assignment compatible with uids, gids, and projids
> coming from userspace nor are they assignment compatible with uids, gids
> or projids stored on disk.  This assignment incompatibility makes it
> easy to ensure that conversions are introduced at the edge of userspace
> and at the interface between on-disk and in-memory data structures.

TL;DR:

Compared to the last version of this patchset I NACKed, this version
increases the per-inode memory footprint by over 100 bytes (i.e. by
more than 10%), introduces a double copy of the inode core data into
the *hottest path in XFS*, breaks log recovery and fails to address
a single NACK I gave for the previous round of the patch set.

So, NACK.

> Converting xfs is an interesting challenge because of the way xfs
> handles it's inode data is very atypical. 

That you have to tell people that find it strange an unusual
immediately indicates that you do not really understand the design
and structure of the XFS code. This makes your refusal to listen to
feedback from someone who is a subject matter expert all the more
difficult to understand.

I'll accept that you might forget something mentioned in a review
when you post an update, but to ignore a second NACK for the same
patch and posting it a third time unchanged is not the best way to
make friends and influence people....

And given that you didn't respond to a single review comment I made
on the previous version of the patch, well, I have my doubts you are
going to respond this time, either.  In previous reviews comments I
have:
	- outlined exactly how to provide a minimally invasive patch
	  set that provides full namespace support as a first step
	  in getting XFS to support namespaces.
	- told you where the ondisk/in-memory boundaries are.
	- told you that certain IDs are filesystem internal and not
	  subject to namespaces.
	- asked questions about how filesystems utilities are
	  supposed to deal with namespaces (i.e. userspace impacts
	  of ioctl interface changes). 

And you haven't responded to any of them.  You can't selectively
ignore review comments you don't like and then magically expect the
reviewers to accept an almost-identical-but-even-more-broken patch
set the next time around.

> Given the number of ioctls that xfs supports it would be irresponsible to
> do anything except insist that kuids, kgids, and kprojids are used in all of
> in memory data structures of xfs, as otherwise it becomes trivially easy to
> miss a needed conversion with the advent of a new ioctl.

Eric, your rhetoric looks so fine and shiny on the wall, but it is
utterly worthless.  You're telling us that userspace interface are
absolutely necessary, but haven't provided any analysis, description
or justification so we can judge the impact of the changes or review
why you think the only way such changes can be made is your way.

Nor have you provided any regression tests to verify that this shiny
new namespace support is working as the maker has intended. IOWs,
I've got no idea what the impact of changing all the ioctls will be,
no way to verify it is correct and you sure as hell aren't going out
of your way to make it easy for us to understand the impact, either.

Further, I'm pretty sure that you are not even aware of the scope of
the issues that namespace awareness raise for some of these XFS
ioctls. I've previously asked some questions about behaviours of
them, but like most of my other review comments you have ignored
those questions.

So, before we go changing anything ioctl related, here's some
questions you need to answer:

	- if you run bulkstat, what do we do with inodes that
	  contain a ID owned by a namespace outside the current
	  namespace?

	- Can we even check the on-disk inode IDs are valid within a
	  specific namespace within the kernel?

	- open_by_handle() would appear to allow root in any
	  namespace to open any file in the filesystem it has a
	  valid handle for regardless of the namespace. Is this
	  allowable? The output of bulkstat can be fed directly to
	  open_by_handle(), so if bulkstat can return inodes outside
	  the namespace, open-by-handle can open them and we can do
	  invisible IO on them.

	- Further, we can extract and set attributes directly by
	  handle and, IIRC, that includes security/trusted namespace
	  attributes....

	- On the same measure, the handles used by XFS handle
	  interfaces are identical to NFS handles and use the same
	  code for decoding and dentry lookup. So, what do handle
	  restrictions mean for NFS servers?

	- have you considered that fs/fhandle.c raises many of these
	  same issues?

	- and seeing xfsdump and xfs_fsr use bulkstat and handles,
	  what do new limitations on these interfaces mean for these
	  utilities?

	- How does xfs_dump/xfs_restore work if we convert all ids
	  based on the current namespace? e.g. dump in one
	  namespace, restore in another.

	- How does xfs_dump/xfs_restore work if we *don't* convert
	  all ids (i.e. export the on-disk values)?

	- How do we document/communicate all these constraints/
	  behaviours to users who might be completely unaware of
	  them?

IOWs, simply converting IDs the ioctls take in or emit is only a
small part of the larger question of how they are supposed to behave
in namespace containers. These questions need to be answered *in
detail* and with *regression tests* before we accept any changes to
the ioctl interfaces.

Eric, I'm not trying to be difficult here - I'm holding the bar at
the same height as it gets held for any significant XFS change that
impacts userspace interfaces and behaviour. You don't maintain the
XFS code, so you can just walk away once namespaces are "done" and
not care that you've left a mess behind.

And if you leave a mess, I'm the person who will have to clean it
up. I don't want to have to clean up your mess, so I'm going to keep
saying no until you can introduce namespace support without making a
mess....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs