On Wed, 2 Mar 2011, Eric Paris wrote:
> I know there exist thoughts on this patch somewhere on the internets.
> Let 'em rip! I can handle it!
>
> -Eric
>
> On Thu, Feb 17, 2011 at 4:27 PM, Eric Paris <eparis@xxxxxxxxxxxxxx> wrote:
> > Bueller? Bueller? Any thoughts? Any problems?
> >
Sorry, Eric, I did spot it months ago, kept on picking it up and
putting it down, never quite got to grips with it. I did try it out,
and so far as I could tell, it was working correctly.
> > On Tue, Jan 11, 2011 at 4:07 PM, Eric Paris <eparis@xxxxxxxxxx> wrote:
> >> This patch implements security.capability xattrs for tmpfs filesystems. The
> >> feodra project, while trying to replace suid apps with file capabilities,
> >> realized that tmpfs, which is used on my build systems, does not support file
> >> capabilities and thus cannot be used to build packages which use file
> >> capabilities. The patch only implements security.capability but there is no
> >> reason it could not be easily expanded to support *.* xattrs as most of the
> >> work is already done. I don't know what other xattrs are in use in the world
> >> or if they necessarily make sense on tmpfs so I didn't make this
> >> implementation completely generic.
> >>
> >> The basic implementation is that I attach a
> >> struct shmem_xattr {
> >> struct list_head list; /* anchored by shmem_inode_info->xattr_list */
> >> char *name;
> >> size_t size;
> >> char value[0];
> >> };
> >> Into the struct shmem_inode_info for each xattr that is set. Since I only
> >> allow security.capability obviously this list is only every 0 or 1 entry long.
> >> I could have been a little simpler, but then the next person having to
> >> implement an xattr would have to redo everything I did instead of me just
> >> doing 90% of their work :)
> >>
> >> Signed-off-by: Eric Paris <eparis@xxxxxxxxxx>
I'm unfamiliar with xattrs, and found the security hooks, the way we
dip into and out of them, quite confusing: not to mean that you need
to add lots of comments, no, so long as it works, and is what people
familiar the territory expect, that's okay.
We do like tmpfs to be useful, but it was unclear to me from your
comments above, whether this is just a toy implementation good for
packaging, or a real implementation of security.capability. I hope
the latter - we do not want something half-baked that will cause
trouble by breaking expectations down the line.
If you can get Acks from James and Christoph, both of whom have been
here before, then it's mostly fine by me; but a few comments below.
> >> ---
> >>
> >> include/linux/shmem_fs.h | 8 +++
> >> mm/shmem.c | 112 ++++++++++++++++++++++++++++++++++++++++++++--
> >> 2 files changed, 116 insertions(+), 4 deletions(-)
No change to fs/Kconfig? You seem to smuggle the xattr and security
support in under CONFIG_TMPFS_POSIX_ACL, and leave it unsupported
without. It's probably a fair assumption that the people with that
option selected are the people who will be interested in this, so
no need for the maze of separate config options which a grownup
filesystem would have here. But at the very least you need to say
more in the TMPFS_POSIX_ACL Kconfig entry (a new name may be more
trouble than it's worth).
> >>
> >> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> >> index 399be5a..6f2ebb8 100644
> >> --- a/include/linux/shmem_fs.h
> >> +++ b/include/linux/shmem_fs.h
> >> @@ -9,6 +9,13 @@
> >>
> >> #define SHMEM_NR_DIRECT 16
> >>
> >> +struct shmem_xattr {
> >> + struct list_head list; /* anchored by shmem_inode_info->xattr_list */
> >> + char *name;
> >> + size_t size;
> >> + char value[0];
> >> +};
> >> +
> >> struct shmem_inode_info {
> >> spinlock_t lock;
> >> unsigned long flags;
> >> @@ -19,6 +26,7 @@ struct shmem_inode_info {
> >> struct page *i_indirect; /* top indirect blocks page */
> >> swp_entry_t i_direct[SHMEM_NR_DIRECT]; /* first blocks */
> >> struct list_head swaplist; /* chain of maybes on swap */
> >> + struct list_head xattr_list; /* list of shmem_xattr */
> >> struct inode vfs_inode;
> >> };
> >>
> >> diff --git a/mm/shmem.c b/mm/shmem.c
> >> index 86cd21d..d2bacd6 100644
> >> --- a/mm/shmem.c
> >> +++ b/mm/shmem.c
> >> @@ -822,6 +822,7 @@ static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
> >> static void shmem_evict_inode(struct inode *inode)
> >> {
> >> struct shmem_inode_info *info = SHMEM_I(inode);
> >> + struct shmem_xattr *xattr, *nxattr;
> >>
> >> if (inode->i_mapping->a_ops == &shmem_aops) {
> >> truncate_inode_pages(inode->i_mapping, 0);
> >> @@ -834,6 +835,9 @@ static void shmem_evict_inode(struct inode *inode)
> >> mutex_unlock(&shmem_swaplist_mutex);
> >> }
> >> }
> >> +
> >> + list_for_each_entry_safe(xattr, nxattr, &info->xattr_list, list)
> >> + kfree(xattr);
> >> BUG_ON(inode->i_blocks);
> >> shmem_free_inode(inode->i_sb);
> >> end_writeback(inode);
> >> @@ -1597,6 +1601,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
> >> spin_lock_init(&info->lock);
> >> info->flags = flags & VM_NORESERVE;
> >> INIT_LIST_HEAD(&info->swaplist);
> >> + INIT_LIST_HEAD(&info->xattr_list);
> >> cache_no_acl(inode);
> >>
> >> switch (mode & S_IFMT) {
> >> @@ -2071,24 +2076,123 @@ static size_t shmem_xattr_security_list(struct dentry *dentry, char *list,
> >> size_t list_len, const char *name,
> >> size_t name_len, int handler_flags)
> >> {
> >> - return security_inode_listsecurity(dentry->d_inode, list, list_len);
> >> + struct shmem_xattr *xattr;
> >> + struct shmem_inode_info *shmem_i;
It's a nit, but (almost) everywhere else in shmem.c the shmem_inode_info
pointer is known as "info": easy for me to fix up if I care, but nicer
if you follow local custom.
> >> + size_t used;
> >> + char *buf = NULL;
> >> +
> >> + used = security_inode_listsecurity(dentry->d_inode, list, list_len);
> >> +
> >> + shmem_i = SHMEM_I(dentry->d_inode);
> >> + if (list)
> >> + buf = list + used;
This is the place that caused me most trouble. On a minor note:
it worried me that security_inode_listsecurity() might return an
error, whereas I think you know and assume that the worst it can
return is 0 - might be worth a comment.
But more major: I found it very odd that you collect one set of things
from security_inode_listsecurity(), then proceed to tack on some more
below from the shmem inode. I looked at other filesystems (well, ext2!)
and couldn't find a precedent. What's this about? Is it because other
filesystems have an on-disk format which determines what they're capable
of, whereas tmpfs is plastic and can reflect what the running system has?
Or is it to allow for future xattrs which might be added to tmpfs, but
frankly I'd rather do without until they're defined?
If it needs to be like this, then please, I do want a comment on
what's going on here. If it need not be like this, then please
delete what's not needed.
> >> +
> >> + spin_lock(&dentry->d_inode->i_lock);
> >> + list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> + size_t len = XATTR_SECURITY_PREFIX_LEN;
> >> + len += strlen(xattr->name) + 1;
> >> + if (list_len - (used + len) >= 0 && buf) {
> >> + strncpy(buf, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN);
> >> + buf += XATTR_SECURITY_PREFIX_LEN;
> >> + strncpy(buf, xattr->name, strlen(xattr->name) + 1);
> >> + buf += strlen(xattr->name) + 1;
> >> + }
> >> + used += len;
> >> + }
> >> + spin_unlock(&dentry->d_inode->i_lock);
> >> +
> >> + return used;
> >> }
> >>
> >> static int shmem_xattr_security_get(struct dentry *dentry, const char *name,
> >> void *buffer, size_t size, int handler_flags)
> >> {
> >> + struct shmem_inode_info *shmem_i;
"info" as above.
> >> + struct shmem_xattr *xattr;
> >> + int ret;
> >> +
> >> if (strcmp(name, "") == 0)
> >> return -EINVAL;
> >> - return xattr_getsecurity(dentry->d_inode, name, buffer, size);
> >> +
> >> + ret = xattr_getsecurity(dentry->d_inode, name, buffer, size);
> >> + if (ret != -EOPNOTSUPP)
> >> + return ret;
> >> +
> >> + /* if we make this generic this needs to go... */
> >> + if (strcmp(name, XATTR_CAPS_SUFFIX))
> >> + return -EOPNOTSUPP;
> >> +
> >> + ret = -ENODATA;
> >> + shmem_i = SHMEM_I(dentry->d_inode);
> >> +
> >> + spin_lock(&dentry->d_inode->i_lock);
> >> + list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> + if (!strcmp(name, xattr->name)) {
> >> + ret = xattr->size;
> >> + if (buffer) {
> >> + if (size < xattr->size)
> >> + ret = -ERANGE;
> >> + else
> >> + memcpy(buffer, xattr->value, xattr->size);
> >> + }
> >> + break;
> >> + }
> >> + }
> >> + spin_unlock(&dentry->d_inode->i_lock);
> >> + return ret;
> >> }
> >>
> >> static int shmem_xattr_security_set(struct dentry *dentry, const char *name,
> >> const void *value, size_t size, int flags, int handler_flags)
> >> {
> >> + int ret;
> >> + struct inode *inode = dentry->d_inode;
> >> + struct shmem_inode_info *shmem_i = SHMEM_I(inode);
"info" as above.
> >> + struct shmem_xattr *xattr;
> >> + struct shmem_xattr *new_xattr;
> >> + size_t len;
> >> +
> >> if (strcmp(name, "") == 0)
> >> return -EINVAL;
> >> - return security_inode_setsecurity(dentry->d_inode, name, value,
> >> - size, flags);
> >> + ret = security_inode_setsecurity(inode, name, value, size, flags);
> >> + if (ret != -EOPNOTSUPP)
> >> + return ret;
> >> +
> >> + /*
> >> + * We only store fcaps for now, but this could be a lot more generic.
> >> + * We could hold the prefix as well as the suffix in the xattr struct
> >> + * We would also need to hold a copy of the suffix rather than a
> >> + * pointer to XATTR_CAPS_SUFFIX
> >> + */
> >> + if (strcmp(name, XATTR_CAPS_SUFFIX))
> >> + return -EOPNOTSUPP;
> >> +
> >> + /* wrap around? */
> >> + len = sizeof(*new_xattr) + size;
> >> + if (len <= sizeof(*new_xattr))
> >> + return -ENOMEM;
> >> +
> >> + new_xattr = kmalloc(GFP_NOFS, len);
> >> + if (!new_xattr)
> >> + return -ENOMEM;
> >> +
> >> + new_xattr->name = XATTR_CAPS_SUFFIX;
> >> + new_xattr->size = size;
> >> + memcpy(new_xattr->value, value, size);
> >> +
> >> + spin_lock(&inode->i_lock);
> >> + list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> + if (!strcmp(name, xattr->name)) {
> >> + list_replace(&xattr->list, &new_xattr->list);
> >> + goto out;
> >> + }
> >> + }
> >> + list_add(&new_xattr->list, &shmem_i->xattr_list);
> >> + xattr = NULL;
> >> +out:
> >> + spin_unlock(&inode->i_lock);
> >> + kfree(xattr);
> >> + return 0;
> >> }
> >>
> >> static const struct xattr_handler shmem_xattr_security_handler = {
I'm sorry if my incomprehension depresses you: it did me!
You'll laugh or cry if I admit to you that I was naive enough
to believe that comment above shmem_xattr_security_list() which says
* Superblocks without xattr inode operations will get security.* xattr
* support from the VFS "for free".
and wondered why you had to add any code at all. Maybe you could say
something better there.
Thanks,
Hugh
