Re: [PATCH 2/2] afs: Add metadata xattrs

NAK.  Don't overload xattrs with magic behavior just to avoid the need
to do proper syscalls or ioctls.  This makes them harder to discover,
audit and security fix.

On Thu, Jul 06, 2017 at 03:50:27PM +0100, David Howells wrote:

233 more quoted lines

---end quoted text---

Christoph Hellwig [off-list ref] wrote:

NAK.  Don't overload xattrs with magic behavior just to avoid the need
to do proper syscalls or ioctls.

How?  This has to work on non-files, files you can't open and mountpoints.
You can't do an ioctl() on a file opened O_PATH:

	if (unlikely(f->f_flags & O_PATH)) {
		f->f_mode = FMODE_PATH;
		f->f_op = &empty_fops;
		return 0;
	}

and you can't specify AT_NO_AUTOMOUNT or AT_NO_FOLLOW to openat(), so ioctl()
is of no use here.

Do you advocate introducing a pioctl() call?  Linus was dead-set against that
as I recall.

I could invent a bunch of AFS-specific syscalls, but I'd rather not do that or
I suppose bring my fsinfo() patches up to scratch - but you didn't like those
either.

Note that using xattrs for fs info is not without precedent in Linux - cifs,
for example.

David

On Jul 6, 2017, at 10:14 AM, David Howells [off-list ref] wrote:

Christoph Hellwig [off-list ref] wrote:

quoted

NAK.  Don't overload xattrs with magic behavior just to avoid the need
to do proper syscalls or ioctls.

How?  This has to work on non-files, files you can't open and mountpoints.
You can't do an ioctl() on a file opened O_PATH:

	if (unlikely(f->f_flags & O_PATH)) {
		f->f_mode = FMODE_PATH;
		f->f_op = &empty_fops;
		return 0;
	}

and you can't specify AT_NO_AUTOMOUNT or AT_NO_FOLLOW to openat(), so ioctl()
is of no use here.

Do you advocate introducing a pioctl() call?  Linus was dead-set against that
as I recall.

I could invent a bunch of AFS-specific syscalls, but I'd rather not do that or
I suppose bring my fsinfo() patches up to scratch - but you didn't like those
either.

Note that using xattrs for fs info is not without precedent in Linux - cifs,
for example.

IMHO, xattrs are a fairly reasonable interface for accessing filesystem-specific
attributes of a file that do not have generic equivalents on other filesystems.
I can't see there being much value to having AFS-specific syscalls, and xattrs
also are more easily accessed by generic userspace tools than ioctl() calls.

There is also the general negative opinion among kernel developers to adding new
ioctls in the first place that means there are few options for how to get
non-standard information.

I could imagine that "fid" (file identifier that is larger than 64-bit inode) is
common enough that it could be added to statx(). I think at least "volume" (maybe
as "label") and "cell" (maybe == "server" or "fsname"?) could be generic part of
fsinfo if there was some willingness to accept that interface upstream.

Cheers, Andreas

On Thu, Jul 6, 2017 at 11:27 AM, Andreas Dilger [off-list ref] wrote:

IMHO, xattrs are a fairly reasonable interface for accessing filesystem-specific
attributes of a file that do not have generic equivalents on other filesystems.
I can't see there being much value to having AFS-specific syscalls, and xattrs
also are more easily accessed by generic userspace tools than ioctl() calls.

Yeah, I think attributes are likely much better than some random crazy
ioctl interface. They can be listed with generic tools, and have
various scripting interfaces in ways that ioctl's do not sanely have.

And people tend to be encouraged to use good descriptive interfaces
due to attributes having *names* instead of numbers.

That said, if these things have some actual generic cross-filesystem
meaning, then some ad-hoc fs attribute might be debatable. It might
still be an attribute, but perhaps better in an actual generic
namespace.

I haven't looked at these particular attibutes yet, though.

                 Linus

On Sat, Jul 08, 2017 at 12:44:54PM -0700, Linus Torvalds wrote:

Yeah, I think attributes are likely much better than some random crazy
ioctl interface. They can be listed with generic tools, and have
various scripting interfaces in ways that ioctl's do not sanely have.

I personally don't have a particular problem with these xattrs.  For
one thing, they are read-only.  You use them just to find out the AFS
cell, the AFS "fid", and the AFS volume name.

I think the place where people will start getting nervous is when we
start adding "write-only" xattrs or where writing to an xattr causes a
side-effect to take place.

So using xattr's to set AFS quota's for a volume, or to tell a client
about a new AFS cell, or to tell the client kernel about the database
servers for an AFS cell --- that I think would be pretty ugly.

Since such API's affect global state, and not a per-file state, I
don't believe xattrs are a good substitute for everything that was
done with the AFS pioctl system call for other operating systems.  For
those uses cases my personal opinion is that defining new Linux system
calls for such things would make a lot more sense.

Or maybe just use sysfs interfaces for such things, although that has
gotten abused in the past too....

      	       	      	    	       - Ted

On 7/8/2017 9:01 PM, Theodore Ts'o wrote:

On Sat, Jul 08, 2017 at 12:44:54PM -0700, Linus Torvalds wrote:

quoted

Yeah, I think attributes are likely much better than some random crazy
ioctl interface. They can be listed with generic tools, and have
various scripting interfaces in ways that ioctl's do not sanely have.

I personally don't have a particular problem with these xattrs.  For
one thing, they are read-only.  You use them just to find out the AFS
cell, the AFS "fid", and the AFS volume name.

I think the place where people will start getting nervous is when we
start adding "write-only" xattrs or where writing to an xattr causes a
side-effect to take place.

Ted,

The list of AFS pioctls and the proposed alternatives for kAFS are
listed at

  https://www.infradead.org/~dhowells/kafs/user_interface.html

While it is true that the majority of the proposed xattrs are read-only
properties of AFS objects (cell, volume, fid, servers, sec_class,
sec_mode) there is one exception that is read-write (acls).  AuriStorFS
permits acls to be set per-file; there was some per-file acl work begun
for OpenAFS but it was never completed.

Is there an alternative for fetching and setting ACLs that should be
considered?

Jeffrey Altman