| Message ID | 20191212105433.1692-6-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/33] xfs: clear kernel only flags in XFS_IOC_ATTRMULTI_BY_HANDLE | expand |
On Thu, Dec 12, 2019 at 11:54:05AM +0100, Christoph Hellwig wrote: > Replace the ATTR_INCOMPLETE flag with a new boolean field in struct > xfs_attr_list_context. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/libxfs/xfs_attr.h | 5 ++--- > fs/xfs/scrub/attr.c | 2 +- > fs/xfs/xfs_attr_list.c | 6 +----- > 3 files changed, 4 insertions(+), 9 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > index 91c2cb14276e..04a628016728 100644 > --- a/fs/xfs/libxfs/xfs_attr.h > +++ b/fs/xfs/libxfs/xfs_attr.h > @@ -36,11 +36,10 @@ struct xfs_attr_list_context; > #define ATTR_KERNOTIME 0x1000 /* [kernel] don't update inode timestamps */ > #define ATTR_KERNOVAL 0x2000 /* [kernel] get attr size only, not value */ > > -#define ATTR_INCOMPLETE 0x4000 /* [kernel] return INCOMPLETE attr keys */ Hmm, did we used to allow ATTR_INCOMPLETE from the attr_multi userspace calls? Maybe we should leave ATTR_INCOMPLETE so that we can blacklist it in case we ever see it coming from userspace? Or at least prevent anyone from reusing 0x4000 and suffering the confusion. I also wonder if we can break userspace this way, but OTOH userspace should never be able to query incomplete attrs and this is an obscure ioctl anyway so maybe it's fine.... --D > #define ATTR_ALLOC 0x8000 /* [kernel] allocate xattr buffer on demand */ > > #define ATTR_KERNEL_FLAGS \ > - (ATTR_KERNOTIME | ATTR_KERNOVAL | ATTR_INCOMPLETE | ATTR_ALLOC) > + (ATTR_KERNOTIME | ATTR_KERNOVAL | ATTR_ALLOC) > > #define XFS_ATTR_FLAGS \ > { ATTR_DONTFOLLOW, "DONTFOLLOW" }, \ > @@ -51,7 +50,6 @@ struct xfs_attr_list_context; > { ATTR_REPLACE, "REPLACE" }, \ > { ATTR_KERNOTIME, "KERNOTIME" }, \ > { ATTR_KERNOVAL, "KERNOVAL" }, \ > - { ATTR_INCOMPLETE, "INCOMPLETE" }, \ > { ATTR_ALLOC, "ALLOC" } > > /* > @@ -123,6 +121,7 @@ typedef struct xfs_attr_list_context { > * error values to the xfs_attr_list caller. > */ > int seen_enough; > + bool allow_incomplete; > > ssize_t count; /* num used entries */ > int dupcnt; /* count dup hashvals seen */ > diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c > index d9f0dd444b80..d804558cdbca 100644 > --- a/fs/xfs/scrub/attr.c > +++ b/fs/xfs/scrub/attr.c > @@ -497,7 +497,7 @@ xchk_xattr( > sx.context.resynch = 1; > sx.context.put_listent = xchk_xattr_listent; > sx.context.tp = sc->tp; > - sx.context.flags = ATTR_INCOMPLETE; > + sx.context.allow_incomplete = true; > sx.sc = sc; > > /* > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c > index d37743bdf274..5139ef983cd6 100644 > --- a/fs/xfs/xfs_attr_list.c > +++ b/fs/xfs/xfs_attr_list.c > @@ -452,7 +452,7 @@ xfs_attr3_leaf_list_int( > } > > if ((entry->flags & XFS_ATTR_INCOMPLETE) && > - !(context->flags & ATTR_INCOMPLETE)) > + !context->allow_incomplete) > continue; /* skip incomplete entries */ > > if (entry->flags & XFS_ATTR_LOCAL) { > @@ -632,10 +632,6 @@ xfs_attr_list( > (cursor->hashval || cursor->blkno || cursor->offset)) > return -EINVAL; > > - /* Only internal consumers can retrieve incomplete attrs. */ > - if (flags & ATTR_INCOMPLETE) > - return -EINVAL; > - > /* > * Check for a properly aligned buffer. > */ > -- > 2.20.1 >
On Wed, Dec 18, 2019 at 01:43:09PM -0800, Darrick J. Wong wrote: > > diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h > > index 91c2cb14276e..04a628016728 100644 > > --- a/fs/xfs/libxfs/xfs_attr.h > > +++ b/fs/xfs/libxfs/xfs_attr.h > > @@ -36,11 +36,10 @@ struct xfs_attr_list_context; > > #define ATTR_KERNOTIME 0x1000 /* [kernel] don't update inode timestamps */ > > #define ATTR_KERNOVAL 0x2000 /* [kernel] get attr size only, not value */ > > > > -#define ATTR_INCOMPLETE 0x4000 /* [kernel] return INCOMPLETE attr keys */ > > Hmm, did we used to allow ATTR_INCOMPLETE from the attr_multi userspace > calls? We allowed that, but it didn't do anything as only the attr list code checked for ATTR_INCOMPLETE. > Maybe we should leave ATTR_INCOMPLETE so that we can blacklist > it in case we ever see it coming from userspace? Or at least prevent > anyone from reusing 0x4000 and suffering the confusion. At then end of the series the whole flags mess is cleaned up and there is a hard barrier from user to kernel flags, so this should all be set. > I also wonder if we can break userspace this way, but OTOH userspace > should never be able to query incomplete attrs and this is an obscure > ioctl anyway so maybe it's fine.... Exactly. Incomplete attrs are an implementation detail that must not leak to userspace.
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index 91c2cb14276e..04a628016728 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -36,11 +36,10 @@ struct xfs_attr_list_context; #define ATTR_KERNOTIME 0x1000 /* [kernel] don't update inode timestamps */ #define ATTR_KERNOVAL 0x2000 /* [kernel] get attr size only, not value */ -#define ATTR_INCOMPLETE 0x4000 /* [kernel] return INCOMPLETE attr keys */ #define ATTR_ALLOC 0x8000 /* [kernel] allocate xattr buffer on demand */ #define ATTR_KERNEL_FLAGS \ - (ATTR_KERNOTIME | ATTR_KERNOVAL | ATTR_INCOMPLETE | ATTR_ALLOC) + (ATTR_KERNOTIME | ATTR_KERNOVAL | ATTR_ALLOC) #define XFS_ATTR_FLAGS \ { ATTR_DONTFOLLOW, "DONTFOLLOW" }, \ @@ -51,7 +50,6 @@ struct xfs_attr_list_context; { ATTR_REPLACE, "REPLACE" }, \ { ATTR_KERNOTIME, "KERNOTIME" }, \ { ATTR_KERNOVAL, "KERNOVAL" }, \ - { ATTR_INCOMPLETE, "INCOMPLETE" }, \ { ATTR_ALLOC, "ALLOC" } /* @@ -123,6 +121,7 @@ typedef struct xfs_attr_list_context { * error values to the xfs_attr_list caller. */ int seen_enough; + bool allow_incomplete; ssize_t count; /* num used entries */ int dupcnt; /* count dup hashvals seen */ diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c index d9f0dd444b80..d804558cdbca 100644 --- a/fs/xfs/scrub/attr.c +++ b/fs/xfs/scrub/attr.c @@ -497,7 +497,7 @@ xchk_xattr( sx.context.resynch = 1; sx.context.put_listent = xchk_xattr_listent; sx.context.tp = sc->tp; - sx.context.flags = ATTR_INCOMPLETE; + sx.context.allow_incomplete = true; sx.sc = sc; /* diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c index d37743bdf274..5139ef983cd6 100644 --- a/fs/xfs/xfs_attr_list.c +++ b/fs/xfs/xfs_attr_list.c @@ -452,7 +452,7 @@ xfs_attr3_leaf_list_int( } if ((entry->flags & XFS_ATTR_INCOMPLETE) && - !(context->flags & ATTR_INCOMPLETE)) + !context->allow_incomplete) continue; /* skip incomplete entries */ if (entry->flags & XFS_ATTR_LOCAL) { @@ -632,10 +632,6 @@ xfs_attr_list( (cursor->hashval || cursor->blkno || cursor->offset)) return -EINVAL; - /* Only internal consumers can retrieve incomplete attrs. */ - if (flags & ATTR_INCOMPLETE) - return -EINVAL; - /* * Check for a properly aligned buffer. */
Replace the ATTR_INCOMPLETE flag with a new boolean field in struct xfs_attr_list_context. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_attr.h | 5 ++--- fs/xfs/scrub/attr.c | 2 +- fs/xfs/xfs_attr_list.c | 6 +----- 3 files changed, 4 insertions(+), 9 deletions(-)