* [PATCH 1/4] xfs: clear kernel only flags in XFS_IOC_ATTRMULTI_BY_HANDLE
2020-01-07 16:54 attr fixes Christoph Hellwig
@ 2020-01-07 16:54 ` Christoph Hellwig
2020-01-07 23:23 ` Darrick J. Wong
2020-01-07 16:54 ` [PATCH 2/4] xfs: reject invalid flags combinations " Christoph Hellwig
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2020-01-07 16:54 UTC (permalink / raw)
To: linux-xfs
Don't allow passing arbitrary flags as they change behavior including
memory allocation that the call stack is not prepared for.
Fixes: ddbca70cc45c ("xfs: allocate xattr buffer on demand")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/libxfs/xfs_attr.h | 7 +++++--
fs/xfs/xfs_ioctl.c | 2 ++
fs/xfs/xfs_ioctl32.c | 2 ++
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 94badfa1743e..91c2cb14276e 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -26,7 +26,7 @@ struct xfs_attr_list_context;
*========================================================================*/
-#define ATTR_DONTFOLLOW 0x0001 /* -- unused, from IRIX -- */
+#define ATTR_DONTFOLLOW 0x0001 /* -- ignored, from IRIX -- */
#define ATTR_ROOT 0x0002 /* use attrs in root (trusted) namespace */
#define ATTR_TRUST 0x0004 /* -- unused, from IRIX -- */
#define ATTR_SECURE 0x0008 /* use attrs in security namespace */
@@ -37,7 +37,10 @@ struct xfs_attr_list_context;
#define ATTR_KERNOVAL 0x2000 /* [kernel] get attr size only, not value */
#define ATTR_INCOMPLETE 0x4000 /* [kernel] return INCOMPLETE attr keys */
-#define ATTR_ALLOC 0x8000 /* allocate xattr buffer on demand */
+#define ATTR_ALLOC 0x8000 /* [kernel] allocate xattr buffer on demand */
+
+#define ATTR_KERNEL_FLAGS \
+ (ATTR_KERNOTIME | ATTR_KERNOVAL | ATTR_INCOMPLETE | ATTR_ALLOC)
#define XFS_ATTR_FLAGS \
{ ATTR_DONTFOLLOW, "DONTFOLLOW" }, \
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 7b35d62ede9f..edfbdb8f85e2 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -462,6 +462,8 @@ xfs_attrmulti_by_handle(
error = 0;
for (i = 0; i < am_hreq.opcount; i++) {
+ ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
+
ops[i].am_error = strncpy_from_user((char *)attr_name,
ops[i].am_attrname, MAXNAMELEN);
if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index c4c4f09113d3..bd9d9ebf85d8 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -450,6 +450,8 @@ xfs_compat_attrmulti_by_handle(
error = 0;
for (i = 0; i < am_hreq.opcount; i++) {
+ ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
+
ops[i].am_error = strncpy_from_user((char *)attr_name,
compat_ptr(ops[i].am_attrname),
MAXNAMELEN);
--
2.24.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/4] xfs: clear kernel only flags in XFS_IOC_ATTRMULTI_BY_HANDLE
2020-01-07 16:54 ` [PATCH 1/4] xfs: clear kernel only flags in XFS_IOC_ATTRMULTI_BY_HANDLE Christoph Hellwig
@ 2020-01-07 23:23 ` Darrick J. Wong
0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2020-01-07 23:23 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Tue, Jan 07, 2020 at 05:54:39PM +0100, Christoph Hellwig wrote:
> Don't allow passing arbitrary flags as they change behavior including
> memory allocation that the call stack is not prepared for.
>
> Fixes: ddbca70cc45c ("xfs: allocate xattr buffer on demand")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/libxfs/xfs_attr.h | 7 +++++--
> fs/xfs/xfs_ioctl.c | 2 ++
> fs/xfs/xfs_ioctl32.c | 2 ++
> 3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 94badfa1743e..91c2cb14276e 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -26,7 +26,7 @@ struct xfs_attr_list_context;
> *========================================================================*/
>
>
> -#define ATTR_DONTFOLLOW 0x0001 /* -- unused, from IRIX -- */
> +#define ATTR_DONTFOLLOW 0x0001 /* -- ignored, from IRIX -- */
> #define ATTR_ROOT 0x0002 /* use attrs in root (trusted) namespace */
> #define ATTR_TRUST 0x0004 /* -- unused, from IRIX -- */
> #define ATTR_SECURE 0x0008 /* use attrs in security namespace */
> @@ -37,7 +37,10 @@ struct xfs_attr_list_context;
> #define ATTR_KERNOVAL 0x2000 /* [kernel] get attr size only, not value */
>
> #define ATTR_INCOMPLETE 0x4000 /* [kernel] return INCOMPLETE attr keys */
> -#define ATTR_ALLOC 0x8000 /* allocate xattr buffer on demand */
> +#define ATTR_ALLOC 0x8000 /* [kernel] allocate xattr buffer on demand */
> +
> +#define ATTR_KERNEL_FLAGS \
> + (ATTR_KERNOTIME | ATTR_KERNOVAL | ATTR_INCOMPLETE | ATTR_ALLOC)
>
> #define XFS_ATTR_FLAGS \
> { ATTR_DONTFOLLOW, "DONTFOLLOW" }, \
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 7b35d62ede9f..edfbdb8f85e2 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -462,6 +462,8 @@ xfs_attrmulti_by_handle(
>
> error = 0;
> for (i = 0; i < am_hreq.opcount; i++) {
> + ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
> +
> ops[i].am_error = strncpy_from_user((char *)attr_name,
> ops[i].am_attrname, MAXNAMELEN);
> if (ops[i].am_error == 0 || ops[i].am_error == MAXNAMELEN)
> diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
> index c4c4f09113d3..bd9d9ebf85d8 100644
> --- a/fs/xfs/xfs_ioctl32.c
> +++ b/fs/xfs/xfs_ioctl32.c
> @@ -450,6 +450,8 @@ xfs_compat_attrmulti_by_handle(
>
> error = 0;
> for (i = 0; i < am_hreq.opcount; i++) {
> + ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
> +
> ops[i].am_error = strncpy_from_user((char *)attr_name,
> compat_ptr(ops[i].am_attrname),
> MAXNAMELEN);
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] xfs: reject invalid flags combinations in XFS_IOC_ATTRMULTI_BY_HANDLE
2020-01-07 16:54 attr fixes Christoph Hellwig
2020-01-07 16:54 ` [PATCH 1/4] xfs: clear kernel only flags in XFS_IOC_ATTRMULTI_BY_HANDLE Christoph Hellwig
@ 2020-01-07 16:54 ` Christoph Hellwig
2020-01-07 16:54 ` [PATCH 3/4] xfs: also remove cached ACLs when removing the underlying attr Christoph Hellwig
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2020-01-07 16:54 UTC (permalink / raw)
To: linux-xfs; +Cc: Darrick J . Wong
While the flags field in the ABI and the on-disk format allows for
multiple namespace flags, that is a logically invalid combination that
scrub complains about. Reject it at the ioctl level, as all other
interface already get this right at higher levels.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/xfs_ioctl.c | 5 +++++
fs/xfs/xfs_ioctl32.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index edfbdb8f85e2..17b4a981be4d 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -462,6 +462,11 @@ xfs_attrmulti_by_handle(
error = 0;
for (i = 0; i < am_hreq.opcount; i++) {
+ if ((ops[i].am_flags & ATTR_ROOT) &&
+ (ops[i].am_flags & ATTR_SECURE)) {
+ ops[i].am_error = -EINVAL;
+ continue;
+ }
ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
ops[i].am_error = strncpy_from_user((char *)attr_name,
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index bd9d9ebf85d8..565c6404ee94 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -450,6 +450,11 @@ xfs_compat_attrmulti_by_handle(
error = 0;
for (i = 0; i < am_hreq.opcount; i++) {
+ if ((ops[i].am_flags & ATTR_ROOT) &&
+ (ops[i].am_flags & ATTR_SECURE)) {
+ ops[i].am_error = -EINVAL;
+ continue;
+ }
ops[i].am_flags &= ~ATTR_KERNEL_FLAGS;
ops[i].am_error = strncpy_from_user((char *)attr_name,
--
2.24.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/4] xfs: also remove cached ACLs when removing the underlying attr
2020-01-07 16:54 attr fixes Christoph Hellwig
2020-01-07 16:54 ` [PATCH 1/4] xfs: clear kernel only flags in XFS_IOC_ATTRMULTI_BY_HANDLE Christoph Hellwig
2020-01-07 16:54 ` [PATCH 2/4] xfs: reject invalid flags combinations " Christoph Hellwig
@ 2020-01-07 16:54 ` Christoph Hellwig
2020-01-07 23:23 ` Darrick J. Wong
2020-01-07 16:54 ` [PATCH 4/4] xfs: fix misuse of the XFS_ATTR_INCOMPLETE flag Christoph Hellwig
2020-01-07 23:23 ` attr fixes Darrick J. Wong
4 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2020-01-07 16:54 UTC (permalink / raw)
To: linux-xfs
We should not just invalidate the ACL when setting the underlying
attribute, but also when removing it. The ioctl interface gets that
right, but the normal xattr inteface skipped the xfs_forget_acl due
to an early return.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_xattr.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 383f0203d103..2288f20ae282 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -74,10 +74,11 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
if (flags & XATTR_REPLACE)
xflags |= ATTR_REPLACE;
- if (!value)
- return xfs_attr_remove(ip, (unsigned char *)name, xflags);
- error = xfs_attr_set(ip, (unsigned char *)name,
+ if (value)
+ error = xfs_attr_set(ip, (unsigned char *)name,
(void *)value, size, xflags);
+ else
+ error = xfs_attr_remove(ip, (unsigned char *)name, xflags);
if (!error)
xfs_forget_acl(inode, name, xflags);
--
2.24.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] xfs: also remove cached ACLs when removing the underlying attr
2020-01-07 16:54 ` [PATCH 3/4] xfs: also remove cached ACLs when removing the underlying attr Christoph Hellwig
@ 2020-01-07 23:23 ` Darrick J. Wong
0 siblings, 0 replies; 9+ messages in thread
From: Darrick J. Wong @ 2020-01-07 23:23 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Tue, Jan 07, 2020 at 05:54:41PM +0100, Christoph Hellwig wrote:
> We should not just invalidate the ACL when setting the underlying
> attribute, but also when removing it. The ioctl interface gets that
> right, but the normal xattr inteface skipped the xfs_forget_acl due
> to an early return.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_xattr.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> index 383f0203d103..2288f20ae282 100644
> --- a/fs/xfs/xfs_xattr.c
> +++ b/fs/xfs/xfs_xattr.c
> @@ -74,10 +74,11 @@ xfs_xattr_set(const struct xattr_handler *handler, struct dentry *unused,
> if (flags & XATTR_REPLACE)
> xflags |= ATTR_REPLACE;
>
> - if (!value)
> - return xfs_attr_remove(ip, (unsigned char *)name, xflags);
> - error = xfs_attr_set(ip, (unsigned char *)name,
> + if (value)
> + error = xfs_attr_set(ip, (unsigned char *)name,
> (void *)value, size, xflags);
> + else
> + error = xfs_attr_remove(ip, (unsigned char *)name, xflags);
> if (!error)
> xfs_forget_acl(inode, name, xflags);
>
> --
> 2.24.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 4/4] xfs: fix misuse of the XFS_ATTR_INCOMPLETE flag
2020-01-07 16:54 attr fixes Christoph Hellwig
` (2 preceding siblings ...)
2020-01-07 16:54 ` [PATCH 3/4] xfs: also remove cached ACLs when removing the underlying attr Christoph Hellwig
@ 2020-01-07 16:54 ` Christoph Hellwig
2020-01-07 23:23 ` attr fixes Darrick J. Wong
4 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2020-01-07 16:54 UTC (permalink / raw)
To: linux-xfs; +Cc: Darrick J . Wong
XFS_ATTR_INCOMPLETE is a flag in the on-disk attribute format, and thus
in a different namespace as the ATTR_* flags in xfs_da_args.flags.
Switch to using a XFS_DA_OP_INCOMPLETE flag in op_flags instead. Without
this users might be able to inject this flag into operations using the
attr by handle ioctl.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/xfs/libxfs/xfs_attr.c | 2 +-
fs/xfs/libxfs/xfs_attr_leaf.c | 4 ++--
fs/xfs/libxfs/xfs_da_btree.h | 4 +++-
fs/xfs/libxfs/xfs_da_format.h | 2 --
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 0d7fcc983b3d..2368a1bfe7e8 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1007,7 +1007,7 @@ xfs_attr_node_addname(
* The INCOMPLETE flag means that we will find the "old"
* attr, not the "new" one.
*/
- args->flags |= XFS_ATTR_INCOMPLETE;
+ args->op_flags |= XFS_DA_OP_INCOMPLETE;
state = xfs_da_state_alloc();
state->args = args;
state->mp = mp;
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 08d4b10ae2d5..fed537a4353d 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -2403,8 +2403,8 @@ xfs_attr3_leaf_lookup_int(
* If we are looking for INCOMPLETE entries, show only those.
* If we are looking for complete entries, show only those.
*/
- if ((args->flags & XFS_ATTR_INCOMPLETE) !=
- (entry->flags & XFS_ATTR_INCOMPLETE)) {
+ if (!!(args->op_flags & XFS_DA_OP_INCOMPLETE) !=
+ !!(entry->flags & XFS_ATTR_INCOMPLETE)) {
continue;
}
if (entry->flags & XFS_ATTR_LOCAL) {
diff --git a/fs/xfs/libxfs/xfs_da_btree.h b/fs/xfs/libxfs/xfs_da_btree.h
index e16610d1c14f..0f4fbb0889ff 100644
--- a/fs/xfs/libxfs/xfs_da_btree.h
+++ b/fs/xfs/libxfs/xfs_da_btree.h
@@ -89,6 +89,7 @@ typedef struct xfs_da_args {
#define XFS_DA_OP_OKNOENT 0x0008 /* lookup/add op, ENOENT ok, else die */
#define XFS_DA_OP_CILOOKUP 0x0010 /* lookup to return CI name if found */
#define XFS_DA_OP_ALLOCVAL 0x0020 /* lookup to alloc buffer if found */
+#define XFS_DA_OP_INCOMPLETE 0x0040 /* lookup INCOMPLETE attr keys */
#define XFS_DA_OP_FLAGS \
{ XFS_DA_OP_JUSTCHECK, "JUSTCHECK" }, \
@@ -96,7 +97,8 @@ typedef struct xfs_da_args {
{ XFS_DA_OP_ADDNAME, "ADDNAME" }, \
{ XFS_DA_OP_OKNOENT, "OKNOENT" }, \
{ XFS_DA_OP_CILOOKUP, "CILOOKUP" }, \
- { XFS_DA_OP_ALLOCVAL, "ALLOCVAL" }
+ { XFS_DA_OP_ALLOCVAL, "ALLOCVAL" }, \
+ { XFS_DA_OP_INCOMPLETE, "INCOMPLETE" }
/*
* Storage for holding state during Btree searches and split/join ops.
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 3dee33043e09..05615d1f4113 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -683,8 +683,6 @@ struct xfs_attr3_leafblock {
/*
* Flags used in the leaf_entry[i].flags field.
- * NOTE: the INCOMPLETE bit must not collide with the flags bits specified
- * on the system call, they are "or"ed together for various operations.
*/
#define XFS_ATTR_LOCAL_BIT 0 /* attr is stored locally */
#define XFS_ATTR_ROOT_BIT 1 /* limit access to trusted attrs */
--
2.24.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: attr fixes
2020-01-07 16:54 attr fixes Christoph Hellwig
` (3 preceding siblings ...)
2020-01-07 16:54 ` [PATCH 4/4] xfs: fix misuse of the XFS_ATTR_INCOMPLETE flag Christoph Hellwig
@ 2020-01-07 23:23 ` Darrick J. Wong
2020-01-08 7:04 ` Christoph Hellwig
4 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2020-01-07 23:23 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Tue, Jan 07, 2020 at 05:54:38PM +0100, Christoph Hellwig wrote:
> Hi all,
>
> this series contains a bunch of fixes for nasty bugs in the attr interface.
Hm, do you want to throw in the ATTR_INCOMPLETE killing patch (#5 in the
old series) too?
--D
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: attr fixes
2020-01-07 23:23 ` attr fixes Darrick J. Wong
@ 2020-01-08 7:04 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2020-01-08 7:04 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs
On Tue, Jan 07, 2020 at 03:23:46PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 07, 2020 at 05:54:38PM +0100, Christoph Hellwig wrote:
> > Hi all,
> >
> > this series contains a bunch of fixes for nasty bugs in the attr interface.
>
> Hm, do you want to throw in the ATTR_INCOMPLETE killing patch (#5 in the
> old series) too?
That isn't really an urgent fix. With the kernel-only flags clearing
it can't cause any harm.
^ permalink raw reply [flat|nested] 9+ messages in thread