Skip to content

Commit

Permalink
Implement File Attribute Support
Browse files Browse the repository at this point in the history
We add support for lsattr and chattr to resolve a regression caused by
88c2839 that broke Python's
xattr.list(). That changet broke Gentoo Portage's FEATURES=xattr, which
depended on Python's xattr.list().

Only attributes common to both Solaris and Linux are supported. These
are 'a', 'd' and 'i' in Linux's lsattr and chattr commands. File
attributes exclusive to Solaris are present in the ZFS code, but cannot
be accessed or modified through this method.  That was the case prior to
this patch. The resolution of issue openzfs#229 should implement
some method to permit access and modification of Solaris-specific
attributes.

https://bugs.gentoo.org/show_bug.cgi?id=483516
Issue openzfs#1691

Original-patch-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
  • Loading branch information
ryao committed Apr 25, 2014
1 parent de39ec1 commit 9160cdb
Show file tree
Hide file tree
Showing 5 changed files with 152 additions and 6 deletions.
30 changes: 30 additions & 0 deletions config/kernel-is_owner_or_cap.m4
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
dnl #
dnl # 2.6.39 API change
dnl # is_owner_or_cap() was renamed to inode_owner_or_capable().
dnl #
AC_DEFUN([ZFS_AC_KERNEL_INODE_OWNER_OR_CAPABLE], [
AC_MSG_CHECKING([whether inode_owner_or_capable() exists])
ZFS_LINUX_TRY_COMPILE([
#include <linux/fs.h>
],[
struct inode *ip = NULL;
inode_owner_or_capable(ip);
],[
AC_MSG_RESULT(yes)
AC_DEFINE(HAVE_INODE_OWNER_OR_CAPABLE, 1, [inode_owner_or_capable() exists])
],[
AC_MSG_RESULT(no)
AC_MSG_CHECKING([whether is_owner_or_cap() exists])
ZFS_LINUX_TRY_COMPILE([
#include <linux/fs.h>
],[
struct inode *ip = NULL;
is_owner_or_cap(ip);
],[
AC_MSG_RESULT(yes)
AC_DEFINE(HAVE_IS_OWNER_OR_CAP, 1, [is_owner_or_cap() exists])
],[
AC_MSG_ERROR(no; file a bug report with ZFSOnLinux)
])
])
])

This comment has been minimized.

Copy link
@behlendorf

behlendorf Apr 29, 2014

This test is redundant with ZFS_AC_KERNEL_INODE_OWNER_OR_CAPABLE in config/kernel-xattr-handler.m4. However, I do like yours better and now that there's a non xattr consumer it makes sense to move it to it's own m4 file.

