Trond, I've discovered an asymmetry in how nfs4 (in v2.6.24-rc7) handles
listxattr vs. setxattr for special files. I caught this with Unionfs when
testing a copyup of a character device from a readonly nfs4 mount to a
writable nfs4 mount.
nfs4_setxattr returns -EPERM if the inode isn't a regular file or a (sticky)
dir. However, nfs4_listxattr returns XATTR_NAME_NFSV4_ACL (16 bytes,
"system.nfs4_acl") regardless of the type of the inode. So during copyup, I
can ->listxattr fine from the source inode, but I get EPERM trying to
->setxattr on the destination branch of of the copyup. (I already handle
the case when copying-up from a f/s which supports xattr's to one which
doesn't, but when both file systems are the same and mounted the same, there
typically should not be difference).
Assuming that the original intent of the EPERM check in nfs4_setxattr was to
prevent setting xattrs over nfs4 for non-files/dirs, then I "fixed" this by
adding a similar check in nfs4_listxattr, which returns an empty xattr list
for those. I've included a patch below for your consideration/review.
Cheers,
Erez.
--cut-here----cut-here----cut-here----cut-here----cut-here----cut-here--
NFS4: ignore non files/dirs in nfs4_getxattr, same as nfs4_listxattr
Signed-off-by: Erez Zadok <[email protected]>
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9e2e1c7..ad46fef 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3656,9 +3656,13 @@ ssize_t nfs4_getxattr(struct dentry *dentry, const char *key, void *buf,
ssize_t nfs4_listxattr(struct dentry *dentry, char *buf, size_t buflen)
{
size_t len = strlen(XATTR_NAME_NFSV4_ACL) + 1;
+ struct inode *inode = dentry->d_inode;
if (!nfs4_server_supports_acls(NFS_SERVER(dentry->d_inode)))
return 0;
+ if (!S_ISREG(inode->i_mode) &&
+ (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
+ return 0;
if (buf && buflen < len)
return -ERANGE;
if (buf)
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
In message <[email protected]>, "J. Bruce Fields" writes:
> On Tue, Jan 08, 2008 at 04:26:29PM -0500, Erez Zadok wrote:
> > Yes, it works equally well for me. Thanks.
>
> OK, thanks.
>
> > Acked-by: Erez Zadok <[email protected]>
>
> Maybe that should be Reviewed-by? Tested-by? Up to Trond, I guess.
Sure, whatever makes sense: Happy-To-Report-It-Worked-For-Him-By ? :-)
> Trond, could you apply?
Erez.
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
On Mon, 2008-01-07 at 22:14 -0500, Erez Zadok wrote:
> Trond, I've discovered an asymmetry in how nfs4 (in v2.6.24-rc7) handles
> listxattr vs. setxattr for special files. I caught this with Unionfs when
> testing a copyup of a character device from a readonly nfs4 mount to a
> writable nfs4 mount.
>
> nfs4_setxattr returns -EPERM if the inode isn't a regular file or a (sticky)
> dir. However, nfs4_listxattr returns XATTR_NAME_NFSV4_ACL (16 bytes,
> "system.nfs4_acl") regardless of the type of the inode. So during copyup, I
> can ->listxattr fine from the source inode, but I get EPERM trying to
> ->setxattr on the destination branch of of the copyup. (I already handle
> the case when copying-up from a f/s which supports xattr's to one which
> doesn't, but when both file systems are the same and mounted the same, there
> typically should not be difference).
>
> Assuming that the original intent of the EPERM check in nfs4_setxattr was to
> prevent setting xattrs over nfs4 for non-files/dirs, then I "fixed" this by
> adding a similar check in nfs4_listxattr, which returns an empty xattr list
> for those. I've included a patch below for your consideration/review.
I can see no reason in the RFC why you shouldn't be able to set an NFSv4
ACL on a special file. AFAICS this is a bug in the nfs4_setxattr code,
which is probably due to some improper cut n' pasting from the posix acl
code.
Bruce?
Cheers
Trond
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
On Mon, Jan 07, 2008 at 10:34:04PM -0500, Trond Myklebust wrote:
>
> On Mon, 2008-01-07 at 22:14 -0500, Erez Zadok wrote:
> > Trond, I've discovered an asymmetry in how nfs4 (in v2.6.24-rc7) handles
> > listxattr vs. setxattr for special files. I caught this with Unionfs when
> > testing a copyup of a character device from a readonly nfs4 mount to a
> > writable nfs4 mount.
> >
> > nfs4_setxattr returns -EPERM if the inode isn't a regular file or a (sticky)
> > dir. However, nfs4_listxattr returns XATTR_NAME_NFSV4_ACL (16 bytes,
> > "system.nfs4_acl") regardless of the type of the inode. So during copyup, I
> > can ->listxattr fine from the source inode, but I get EPERM trying to
> > ->setxattr on the destination branch of of the copyup. (I already handle
> > the case when copying-up from a f/s which supports xattr's to one which
> > doesn't, but when both file systems are the same and mounted the same, there
> > typically should not be difference).
> >
> > Assuming that the original intent of the EPERM check in nfs4_setxattr was to
> > prevent setting xattrs over nfs4 for non-files/dirs, then I "fixed" this by
> > adding a similar check in nfs4_listxattr, which returns an empty xattr list
> > for those. I've included a patch below for your consideration/review.
>
> I can see no reason in the RFC why you shouldn't be able to set an NFSv4
> ACL on a special file. AFAICS this is a bug in the nfs4_setxattr code,
> which is probably due to some improper cut n' pasting from the posix acl
> code.
>
> Bruce?
I think you're right.
I'm also mystified by this rule that the sticky bit prevents non-owners
from writing a directory's extended attributes. (See fs/xattr.c.) Any
idea what the source of that is? Anyway, I don't see why the client
should enforce such a rule in this case.
Erez, does just removing this check solve your problem?
--b.
>From 9fa16668ed09b212ec0325786d91ac65734336ea Mon Sep 17 00:00:00 2001
From: J. Bruce Fields <[email protected]>
Date: Tue, 8 Jan 2008 12:41:41 -0500
Subject: [PATCH] nfs4: allow nfsv4 acls on non-regular-files
The rfc doesn't give any reason it shouldn't be possible to set an
attribute on a non-regular file. And if the server supports it, then it
shouldn't be up to us to prevent it.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfs/nfs4proc.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f03d9d5..e9b9903 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3625,10 +3625,6 @@ int nfs4_setxattr(struct dentry *dentry, const char *key, const void *buf,
if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
return -EOPNOTSUPP;
- if (!S_ISREG(inode->i_mode) &&
- (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
- return -EPERM;
-
return nfs4_proc_set_acl(inode, buf, buflen);
}
--
1.5.4.rc2.60.gb2e62
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
In message <[email protected]>, "J. Bruce Fields" writes:
> On Mon, Jan 07, 2008 at 10:34:04PM -0500, Trond Myklebust wrote:
> >
> > On Mon, 2008-01-07 at 22:14 -0500, Erez Zadok wrote:
> > > Trond, I've discovered an asymmetry in how nfs4 (in v2.6.24-rc7) handles
> > > listxattr vs. setxattr for special files. I caught this with Unionfs when
> > > testing a copyup of a character device from a readonly nfs4 mount to a
> > > writable nfs4 mount.
> > >
> > > nfs4_setxattr returns -EPERM if the inode isn't a regular file or a (sticky)
> > > dir. However, nfs4_listxattr returns XATTR_NAME_NFSV4_ACL (16 bytes,
> > > "system.nfs4_acl") regardless of the type of the inode. So during copyup, I
> > > can ->listxattr fine from the source inode, but I get EPERM trying to
> > > ->setxattr on the destination branch of of the copyup. (I already handle
> > > the case when copying-up from a f/s which supports xattr's to one which
> > > doesn't, but when both file systems are the same and mounted the same, there
> > > typically should not be difference).
> > >
> > > Assuming that the original intent of the EPERM check in nfs4_setxattr was to
> > > prevent setting xattrs over nfs4 for non-files/dirs, then I "fixed" this by
> > > adding a similar check in nfs4_listxattr, which returns an empty xattr list
> > > for those. I've included a patch below for your consideration/review.
> >
> > I can see no reason in the RFC why you shouldn't be able to set an NFSv4
> > ACL on a special file. AFAICS this is a bug in the nfs4_setxattr code,
> > which is probably due to some improper cut n' pasting from the posix acl
> > code.
> >
> > Bruce?
>
> I think you're right.
>
> I'm also mystified by this rule that the sticky bit prevents non-owners
> from writing a directory's extended attributes. (See fs/xattr.c.) Any
> idea what the source of that is? Anyway, I don't see why the client
> should enforce such a rule in this case.
>
> Erez, does just removing this check solve your problem?
Yes, it works equally well for me. Thanks.
Acked-by: Erez Zadok <[email protected]>
> --b.
>
> From 9fa16668ed09b212ec0325786d91ac65734336ea Mon Sep 17 00:00:00 2001
> From: J. Bruce Fields <[email protected]>
> Date: Tue, 8 Jan 2008 12:41:41 -0500
> Subject: [PATCH] nfs4: allow nfsv4 acls on non-regular-files
>
> The rfc doesn't give any reason it shouldn't be possible to set an
> attribute on a non-regular file. And if the server supports it, then it
> shouldn't be up to us to prevent it.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 4 ----
> 1 files changed, 0 insertions(+), 4 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index f03d9d5..e9b9903 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3625,10 +3625,6 @@ int nfs4_setxattr(struct dentry *dentry, const char *key, const void *buf,
> if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
> return -EOPNOTSUPP;
>
> - if (!S_ISREG(inode->i_mode) &&
> - (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
> - return -EPERM;
> -
> return nfs4_proc_set_acl(inode, buf, buflen);
> }
>
> --
> 1.5.4.rc2.60.gb2e62
Erez.
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs
On Tue, Jan 08, 2008 at 04:26:29PM -0500, Erez Zadok wrote:
> Yes, it works equally well for me. Thanks.
OK, thanks.
> Acked-by: Erez Zadok <[email protected]>
Maybe that should be Reviewed-by? Tested-by? Up to Trond, I guess.
Trond, could you apply?
--b.
From: J. Bruce Fields <[email protected]>
Date: Tue, 8 Jan 2008 12:41:41 -0500
Subject: [PATCH] nfs4: allow nfsv4 acls on non-regular-files
The rfc doesn't give any reason it shouldn't be possible to set an
attribute on a non-regular file. And if the server supports it, then it
shouldn't be up to us to prevent it.
Thanks to Erez Zadok for bug report and Trond Myklebust for analysis.
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfs/nfs4proc.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f03d9d5..e9b9903 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3625,10 +3625,6 @@ int nfs4_setxattr(struct dentry *dentry, const char *key, const void *buf,
if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
return -EOPNOTSUPP;
- if (!S_ISREG(inode->i_mode) &&
- (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
- return -EPERM;
-
return nfs4_proc_set_acl(inode, buf, buflen);
}
--
1.5.4.rc2.60.gb2e62
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
Please note that [email protected] is being discontinued.
Please subscribe to [email protected] instead.
http://vger.kernel.org/vger-lists.html#linux-nfs