On Thu, Jun 16, 2011 at 03:47:27PM +0000, Emmanuel Dreyfus wrote:
> Hello
>
> Here is a patch that adds mount -o extattr for auto-enabling UFS1
> extended attributes.
> http://ftp.espci.fr/shadow/manu/mount_extattr.patch
>
> Any comment on it before I commit?
>
> The next step is to add UFS_EXTATTR to GENERIC, XEN3_DOM0 and XEN3_DOMU
> kernels (and others? which ones?). If there a smart way, or should I
> do it for all ports?
The more I think about this code -- not yours, but the original
implementation -- the less comfortable I am with it. It seems to be
something of a conceptual mess: it keeps metadata in plain files and
treats the metadata writes like file data writes, going around our entire
mechanism for preserving metadata integrity.
As you've observed, this poses an increased risk of filesystem corruption,
and this code is not sufficiently robust to handle such corruption on
system startup without crashing the kernel.
And there are locking issues with "magically" creating file Y when an
operation is done on file X, some of which you've found/fixed, but I
bet there are more.
Given the way this code works, creating a shadow tree of files
elsewhere in the filesystem namespace, it seems like it is almost the
canonical example of something that should be a layered filesystem
rather than internally implemented in FFS. Then it would be possible,
for example, to mount the upper layer "sync" so that attribute writes
became synchronous and there were much less risk of corruption on crash.
Since extended attributes are often used to store data that are used for
authentication purposes, it seems to me there will potentially be a
dramatic change in system behavior when they are turned on as the system
goes to multiuser mode -- far from ideal though I suppose acceptable if
this is very well documented and the usual security level restrictions
are applied (can't remount the filesystem without -o extattr if
security level > 0, etc).
I think this code is a very good example of why disabled-by-default
options are a really bad idea in our kernel. If this code had not spent
a decade bit-rotting, I believe its serious problems would have been much
more obvious and by now it would have likely been removed from the tree.
I think that -- at least -- fsck needs to learn to put the extended
attribute "database" into a state in which it cannot crash the kernel,
before this code should be turned on by default. However, given that
the extended attributes are treated like file data, that poses a risk
of truncation, which could have serious security consequences. So,
I think that before this code is turned on by default, it really should
to:
1) Be modified to perform all extattr writes synchronously,
arranging to flush the disk cache after each. Alternately
it might be possible to use WAPBL to preserve the ordering
of the extattr and other metadata writes -- but have we ever
used WAPBL for file-data-block writes like this?
Note that there are tricky corner cases when attributes hold
authorization data. For example, what if an application
changes the extended attributes, then writes sensitive data
to a file, thinking it is protected? It is necessary to order
the extattr write and the subsequent data write properly or
this is not safe.
2) Be able to repair damaged attribute databases, stopping and
demanding manual intervention (manual fsck?) when it detects
corruption that might have security or stability consequences.
3) In an ideal world, be reimplemented as a layer rather than a
bunch of code inside FFS.
Thor