| Message ID | 20200916002253.GP3265@brightrain.aerifal.cx (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | changes for addding fchmodat2 syscall | expand |
On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote: > It was discovered while implementing userspace emulation of fchmodat > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise > it's not possible to target symlinks with chmod operations) that some > filesystems erroneously allow access mode of symlinks to be changed, > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit > a492b1e5ef). This inconsistency is non-conforming and wrong, and the > consensus seems to be that it was unintentional to allow link modes to > be changed in the first place. > > Signed-off-by: Rich Felker <dalias@libc.org> > --- > fs/open.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/open.c b/fs/open.c > index 9af548fb841b..cdb7964aaa6e 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) > struct iattr newattrs; > int error; > > + /* Block chmod from getting to fs layer. Ideally the fs would either > + * allow it or fail with EOPNOTSUPP, but some are buggy and return > + * an error but change the mode, which is non-conforming and wrong. */ > + if (S_ISLNK(inode->i_mode)) > + return -EOPNOTSUPP; I still fail to understand why these "buggy" filesystems can not be fixed. Why are you papering over a filesystem-specific-bug with this core kernel change that we will forever have to keep? thanks, greg k-h
On Wed, Sep 16, 2020 at 08:18:15AM +0200, Greg KH wrote: > On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote: > > It was discovered while implementing userspace emulation of fchmodat > > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise > > it's not possible to target symlinks with chmod operations) that some > > filesystems erroneously allow access mode of symlinks to be changed, > > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit > > a492b1e5ef). This inconsistency is non-conforming and wrong, and the > > consensus seems to be that it was unintentional to allow link modes to > > be changed in the first place. > > > > Signed-off-by: Rich Felker <dalias@libc.org> > > --- > > fs/open.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/fs/open.c b/fs/open.c > > index 9af548fb841b..cdb7964aaa6e 100644 > > --- a/fs/open.c > > +++ b/fs/open.c > > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) > > struct iattr newattrs; > > int error; > > > > + /* Block chmod from getting to fs layer. Ideally the fs would either > > + * allow it or fail with EOPNOTSUPP, but some are buggy and return > > + * an error but change the mode, which is non-conforming and wrong. */ > > + if (S_ISLNK(inode->i_mode)) > > + return -EOPNOTSUPP; > > I still fail to understand why these "buggy" filesystems can not be > fixed. Why are you papering over a filesystem-specific-bug with this > core kernel change that we will forever have to keep? Because checking this once in the VFS is much saner than trying to patch up a gazillion file systems.
On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote: > It was discovered while implementing userspace emulation of fchmodat > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise > it's not possible to target symlinks with chmod operations) that some > filesystems erroneously allow access mode of symlinks to be changed, > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit > a492b1e5ef). This inconsistency is non-conforming and wrong, and the > consensus seems to be that it was unintentional to allow link modes to > be changed in the first place. > > Signed-off-by: Rich Felker <dalias@libc.org> > --- > fs/open.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/open.c b/fs/open.c > index 9af548fb841b..cdb7964aaa6e 100644 > --- a/fs/open.c > +++ b/fs/open.c > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) > struct iattr newattrs; > int error; > > + /* Block chmod from getting to fs layer. Ideally the fs would either > + * allow it or fail with EOPNOTSUPP, but some are buggy and return > + * an error but change the mode, which is non-conforming and wrong. */ > + if (S_ISLNK(inode->i_mode)) > + return -EOPNOTSUPP; Our usualy place for this would be setattr_prepare. Also the comment style is off, and I don't think we should talk about buggy file systems here, but a policy to not allow the chmod. I also suspect the right error value is EINVAL - EOPNOTSUPP isn't really used in normal posix file system interfaces.
On Wed, Sep 16, 2020 at 08:18:15AM +0200, Greg KH wrote: > On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote: > > It was discovered while implementing userspace emulation of fchmodat > > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise > > it's not possible to target symlinks with chmod operations) that some > > filesystems erroneously allow access mode of symlinks to be changed, > > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit > > a492b1e5ef). This inconsistency is non-conforming and wrong, and the > > consensus seems to be that it was unintentional to allow link modes to > > be changed in the first place. > > > > Signed-off-by: Rich Felker <dalias@libc.org> > > --- > > fs/open.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/fs/open.c b/fs/open.c > > index 9af548fb841b..cdb7964aaa6e 100644 > > --- a/fs/open.c > > +++ b/fs/open.c > > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) > > struct iattr newattrs; > > int error; > > > > + /* Block chmod from getting to fs layer. Ideally the fs would either > > + * allow it or fail with EOPNOTSUPP, but some are buggy and return > > + * an error but change the mode, which is non-conforming and wrong. */ > > + if (S_ISLNK(inode->i_mode)) > > + return -EOPNOTSUPP; > > I still fail to understand why these "buggy" filesystems can not be > fixed. Why are you papering over a filesystem-specific-bug with this Because that's what Christoph wanted, and it seems exposure of the vector for applying chmod to symlinks was unintentional to begin with. I have no preference how this is fixed as long as breakage is not exposed to userspace via the new fchmodat2 syscall (since a broken syscall would be worse than not having it at all). > core kernel change that we will forever have to keep? There's no fundamental reason it would have to be kept forever. The contract remains "either it works and reports success, or it makes no change and reports EOPNOTSUPP". It just can't do both. Rich
On Wed, Sep 16, 2020 at 07:25:53AM +0100, Christoph Hellwig wrote: > On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote: > > It was discovered while implementing userspace emulation of fchmodat > > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise > > it's not possible to target symlinks with chmod operations) that some > > filesystems erroneously allow access mode of symlinks to be changed, > > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit > > a492b1e5ef). This inconsistency is non-conforming and wrong, and the > > consensus seems to be that it was unintentional to allow link modes to > > be changed in the first place. > > > > Signed-off-by: Rich Felker <dalias@libc.org> > > --- > > fs/open.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/fs/open.c b/fs/open.c > > index 9af548fb841b..cdb7964aaa6e 100644 > > --- a/fs/open.c > > +++ b/fs/open.c > > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) > > struct iattr newattrs; > > int error; > > > > + /* Block chmod from getting to fs layer. Ideally the fs would either > > + * allow it or fail with EOPNOTSUPP, but some are buggy and return > > + * an error but change the mode, which is non-conforming and wrong. */ > > + if (S_ISLNK(inode->i_mode)) > > + return -EOPNOTSUPP; > > Our usualy place for this would be setattr_prepare. Also the comment > style is off, and I don't think we should talk about buggy file systems > here, but a policy to not allow the chmod. I also suspect the right > error value is EINVAL - EOPNOTSUPP isn't really used in normal posix > file system interfaces. EINVAL is non-conforming here. POSIX defines it as unsupported mode or flag. Lack of support for setting an access mode on symlinks is a distinct failure reason, and POSIX does not allow overloading error codes like this. Even if it were permitted, it would be bad to do this because it would make it impossible for the application to tell whether the cause of failure is an invalid mode or that the filesystem/implementation doesn't support modes on symlinks. This matters because one is usually a fatal error and the other is a condition to ignore. Moreover, the affected applications (e.g. coreutils, tar, etc.) already accept EOPNOTSUPP here from libc. Defining a new error would break them unless libc translated whatever the syscall returns to the expected EOPNOTSUPP. Rich
On Wed, Sep 16, 2020 at 07:25:53AM +0100, Christoph Hellwig wrote: > On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote: > > It was discovered while implementing userspace emulation of fchmodat > > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise > > it's not possible to target symlinks with chmod operations) that some > > filesystems erroneously allow access mode of symlinks to be changed, > > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit > > a492b1e5ef). This inconsistency is non-conforming and wrong, and the > > consensus seems to be that it was unintentional to allow link modes to > > be changed in the first place. > > > > Signed-off-by: Rich Felker <dalias@libc.org> > > --- > > fs/open.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/fs/open.c b/fs/open.c > > index 9af548fb841b..cdb7964aaa6e 100644 > > --- a/fs/open.c > > +++ b/fs/open.c > > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) > > struct iattr newattrs; > > int error; > > > > + /* Block chmod from getting to fs layer. Ideally the fs would either > > + * allow it or fail with EOPNOTSUPP, but some are buggy and return > > + * an error but change the mode, which is non-conforming and wrong. */ > > + if (S_ISLNK(inode->i_mode)) > > + return -EOPNOTSUPP; > > Our usualy place for this would be setattr_prepare. Also the comment > style is off, and I don't think we should talk about buggy file systems > here, but a policy to not allow the chmod. I also suspect the right > error value is EINVAL - EOPNOTSUPP isn't really used in normal posix > file system interfaces. Er... Wasn't that an ACL-related crap? XFS calling posix_acl_chmod() after it has committed to i_mode change, propagating the error to caller of ->notify_change(), IIRC... Put it another way, why do we want if (!inode->i_op->set_acl) return -EOPNOTSUPP; in posix_acl_chmod(), when we have if (!IS_POSIXACL(inode)) return 0; right next to it? If nothing else, make that if (!IS_POSIXACL(inode) || !inode->i_op->get_acl) return 0; // piss off - nothing to adjust here
On Thu, Sep 17, 2020 at 05:07:15AM +0100, Al Viro wrote: > On Wed, Sep 16, 2020 at 07:25:53AM +0100, Christoph Hellwig wrote: > > On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote: > > > It was discovered while implementing userspace emulation of fchmodat > > > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise > > > it's not possible to target symlinks with chmod operations) that some > > > filesystems erroneously allow access mode of symlinks to be changed, > > > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit > > > a492b1e5ef). This inconsistency is non-conforming and wrong, and the > > > consensus seems to be that it was unintentional to allow link modes to > > > be changed in the first place. > > > > > > Signed-off-by: Rich Felker <dalias@libc.org> > > > --- > > > fs/open.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/fs/open.c b/fs/open.c > > > index 9af548fb841b..cdb7964aaa6e 100644 > > > --- a/fs/open.c > > > +++ b/fs/open.c > > > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) > > > struct iattr newattrs; > > > int error; > > > > > > + /* Block chmod from getting to fs layer. Ideally the fs would either > > > + * allow it or fail with EOPNOTSUPP, but some are buggy and return > > > + * an error but change the mode, which is non-conforming and wrong. */ > > > + if (S_ISLNK(inode->i_mode)) > > > + return -EOPNOTSUPP; > > > > Our usualy place for this would be setattr_prepare. Also the comment > > style is off, and I don't think we should talk about buggy file systems > > here, but a policy to not allow the chmod. I also suspect the right > > error value is EINVAL - EOPNOTSUPP isn't really used in normal posix > > file system interfaces. > > Er... Wasn't that an ACL-related crap? XFS calling posix_acl_chmod() > after it has committed to i_mode change, propagating the error to > caller of ->notify_change(), IIRC... > > Put it another way, why do we want > if (!inode->i_op->set_acl) > return -EOPNOTSUPP; > in posix_acl_chmod(), when we have > if (!IS_POSIXACL(inode)) > return 0; > right next to it? If nothing else, make that > if (!IS_POSIXACL(inode) || !inode->i_op->get_acl) > return 0; // piss off - nothing to adjust here Arrgh... That'd break shmem and similar filesystems... Still, it feels like we should _not_ bother in cases when there's no ACL for that sucker; after all, if get_acl() returns NULL, we quietly return 0 and that's it. How about something like this instead? diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 95882b3f5f62..2339160fabab 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -559,8 +559,6 @@ posix_acl_chmod(struct inode *inode, umode_t mode) if (!IS_POSIXACL(inode)) return 0; - if (!inode->i_op->set_acl) - return -EOPNOTSUPP; acl = get_acl(inode, ACL_TYPE_ACCESS); if (IS_ERR_OR_NULL(acl)) { @@ -569,6 +567,10 @@ posix_acl_chmod(struct inode *inode, umode_t mode) return PTR_ERR(acl); } + if (!inode->i_op->set_acl) { + posix_acl_release(acl); + return -EOPNOTSUPP; + } ret = __posix_acl_chmod(&acl, GFP_KERNEL, mode); if (ret) return ret;
On Thu, Sep 17, 2020 at 05:15:03AM +0100, Al Viro wrote: > On Thu, Sep 17, 2020 at 05:07:15AM +0100, Al Viro wrote: > > On Wed, Sep 16, 2020 at 07:25:53AM +0100, Christoph Hellwig wrote: > > > On Tue, Sep 15, 2020 at 08:22:54PM -0400, Rich Felker wrote: > > > > It was discovered while implementing userspace emulation of fchmodat > > > > AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise > > > > it's not possible to target symlinks with chmod operations) that some > > > > filesystems erroneously allow access mode of symlinks to be changed, > > > > but return failure with EOPNOTSUPP (see glibc issue #14578 and commit > > > > a492b1e5ef). This inconsistency is non-conforming and wrong, and the > > > > consensus seems to be that it was unintentional to allow link modes to > > > > be changed in the first place. > > > > > > > > Signed-off-by: Rich Felker <dalias@libc.org> > > > > --- > > > > fs/open.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/fs/open.c b/fs/open.c > > > > index 9af548fb841b..cdb7964aaa6e 100644 > > > > --- a/fs/open.c > > > > +++ b/fs/open.c > > > > @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) > > > > struct iattr newattrs; > > > > int error; > > > > > > > > + /* Block chmod from getting to fs layer. Ideally the fs would either > > > > + * allow it or fail with EOPNOTSUPP, but some are buggy and return > > > > + * an error but change the mode, which is non-conforming and wrong. */ > > > > + if (S_ISLNK(inode->i_mode)) > > > > + return -EOPNOTSUPP; > > > > > > Our usualy place for this would be setattr_prepare. Also the comment > > > style is off, and I don't think we should talk about buggy file systems > > > here, but a policy to not allow the chmod. I also suspect the right > > > error value is EINVAL - EOPNOTSUPP isn't really used in normal posix > > > file system interfaces. > > > > Er... Wasn't that an ACL-related crap? XFS calling posix_acl_chmod() > > after it has committed to i_mode change, propagating the error to > > caller of ->notify_change(), IIRC... > > > > Put it another way, why do we want > > if (!inode->i_op->set_acl) > > return -EOPNOTSUPP; > > in posix_acl_chmod(), when we have > > if (!IS_POSIXACL(inode)) > > return 0; > > right next to it? If nothing else, make that > > if (!IS_POSIXACL(inode) || !inode->i_op->get_acl) > > return 0; // piss off - nothing to adjust here > > Arrgh... That'd break shmem and similar filesystems... Still, it > feels like we should _not_ bother in cases when there's no ACL > for that sucker; after all, if get_acl() returns NULL, we quietly > return 0 and that's it. > > How about something like this instead? > > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index 95882b3f5f62..2339160fabab 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -559,8 +559,6 @@ posix_acl_chmod(struct inode *inode, umode_t mode) > > if (!IS_POSIXACL(inode)) > return 0; > - if (!inode->i_op->set_acl) > - return -EOPNOTSUPP; > > acl = get_acl(inode, ACL_TYPE_ACCESS); > if (IS_ERR_OR_NULL(acl)) { > @@ -569,6 +567,10 @@ posix_acl_chmod(struct inode *inode, umode_t mode) > return PTR_ERR(acl); > } > > + if (!inode->i_op->set_acl) { > + posix_acl_release(acl); > + return -EOPNOTSUPP; > + } > ret = __posix_acl_chmod(&acl, GFP_KERNEL, mode); > if (ret) > return ret; Does this make chmod of links behave consistently (either succeeding with return value 0, or returning -EOPNOTSUPP without doing anything) for all filesystems? I'm fine with (and would probably prefer) this fix if it's a complete one. If this goes in I think my patch 1/2 can just be dropped and patch 2/2 behaves as intended; does that sound correct to you? Rich
On Thu, Sep 17, 2020 at 05:15:03AM +0100, Al Viro wrote: > Arrgh... That'd break shmem and similar filesystems... Still, it > feels like we should _not_ bother in cases when there's no ACL > for that sucker; after all, if get_acl() returns NULL, we quietly > return 0 and that's it. > > How about something like this instead? Do you plan to turn this into a submission? Rich, can you share your original reproducer? I would be really helpful to have it wired up in xfstests as a regression tests.
diff --git a/fs/open.c b/fs/open.c index 9af548fb841b..cdb7964aaa6e 100644 --- a/fs/open.c +++ b/fs/open.c @@ -570,6 +570,12 @@ int chmod_common(const struct path *path, umode_t mode) struct iattr newattrs; int error; + /* Block chmod from getting to fs layer. Ideally the fs would either + * allow it or fail with EOPNOTSUPP, but some are buggy and return + * an error but change the mode, which is non-conforming and wrong. */ + if (S_ISLNK(inode->i_mode)) + return -EOPNOTSUPP; + error = mnt_want_write(path->mnt); if (error) return error;
It was discovered while implementing userspace emulation of fchmodat AT_SYMLINK_NOFOLLOW (using O_PATH and procfs magic symlinks; otherwise it's not possible to target symlinks with chmod operations) that some filesystems erroneously allow access mode of symlinks to be changed, but return failure with EOPNOTSUPP (see glibc issue #14578 and commit a492b1e5ef). This inconsistency is non-conforming and wrong, and the consensus seems to be that it was unintentional to allow link modes to be changed in the first place. Signed-off-by: Rich Felker <dalias@libc.org> --- fs/open.c | 6 ++++++ 1 file changed, 6 insertions(+)