1 change: 1 addition & 0 deletions config/kernel.m4
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ AC_DEFUN([ZFS_AC_CONFIG_KERNEL], [
ZFS_AC_KERNEL_5ARG_SGET
ZFS_AC_KERNEL_LSEEK_EXECUTE
ZFS_AC_KERNEL_VFS_ITERATE
ZFS_AC_KERNEL_INODE_OWNER_OR_CAPABLE
AS_IF([test "$LINUX_OBJ" != "$LINUX"], [
KERNELMAKE_PARAMS="$KERNELMAKE_PARAMS O=$LINUX_OBJ"
Expand Down
3 changes: 0 additions & 3 deletions include/linux/vfs_compat.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,6 @@ typedef int zpl_umode_t;
#define zpl_sget(type, cmp, set, fl, mtd) sget(type, cmp, set, mtd)
#endif /* HAVE_5ARG_SGET */

#define ZFS_IOC_GETFLAGS FS_IOC_GETFLAGS
#define ZFS_IOC_SETFLAGS FS_IOC_SETFLAGS

#if defined(SEEK_HOLE) && defined(SEEK_DATA) && !defined(HAVE_LSEEK_EXECUTE)
static inline loff_t
lseek_execute(
Expand Down
32 changes: 32 additions & 0 deletions module/zfs/zfs_znode.c
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,25 @@ zfs_znode_alloc(zfs_sb_t *zsb, dmu_buf_t *db, int blksz,
return (NULL);
}

void
zfs_set_inode_flags(znode_t *zp, struct inode *ip)
{
/*
* Linux and Solaris have different sets of file attributes, so we
* restrict this conversion to the intersection of the two.
*/

if (zp->z_pflags & ZFS_IMMUTABLE)
ip->i_flags |= S_IMMUTABLE;
else
ip->i_flags &= ~S_IMMUTABLE;

if (zp->z_pflags & ZFS_APPENDONLY)
ip->i_flags |= S_APPEND;
else
ip->i_flags &= ~S_APPEND;
}

/*
* Update the embedded inode given the znode. We should work toward
* eliminating this function as soon as possible by removing values
Expand Down Expand Up @@ -479,6 +498,7 @@ zfs_inode_update(znode_t *zp)
ip->i_gid = SGID_TO_KGID(zp->z_gid);
set_nlink(ip, zp->z_links);
ip->i_mode = zp->z_mode;
zfs_set_inode_flags(zp, ip);
ip->i_blkbits = SPA_MINBLOCKSHIFT;
dmu_object_size_from_db(sa_get_db(zp->z_sa_hdl), &blksize,
(u_longlong_t *)&ip->i_blocks);
Expand Down Expand Up @@ -849,6 +869,18 @@ zfs_xvattr_set(znode_t *zp, xvattr_t *xvap, dmu_tx_t *tx)
zp->z_pflags, tx);
XVA_SET_RTN(xvap, XAT_SPARSE);
}

/*
* The Solaris VFS gives filesystem drivers full control over how file
* attributes are stored inside the kernel, but the Linux VFS stores
* some of this inside the inode structure, so we must update it to
* keep things in sync.
*/
#ifdef __linux__
spin_lock(&ZTOI(zp)->i_lock);
zfs_set_inode_flags(zp, ZTOI(zp));
spin_unlock(&ZTOI(zp)->i_lock);
#endif

This comment has been minimized.

Copy link
@behlendorf

behlendorf Apr 29, 2014

Why is this needed? zfs_set_inode_flags should already be called on success through zfs_setattr->zfs_inode_update.

This comment has been minimized.

Copy link
@ryao

ryao Apr 30, 2014

Author Owner

It is not. My mistake.

}

int
Expand Down
92 changes: 89 additions & 3 deletions module/zfs/zpl_file.c
Original file line number Diff line number Diff line change
Expand Up @@ -520,13 +520,99 @@ zpl_fallocate(struct file *filp, int mode, loff_t offset, loff_t len)
}
#endif /* HAVE_FILE_FALLOCATE */

/*
* Map zfs file z_pflags (xvattr_t) to linux file attributes. Only file
* attributes common to both Linux and Solaris are mapped.
*/
static int
zpl_ioctl_getflags(struct file *filp, void __user *arg)
{
struct inode *ip = filp->f_dentry->d_inode;
unsigned int ioctl_flags = 0;
uint64_t zfs_flags = ITOZ(ip)->z_pflags;
int error;

if (zfs_flags & ZFS_IMMUTABLE)
ioctl_flags |= FS_IMMUTABLE_FL;

if (zfs_flags & ZFS_APPENDONLY)
ioctl_flags |= FS_APPEND_FL;

if (zfs_flags & ZFS_NODUMP)
ioctl_flags |= FS_NODUMP_FL;

ioctl_flags &= FS_FL_USER_VISIBLE;

error = copy_to_user(arg, &ioctl_flags, sizeof (ioctl_flags));

return (error);
}

#define fchange(f0, f1, b0, b1) ((((f0) & (b0)) == (b0)) != \
(((b1) & (f1)) == (f1)))

This comment has been minimized.

Copy link
@behlendorf

behlendorf Apr 29, 2014

Personally I find the use of this macro more confusing than helpful. Could it perhaps be reworked to be clearer, if not definitely add a comment explaining what it does and why it's here.

This comment has been minimized.

Copy link
@ryao

ryao Apr 30, 2014

Author Owner

I suppose it is possible to use a debrujin sequence with a lookup table to simplify this, but that would likely be even harder to understand and is probably overkill for an O(1) operation. There seems to be no clearly readable way of handling this. A comment is probably the best we can do.

This comment has been minimized.

Copy link
@behlendorf

behlendorf Apr 30, 2014

Adding the comment is fine.


static int
zpl_ioctl_setflags(struct file *filp, void __user *arg)
{
struct inode *ip = filp->f_dentry->d_inode;
uint64_t zfs_flags = ITOZ(ip)->z_pflags;
unsigned int ioctl_flags;
cred_t *cr = CRED();
xvattr_t xva;
xoptattr_t *xoap;
int error;

if (copy_from_user(&ioctl_flags, arg, sizeof (ioctl_flags)))
return (-EFAULT);

if ((ioctl_flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | FS_NODUMP_FL)))
return (-EOPNOTSUPP);

if ((ioctl_flags & ~(FS_FL_USER_MODIFIABLE)))
return (-EACCES);

if ((fchange(ioctl_flags, zfs_flags, FS_IMMUTABLE_FL, ZFS_IMMUTABLE) ||
fchange(ioctl_flags, zfs_flags, FS_APPEND_FL, ZFS_APPENDONLY)) &&
!capable(CAP_LINUX_IMMUTABLE))
return (-EACCES);

#ifdef HAVE_INODE_OWNER_OR_CAPABLE
if (!inode_owner_or_capable(ip))
#else
if (!is_owner_or_cap(ip))
#endif

This comment has been minimized.

Copy link
@behlendorf

behlendorf Apr 29, 2014

Please use the existing zpl_inode_owner_or_capable() wrapper for this check.

return (-EACCES);

xva_init(&xva);
xoap = xva_getxoptattr(&xva);

XVA_SET_REQ(&xva, XAT_IMMUTABLE);
if (ioctl_flags & FS_IMMUTABLE_FL)
xoap->xoa_immutable = B_TRUE;

XVA_SET_REQ(&xva, XAT_APPENDONLY);
if (ioctl_flags & FS_APPEND_FL)
xoap->xoa_appendonly = B_TRUE;

XVA_SET_REQ(&xva, XAT_NODUMP);
if (ioctl_flags & FS_NODUMP_FL)
xoap->xoa_nodump = B_TRUE;

crhold(cr);
error = -zfs_setattr(ip, (vattr_t *)&xva, 0, cr);
crfree(cr);

return (error);
}

static long
zpl_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
switch (cmd) {
case ZFS_IOC_GETFLAGS:
case ZFS_IOC_SETFLAGS:
return (-EOPNOTSUPP);
case FS_IOC_GETFLAGS:
return (zpl_ioctl_getflags(filp, (void *)arg));
case FS_IOC_SETFLAGS:
return (zpl_ioctl_setflags(filp, (void *)arg));
default:
return (-ENOTTY);
}
Expand Down

0 comments on commit 9160cdb

Please sign in to comment.