[LU-10785] ll_acl_{access,default}_xattr_handler confuse name and prefix

• Type: Bug
• Resolution: Fixed
• Priority: Minor
• Fix Version/s: Lustre 2.12.0
• Affects Version/s: Lustre 2.11.0, Lustre 2.10.3
• Severity: 3
• Assignee: John Hammond
• Reporter: John Hammond

Description

In the definition of ll_acl_access_xattr_handler we use the xattr name ("system.posix_acl_access") to initialize the prefix member:

static const struct xattr_handler ll_acl_access_xattr_handler = {
	.prefix = XATTR_NAME_POSIX_ACL_ACCESS,
	.flags = XATTR_ACL_ACCESS_T,
#if defined(HAVE_XATTR_HANDLER_SIMPLIFIED)
	.get = ll_xattr_get_common_4_3,
	.set = ll_xattr_set_common_4_3,
#elif !defined(HAVE_XATTR_HANDLER_INODE_PARAM)
	.get = ll_xattr_get_common_3_11,
	.set = ll_xattr_set_common_3_11,
#else
     	.get = ll_xattr_get_common,
	.set = ll_xattr_set_common,
#endif
};

We should leave prefix unassigned and use the name member instead. This causes getfacl() (which uses getxattr()) to fail for some kernel versions. Similarly for ll_acl_default_xattr_handler.

There also may be conflicts between Lustre and upstream regarding the use of the flags member of struct xattr_handler. Lustre uses

#define XATTR_ACL_ACCESS_T      4
#define XATTR_ACL_DEFAULT_T     5

Upstream uses:

/* a_type field in acl_user_posix_entry_t */
#define ACL_TYPE_ACCESS		(0x8000)
#define ACL_TYPE_DEFAULT	(0x4000)

I did not try to determine how the kernel uses the flags member.

Other filesystems use kernel defined handers for ACLs:

static const struct xattr_handler *ext4_xattr_handler_map[] = {
	[EXT4_XATTR_INDEX_USER]		     = &ext4_xattr_user_handler,
#ifdef CONFIG_EXT4_FS_POSIX_ACL
	[EXT4_XATTR_INDEX_POSIX_ACL_ACCESS]  = &posix_acl_access_xattr_handler,
	[EXT4_XATTR_INDEX_POSIX_ACL_DEFAULT] = &posix_acl_default_xattr_handler,
#endif
	[EXT4_XATTR_INDEX_TRUSTED]	     = &ext4_xattr_trusted_handler,
#ifdef CONFIG_EXT4_FS_SECURITY
	[EXT4_XATTR_INDEX_SECURITY]	     = &ext4_xattr_security_handler,
#endif
};

We should see if these can be used by Lustre as well.

Some auditing is needed.

Issue Links

• is related to: LU-9679 Prepare lustre for adoption into the linux kernel

Comments

[James A Simmons]

Just as a note I pushed https://review.whamcloud.com/#/c/31921  to verify these changes

[Peter Jones]

Landed for 2.12

[James A Simmons]

Before we close this I like to test Ubunut 16 sanity test again.

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch https://review.whamcloud.com/31595/
Subject: LU-10785 llite: use xattr_handler name for ACLs
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: a9998927281647f9d9b8bfa50c80b7e15cf05eda

[James A Simmons]

Nope. I did some digging and found the bug upstream which caused 102a and 102b to fail. Now it works out of the box. Only 103a fails for the upstream client. Do you see any failures with acl/run_test permisson ? Your patches look great. Thank you!!!

[John Hammond]

Did these tests pass before? Can you post your port?

[James A Simmons]

I ported your changes to the upstream kernel and ran sanity test 102 and 103. From the test I see the following test fail:

sanity: FAIL: test_102a /lustre/lustre/f102a.sanity missing 3 trusted.name xattrs
sanity: FAIL: test_102b can't get trusted.lov from /lustre/lustre/f102b.sanity
sanity: FAIL: test_103a run_acl_subtest cp failed

This is testing with your latest patches.

[James A Simmons]

Thank you John for tracking down these issues. I will port to upstream and report back the results

[Gerrit Updater]

John L. Hammond ([e-mail]) uploaded a new patch: https://review.whamcloud.com/31595
Subject: LU-10785 llite: use xattr_handler name for ACLs
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 269568dea51bcd678183402f3898b484fce08627