| Message ID | 171270969941.3631889.11060276222007768999.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/32] xfs: rearrange xfs_attr_match parameters | expand |
On Tue, Apr 09, 2024 at 05:59:30PM -0700, Darrick J. Wong wrote: > From: Allison Henderson <allison.henderson@oracle.com> > > Parent pointers returned to the get_fattr tool cause errors since > the tool cannot parse parent pointers. Fix this by filtering parent > parent pointers from xfs_xattr_put_listent. With the new format returning the attrs should not cause parsing errors. OTOH we now have duplicate names, which means a get operation based on the name can't actually work in that case. I'd also argue that parent pointers are internal enough that they should not be exposed through the normal xattr interfaces. > +/* > + * This file defines functions to work with externally visible extended > + * attributes, such as those in user, system, or security namespaces. They > + * should not be used for internally used attributes. Consider xfs_attr.c. > + */ As long as xfs_attr_change and xfs_attr_grab_log_assist are xfs_xattr.c that is not actually true. However I think they should be moved to xfs_attr.c (and in case of xfs_attr_change merged into xfs_attr_set) to make this comment true. However I'd make it part of the top of file comment above the include statements. And please add it in a separate commit as it has nothing to do with the other changes here.
On Tue, Apr 09, 2024 at 10:51:18PM -0700, Christoph Hellwig wrote: > On Tue, Apr 09, 2024 at 05:59:30PM -0700, Darrick J. Wong wrote: > > From: Allison Henderson <allison.henderson@oracle.com> > > > > Parent pointers returned to the get_fattr tool cause errors since > > the tool cannot parse parent pointers. Fix this by filtering parent > > parent pointers from xfs_xattr_put_listent. > > With the new format returning the attrs should not cause parsing errors. > OTOH we now have duplicate names, which means a get operation based on > the name can't actually work in that case. > > I'd also argue that parent pointers are internal enough that they > should not be exposed through the normal xattr interfaces. Yeah, I probably should change the commit message to: "xfs: don't return XFS_ATTR_PARENT attributes via listxattr "Parent pointers are internal filesystem metadata. They're not intended to be directly visible to userspace, so filter them out of xfs_xattr_put_listent so that they don't appear in listxattr." > > +/* > > + * This file defines functions to work with externally visible extended > > + * attributes, such as those in user, system, or security namespaces. They > > + * should not be used for internally used attributes. Consider xfs_attr.c. > > + */ > > As long as xfs_attr_change and xfs_attr_grab_log_assist are xfs_xattr.c > that is not actually true. However I think they should be moved to > xfs_attr.c (and in case of xfs_attr_change merged into xfs_attr_set) > to make this comment true. I don't want to hoist all the larp enabling jun^Wmachinery to libxfs and then have to stub that out in userspace. I'd rather get rid of larp mode entirely, after which point xfs_attr_change becomes a trivial helper that can be collapsed. > However I'd make it part of the top of file comment above the include > statements. And please add it in a separate commit as it has nothing > to do with the other changes here. Or just get rid of the comment entirely? It came from the verity series. --D
On Wed, Apr 10, 2024 at 02:58:27PM -0700, Darrick J. Wong wrote: > "xfs: don't return XFS_ATTR_PARENT attributes via listxattr > > "Parent pointers are internal filesystem metadata. They're not intended > to be directly visible to userspace, so filter them out of > xfs_xattr_put_listent so that they don't appear in listxattr." Looks good. > > However I'd make it part of the top of file comment above the include > > statements. And please add it in a separate commit as it has nothing > > to do with the other changes here. > > Or just get rid of the comment entirely? It came from the verity > series. Fine with me.
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h index 1395ad1937c53..ebde6eb1da65d 100644 --- a/fs/xfs/libxfs/xfs_da_format.h +++ b/fs/xfs/libxfs/xfs_da_format.h @@ -726,6 +726,9 @@ struct xfs_attr3_leafblock { XFS_ATTR_SECURE | \ XFS_ATTR_PARENT) +/* Private attr namespaces not exposed to userspace */ +#define XFS_ATTR_PRIVATE_NSP_MASK (XFS_ATTR_PARENT) + #define XFS_ATTR_ONDISK_MASK (XFS_ATTR_NSP_ONDISK_MASK | \ XFS_ATTR_LOCAL | \ XFS_ATTR_INCOMPLETE) diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c index 85e886ee20e03..00b591f6c5ca1 100644 --- a/fs/xfs/xfs_xattr.c +++ b/fs/xfs/xfs_xattr.c @@ -20,6 +20,12 @@ #include <linux/posix_acl_xattr.h> +/* + * This file defines functions to work with externally visible extended + * attributes, such as those in user, system, or security namespaces. They + * should not be used for internally used attributes. Consider xfs_attr.c. + */ + /* * Get permission to use log-assisted atomic exchange of file extents. * Callers must not be running any transactions or hold any ILOCKs. @@ -215,6 +221,10 @@ xfs_xattr_put_listent( ASSERT(context->count >= 0); + /* Don't expose private xattr namespaces. */ + if (flags & XFS_ATTR_PRIVATE_NSP_MASK) + return; + if (flags & XFS_ATTR_ROOT) { #ifdef CONFIG_XFS_POSIX_ACL if (namelen == SGI_ACL_FILE_SIZE &&