* Re: [PATCH 2/2] afs: Add metadata xattrs [not found] ` <149935262759.29744.6299062653432480230.stgit@warthog.procyon.org.uk> @ 2017-07-06 15:23 ` Christoph Hellwig 2017-07-06 16:14 ` David Howells 1 sibling, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2017-07-06 15:23 UTC (permalink / raw) To: David Howells; +Cc: torvalds, linux-fsdevel, linux-afs, linux-kernel, linux-api 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: > Add xattrs to allow the user to get/set metadata in lieu of having pioctl() > available. The following xattrs are now available: > > (*) afs.cell > > The name of the cell in which the vnode's volume resides. > > (*) afs.fid > > The volume ID, vnode ID and vnode uniquifier of the file as three hex > numbers separated by colons. > > (*) afs.volume > > The name of the volume in which the vnode resides. > > For example: > > # getfattr -d -m ".*" /mnt/scratch > getfattr: Removing leading '/' from absolute path names > # file: mnt/scratch > afs.cell="mycell.myorg.org" > afs.fid="10000b:1:1" > afs.volume="scratch" > > Signed-off-by: David Howells <dhowells@redhat.com> > --- > > fs/afs/Makefile | 3 + > fs/afs/dir.c | 1 > fs/afs/file.c | 1 > fs/afs/inode.c | 7 +++ > fs/afs/internal.h | 5 ++ > fs/afs/mntpt.c | 1 > fs/afs/super.c | 1 > fs/afs/xattr.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 8 files changed, 138 insertions(+), 2 deletions(-) > create mode 100644 fs/afs/xattr.c > > diff --git a/fs/afs/Makefile b/fs/afs/Makefile > index 4f64b95d57bd..095c54165dfd 100644 > --- a/fs/afs/Makefile > +++ b/fs/afs/Makefile > @@ -27,6 +27,7 @@ kafs-objs := \ > vlocation.o \ > vnode.o \ > volume.o \ > - write.o > + write.o \ > + xattr.o > > obj-$(CONFIG_AFS_FS) := kafs.o > diff --git a/fs/afs/dir.c b/fs/afs/dir.c > index 949f960337f5..613a77058263 100644 > --- a/fs/afs/dir.c > +++ b/fs/afs/dir.c > @@ -61,6 +61,7 @@ const struct inode_operations afs_dir_inode_operations = { > .permission = afs_permission, > .getattr = afs_getattr, > .setattr = afs_setattr, > + .listxattr = afs_listxattr, > }; > > const struct dentry_operations afs_fs_dentry_operations = { > diff --git a/fs/afs/file.c b/fs/afs/file.c > index 0d5b8508869b..510cba15fa56 100644 > --- a/fs/afs/file.c > +++ b/fs/afs/file.c > @@ -46,6 +46,7 @@ const struct inode_operations afs_file_inode_operations = { > .getattr = afs_getattr, > .setattr = afs_setattr, > .permission = afs_permission, > + .listxattr = afs_listxattr, > }; > > const struct address_space_operations afs_fs_aops = { > diff --git a/fs/afs/inode.c b/fs/afs/inode.c > index aae55dd15108..342316a9e3e0 100644 > --- a/fs/afs/inode.c > +++ b/fs/afs/inode.c > @@ -28,6 +28,11 @@ struct afs_iget_data { > struct afs_volume *volume; /* volume on which resides */ > }; > > +static const struct inode_operations afs_symlink_inode_operations = { > + .get_link = page_get_link, > + .listxattr = afs_listxattr, > +}; > + > /* > * map the AFS file status to the inode member variables > */ > @@ -67,7 +72,7 @@ static int afs_inode_map_status(struct afs_vnode *vnode, struct key *key) > inode->i_fop = &afs_mntpt_file_operations; > } else { > inode->i_mode = S_IFLNK | vnode->status.mode; > - inode->i_op = &page_symlink_inode_operations; > + inode->i_op = &afs_symlink_inode_operations; > } > inode_nohighmem(inode); > break; > diff --git a/fs/afs/internal.h b/fs/afs/internal.h > index 4e2556606623..82e16556afea 100644 > --- a/fs/afs/internal.h > +++ b/fs/afs/internal.h > @@ -731,6 +731,11 @@ extern int afs_writeback_all(struct afs_vnode *); > extern int afs_flush(struct file *, fl_owner_t); > extern int afs_fsync(struct file *, loff_t, loff_t, int); > > +/* > + * xattr.c > + */ > +extern const struct xattr_handler *afs_xattr_handlers[]; > +extern ssize_t afs_listxattr(struct dentry *, char *, size_t); > > /*****************************************************************************/ > /* > diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c > index bd3b65cde282..690fea9d84c3 100644 > --- a/fs/afs/mntpt.c > +++ b/fs/afs/mntpt.c > @@ -35,6 +35,7 @@ const struct inode_operations afs_mntpt_inode_operations = { > .lookup = afs_mntpt_lookup, > .readlink = page_readlink, > .getattr = afs_getattr, > + .listxattr = afs_listxattr, > }; > > const struct inode_operations afs_autocell_inode_operations = { > diff --git a/fs/afs/super.c b/fs/afs/super.c > index c79633e5cfd8..67680c2d96cf 100644 > --- a/fs/afs/super.c > +++ b/fs/afs/super.c > @@ -319,6 +319,7 @@ static int afs_fill_super(struct super_block *sb, > sb->s_blocksize_bits = PAGE_SHIFT; > sb->s_magic = AFS_FS_MAGIC; > sb->s_op = &afs_super_ops; > + sb->s_xattr = afs_xattr_handlers; > ret = super_setup_bdi(sb); > if (ret) > return ret; > diff --git a/fs/afs/xattr.c b/fs/afs/xattr.c > new file mode 100644 > index 000000000000..2830e4f48d85 > --- /dev/null > +++ b/fs/afs/xattr.c > @@ -0,0 +1,121 @@ > +/* Extended attribute handling for AFS. We use xattrs to get and set metadata > + * instead of providing pioctl(). > + * > + * Copyright (C) 2017 Red Hat, Inc. All Rights Reserved. > + * Written by David Howells (dhowells@redhat.com) > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public Licence > + * as published by the Free Software Foundation; either version > + * 2 of the Licence, or (at your option) any later version. > + */ > + > +#include <linux/slab.h> > +#include <linux/fs.h> > +#include <linux/xattr.h> > +#include "internal.h" > + > +static const char afs_xattr_list[] = > + "afs.cell\0" > + "afs.fid\0" > + "afs.volume"; > + > +/* > + * Retrieve a list of the supported xattrs. > + */ > +ssize_t afs_listxattr(struct dentry *dentry, char *buffer, size_t size) > +{ > + if (size == 0) > + return sizeof(afs_xattr_list); > + if (size < sizeof(afs_xattr_list)) > + return -ERANGE; > + memcpy(buffer, afs_xattr_list, sizeof(afs_xattr_list)); > + return sizeof(afs_xattr_list); > +} > + > +/* > + * Get the name of the cell on which a file resides. > + */ > +static int afs_xattr_get_cell(const struct xattr_handler *handler, > + struct dentry *dentry, > + struct inode *inode, const char *name, > + void *buffer, size_t size) > +{ > + struct afs_vnode *vnode = AFS_FS_I(inode); > + struct afs_cell *cell = vnode->volume->cell; > + size_t namelen; > + > + namelen = strlen(cell->name); > + if (size == 0) > + return namelen; > + if (namelen > size) > + return -ERANGE; > + memcpy(buffer, cell->name, size); > + return namelen; > +} > + > +static const struct xattr_handler afs_xattr_afs_cell_handler = { > + .name = "afs.cell", > + .get = afs_xattr_get_cell, > +}; > + > +/* > + * Get the volume ID, vnode ID and vnode uniquifier of a file as a sequence of > + * hex numbers separated by colons. > + */ > +static int afs_xattr_get_fid(const struct xattr_handler *handler, > + struct dentry *dentry, > + struct inode *inode, const char *name, > + void *buffer, size_t size) > +{ > + struct afs_vnode *vnode = AFS_FS_I(inode); > + char text[8 + 1 + 8 + 1 + 8 + 1]; > + size_t len; > + > + len = sprintf(text, "%x:%x:%x", > + vnode->fid.vid, vnode->fid.vnode, vnode->fid.unique); > + if (size == 0) > + return len; > + if (len > size) > + return -ERANGE; > + memcpy(buffer, text, len); > + return len; > +} > + > +static const struct xattr_handler afs_xattr_afs_fid_handler = { > + .name = "afs.fid", > + .get = afs_xattr_get_fid, > +}; > + > +/* > + * Get the name of the volume on which a file resides. > + */ > +static int afs_xattr_get_volume(const struct xattr_handler *handler, > + struct dentry *dentry, > + struct inode *inode, const char *name, > + void *buffer, size_t size) > +{ > + struct afs_vnode *vnode = AFS_FS_I(inode); > + const char *volname = vnode->volume->vlocation->vldb.name; > + size_t namelen; > + > + namelen = strlen(volname); > + if (size == 0) > + return namelen; > + if (namelen > size) > + return -ERANGE; > + memcpy(buffer, volname, size); > + return namelen; > +} > + > +static const struct xattr_handler afs_xattr_afs_volume_handler = { > + .name = "afs.volume", > + .get = afs_xattr_get_volume, > +}; > + > +const struct xattr_handler *afs_xattr_handlers[] = { > + &afs_xattr_afs_cell_handler, > + &afs_xattr_afs_fid_handler, > + &afs_xattr_afs_volume_handler, > + NULL > +}; > > > _______________________________________________ > linux-afs mailing list > http://lists.infradead.org/mailman/listinfo/linux-afs ---end quoted text--- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] afs: Add metadata xattrs [not found] ` <149935262759.29744.6299062653432480230.stgit@warthog.procyon.org.uk> 2017-07-06 15:23 ` [PATCH 2/2] afs: Add metadata xattrs Christoph Hellwig @ 2017-07-06 16:14 ` David Howells 2017-07-06 18:27 ` Andreas Dilger 1 sibling, 1 reply; 6+ messages in thread From: David Howells @ 2017-07-06 16:14 UTC (permalink / raw) To: Christoph Hellwig Cc: dhowells, viro, linux-fsdevel, linux-api, torvalds, linux-afs, linux-kernel Christoph Hellwig <hch@infradead.org> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] afs: Add metadata xattrs 2017-07-06 16:14 ` David Howells @ 2017-07-06 18:27 ` Andreas Dilger 2017-07-08 19:44 ` Linus Torvalds 0 siblings, 1 reply; 6+ messages in thread From: Andreas Dilger @ 2017-07-06 18:27 UTC (permalink / raw) To: David Howells Cc: Christoph Hellwig, Alexander Viro, linux-fsdevel, linux-api, torvalds, linux-afs, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1844 bytes --] On Jul 6, 2017, at 10:14 AM, David Howells <dhowells@redhat.com> wrote: > > Christoph Hellwig <hch@infradead.org> 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. 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 [-- Attachment #2: Message signed with OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] afs: Add metadata xattrs 2017-07-06 18:27 ` Andreas Dilger @ 2017-07-08 19:44 ` Linus Torvalds 2017-07-09 1:01 ` Theodore Ts'o 0 siblings, 1 reply; 6+ messages in thread From: Linus Torvalds @ 2017-07-08 19:44 UTC (permalink / raw) To: Andreas Dilger Cc: David Howells, Christoph Hellwig, Alexander Viro, linux-fsdevel, Linux API, linux-afs, Linux Kernel Mailing List On Thu, Jul 6, 2017 at 11:27 AM, Andreas Dilger <adilger@dilger.ca> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] afs: Add metadata xattrs 2017-07-08 19:44 ` Linus Torvalds @ 2017-07-09 1:01 ` Theodore Ts'o [not found] ` <20170709010155.3nql5ixdeozemgfd-AKGzg7BKzIDYtjvyW6yDsg@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Theodore Ts'o @ 2017-07-09 1:01 UTC (permalink / raw) To: Linus Torvalds Cc: Andreas Dilger, David Howells, Christoph Hellwig, Alexander Viro, linux-fsdevel, Linux API, linux-afs, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <20170709010155.3nql5ixdeozemgfd-AKGzg7BKzIDYtjvyW6yDsg@public.gmane.org>]
* Re: [PATCH 2/2] afs: Add metadata xattrs [not found] ` <20170709010155.3nql5ixdeozemgfd-AKGzg7BKzIDYtjvyW6yDsg@public.gmane.org> @ 2017-07-09 2:37 ` Jeffrey Altman 0 siblings, 0 replies; 6+ messages in thread From: Jeffrey Altman @ 2017-07-09 2:37 UTC (permalink / raw) To: Theodore Ts'o, Linus Torvalds, Andreas Dilger, David Howells, Christoph Hellwig, Alexander Viro, linux-fsdevel, Linux API, linux-afs-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linux Kernel Mailing List [-- Attachment #1.1: Type: text/plain, Size: 1301 bytes --] On 7/8/2017 9:01 PM, Theodore Ts'o wrote: > 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. 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 [-- Attachment #1.2: jaltman.vcf --] [-- Type: text/x-vcard, Size: 437 bytes --] begin:vcard fn:Jeffrey Altman n:Altman;Jeffrey org:AuriStor, Inc. adr:Suite 6B;;255 West 94Th Street;New York;New York;10025-6985;United States email;internet:jaltman-hRzEac23uH1Wk0Htik3J/w@public.gmane.org title:Founder and CEO tel;work:+1-212-769-9018 note;quoted-printable:LinkedIn: https://www.linkedin.com/in/jeffreyaltman=0D=0A= Skype: jeffrey.e.altman=0D=0A= url:https://www.auristor.com/ version:2.1 end:vcard [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 4057 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-09 2:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <149935261019.29744.8564287571048506851.stgit@warthog.procyon.org.uk> [not found] ` <149935262759.29744.6299062653432480230.stgit@warthog.procyon.org.uk> 2017-07-06 15:23 ` [PATCH 2/2] afs: Add metadata xattrs Christoph Hellwig 2017-07-06 16:14 ` David Howells 2017-07-06 18:27 ` Andreas Dilger 2017-07-08 19:44 ` Linus Torvalds 2017-07-09 1:01 ` Theodore Ts'o [not found] ` <20170709010155.3nql5ixdeozemgfd-AKGzg7BKzIDYtjvyW6yDsg@public.gmane.org> 2017-07-09 2:37 ` Jeffrey Altman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).