xfs_fsr: more selinux fixes

PATCH Superseded Eric Sandeen 2 participants 5 messages Nov 11, 2015 – Nov 12, 2015

lore

Share & tools

lore

Eric Sandeen

author

2015-11-11 21:53 UTC #0 lore raw

PATCH 1/1 xfs_fsr: more selinux fixes

Commit:

1adfe5c xfs_fsr: fix SWAPEXT failures under selinux

attempted to fix up the fork offset under selinux, where the temp file is created with a local attribute, but the target file has remote attributes; this can lead to a smaller data area in the temp inode, without enough room to swap extents from the target inode. I remedied this by pushing the temp file attribute to remote, but *only* if the target file's attr was also remote.

However, I have a case from the field where the parent dir and the target file both have a context of:

system_u:object_r:samba_share_t:s0

but new files created in the dir have a context of

unconfined_u:object_r:samba_share_t:s0

This means the temp file has a smaller forkoff, and less space in the inode for data, so we fail to swap the extents between the two, because they don't fit.

The following patch fixes this by allowing xfs_fsr to kick the tempfile's attr out of local format even if the target file's attr is local, if this will move the forkoff in the right direction. This does pass all our fsr xfstests, though I'm not sure we have any real coverage of fsr under selinux...

The only functional change is the test at the very end of the patch; the rest is comments, ascii art, and removing the now-extraneous XFS_IOC_FSGETXATTRA ioctl.

Signed-off-by Eric Sandeen <[email protected]>

---

fsr/xfs_fsr.c +8 ~~-20~~

index c8ef18f..68b9819 100644
@@ -1104,14 +1104,12 @@ fsr_setup_attr_fork(
 			}
 			continue;
 		} else if (i == 0) {
-			struct fsxattr	fsx;
 			/*
 			 * First pass, and temp file already has an inline
 			 * xattr, probably due to selinux.
 			 *
 			 * It's *possible* that the temp file attr area
-			 * is larger than the target file's, if the 
-			 * target file's attrs are not inline:
+			 * is larger than the target file's:
 			 *
 			 *  Target		 Temp
 			 * +-------+ 0		+-------+ 0

@@ -1121,28 +1119,18 @@ fsr_setup_attr_fork(
 			 * |	   |		v-------v forkoff
 			 * |	   |		|       |
 			 * v-------v forkoff	| Attr  | local
-			 * | Attr  | ext/btree	|       |
+			 * | Attr  | 		|       |
 			 * +-------+		+-------+
-			 *
-			 * FSGETXATTRA will tell us nr of attr extents in
-			 * target, if any.  If none, it's local:
 			 */
 
-			memset(&fsx, 0, sizeof(fsx));
-			if (ioctl(fd, XFS_IOC_FSGETXATTRA, &fsx)) {
-				fsrprintf(_("FSGETXATTRA failed on target\n"));
-				return -1;
-			}
-
 			/*
-			 * If target attr area is less than the temp's (diff < 0)
-			 * and the target is not local, write a big attr to
-			 * the temp file to knock the attr out of local format,
-			 * to match the target.  (This should actually *increase*
-			 * the temp file's forkoffset when the attr moves out
-			 * of the inode)
+			 * If target attr area is less than the temp's
+			 * (diff < 0), write a big attr to the temp file to knock
+			 * the attr out of local format.
+			 * (This should actually *increase* the temp file's
+			 * forkoffset when the attr moves out of the inode)
 			 */
- 			if (diff < 0 && fsx.fsx_nextents > 0) {
+ 			if (diff < 0) {
 				char val[2048];
 				memset(val, 'X', 2048);
 				if (fsetxattr(tfd, name, val, 2048, 0)) {

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Brian Foster

2015-11-12 13:12 UTC #1 lore raw

Quoted 42 lines · Eric Sandeen

Commit:
1adfe5c xfs_fsr: fix SWAPEXT failures under selinux
attempted to fix up the fork offset under selinux, where the temp file is created with a local attribute, but the target file has remote attributes; this can lead to a smaller data area in the temp inode, without enough room to swap extents from the target inode. I remedied this by pushing the temp file attribute to remote, but *only* if the target file's attr was also remote.
However, I have a case from the field where the parent dir and the target file both have a context of:
system_u:object_r:samba_share_t:s0
but new files created in the dir have a context of
unconfined_u:object_r:samba_share_t:s0
This means the temp file has a smaller forkoff, and less space in the inode for data, so we fail to swap the extents between the two, because they don't fit.
The following patch fixes this by allowing xfs_fsr to kick the tempfile's attr out of local format even if the target file's attr is local, if this will move the forkoff in the right direction. This does pass all our fsr xfstests, though I'm not sure we have any real coverage of fsr under selinux...
The only functional change is the test at the very end of the patch; the rest is comments, ascii art, and removing the now-extraneous XFS_IOC_FSGETXATTRA ioctl.
Signed-off-by Eric Sandeen <[email protected]>
---
fsr/xfs_fsr.c +0 -0
index c8ef18f..68b9819 100644

...

Quoted 5 lines

fsr/xfs_fsr.c +0 -4

 			/*
-			 * If target attr area is less than the temp's (diff < 0)
-			 * and the target is not local, write a big attr to
-			 * the temp file to knock the attr out of local format,
-			 * to match the target.  (This should actually *increase*

fsr/xfs_fsr.c fsr_setup_attr_fork( · line 1133

Quoted 10 lines

fsr/xfs_fsr.c +6 -3

-			 * the temp file's forkoffset when the attr moves out
-			 * of the inode)
+			 * If target attr area is less than the temp's
+			 * (diff < 0), write a big attr to the temp file to knock
+			 * the attr out of local format.
+			 * (This should actually *increase* the temp file's
+			 * forkoffset when the attr moves out of the inode)
 			 */
- 			if (diff < 0 && fsx.fsx_nextents > 0) {
+ 			if (diff < 0) {

Space before tab issue on the line above. Looks fine otherwise, but I wonder if it would also be a good idea to add an informative fsrprintf() here if we proceed when (fsx.fsx_nextents > 0)?

Brian

Quoted 8 lines

fsr/xfs_fsr.c +0 -0
 				char val[2048];
 				memset(val, 'X', 2048);
 				if (fsetxattr(tfd, name, val, 2048, 0)) {
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2015-11-12 15:50 UTC #2 lore raw

Quoted 55 lines · Brian Foster

Quoted 42 lines · Eric Sandeen
Commit:
1adfe5c xfs_fsr: fix SWAPEXT failures under selinux
attempted to fix up the fork offset under selinux, where the temp file is created with a local attribute, but the target file has remote attributes; this can lead to a smaller data area in the temp inode, without enough room to swap extents from the target inode. I remedied this by pushing the temp file attribute to remote, but *only* if the target file's attr was also remote.
However, I have a case from the field where the parent dir and the target file both have a context of:
system_u:object_r:samba_share_t:s0
but new files created in the dir have a context of
unconfined_u:object_r:samba_share_t:s0
This means the temp file has a smaller forkoff, and less space in the inode for data, so we fail to swap the extents between the two, because they don't fit.
The following patch fixes this by allowing xfs_fsr to kick the tempfile's attr out of local format even if the target file's attr is local, if this will move the forkoff in the right direction. This does pass all our fsr xfstests, though I'm not sure we have any real coverage of fsr under selinux...
The only functional change is the test at the very end of the patch; the rest is comments, ascii art, and removing the now-extraneous XFS_IOC_FSGETXATTRA ioctl.
Signed-off-by Eric Sandeen <[email protected]>
---
fsr/xfs_fsr.c +0 -0
index c8ef18f..68b9819 100644
...
Quoted 11 lines
fsr/xfs_fsr.c +4 -6
 			/*
-			 * If target attr area is less than the temp's (diff < 0)
-			 * and the target is not local, write a big attr to
-			 * the temp file to knock the attr out of local format,
-			 * to match the target.  (This should actually *increase*
-			 * the temp file's forkoffset when the attr moves out
-			 * of the inode)
+			 * If target attr area is less than the temp's
+			 * (diff < 0), write a big attr to the temp file to knock
+			 * the attr out of local format.
+			 * (This should actually *increase* the temp file's

Quoted 8 lines

fsr/xfs_fsr.c fsr_setup_attr_fork( · line 1133
Quoted 4 lines
fsr/xfs_fsr.c +2 -1
+			 * forkoffset when the attr moves out of the inode)
 			 */
- 			if (diff < 0 && fsx.fsx_nextents > 0) {
+ 			if (diff < 0) {
Space before tab issue on the line above. Looks fine otherwise, but I wonder if it would also be a good idea to add an informative fsrprintf() here if we proceed when (fsx.fsx_nextents > 0)?

I can add a debug message, sure - I'm not sure when it would be useful, but we have plenty of other fiddly messages in there about what action is taken.

Thanks,
-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Eric Sandeen

author

2015-11-12 15:58 UTC #3 lore raw

PATCH v2 1/1 xfs_fsr: more selinux fixes

In reply to Eric Sandeen

Commit:

1adfe5c xfs_fsr: fix SWAPEXT failures under selinux

However, I have a case from the field where the parent dir and the target file both have a context of:

system_u:object_r:samba_share_t:s0

but new files created in the dir have a context of

unconfined_u:object_r:samba_share_t:s0

This means the temp file has a smaller forkoff, and less space in the inode for data, so we fail to swap the extents between the two, because they don't fit.

The only functional change is the test at the very end of the patch; the rest is comments, ascii art, and removing the now-extraneous XFS_IOC_FSGETXATTRA ioctl.

Signed-off-by Eric Sandeen <[email protected]>

---

V2: Fix a whitespace issue.

Brian, I didn't add the debug printf; we don't do it for any other attr operations unless they fail, and I don't think it's needed here either. And I couldn't fit it gracefully into 80 cols. ;)

fsr/xfs_fsr.c +8 ~~-20~~

index b902acc..2bae1d0 100644
@@ -1115,14 +1115,12 @@ fsr_setup_attr_fork(
 			}
 			continue;
 		} else if (i == 0) {
-			struct fsxattr	fsx;
 			/*
 			 * First pass, and temp file already has an inline
 			 * xattr, probably due to selinux.
 			 *
 			 * It's *possible* that the temp file attr area
-			 * is larger than the target file's, if the 
-			 * target file's attrs are not inline:
+			 * is larger than the target file's:
 			 *
 			 *  Target		 Temp
 			 * +-------+ 0		+-------+ 0

@@ -1132,28 +1130,18 @@ fsr_setup_attr_fork(
 			 * |	   |		v-------v forkoff
 			 * |	   |		|       |
 			 * v-------v forkoff	| Attr  | local
-			 * | Attr  | ext/btree	|       |
+			 * | Attr  | 		|       |
 			 * +-------+		+-------+
-			 *
-			 * FSGETXATTRA will tell us nr of attr extents in
-			 * target, if any.  If none, it's local:
 			 */
 
-			memset(&fsx, 0, sizeof(fsx));
-			if (ioctl(fd, XFS_IOC_FSGETXATTRA, &fsx)) {
-				fsrprintf(_("FSGETXATTRA failed on target\n"));
-				return -1;
-			}
-
 			/*
-			 * If target attr area is less than the temp's (diff < 0)
-			 * and the target is not local, write a big attr to
-			 * the temp file to knock the attr out of local format,
-			 * to match the target.  (This should actually *increase*
-			 * the temp file's forkoffset when the attr moves out
-			 * of the inode)
+			 * If target attr area is less than the temp's
+			 * (diff < 0) write a big attr to the temp file to knock
+			 * the attr out of local format.
+			 * (This should actually *increase* the temp file's
+			 * forkoffset when the attr moves out of the inode)
 			 */
- 			if (diff < 0 && fsx.fsx_nextents > 0) {
+			if (diff < 0) {
 				char val[2048];
 				memset(val, 'X', 2048);
 				if (fsetxattr(tfd, name, val, 2048, 0)) {

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Brian Foster

Reviewed-by on v2 1/1

2015-11-12 16:05 UTC #4 lore raw

Quoted 35 lines · Eric Sandeen

Commit:
1adfe5c xfs_fsr: fix SWAPEXT failures under selinux
attempted to fix up the fork offset under selinux, where the temp file is created with a local attribute, but the target file has remote attributes; this can lead to a smaller data area in the temp inode, without enough room to swap extents from the target inode. I remedied this by pushing the temp file attribute to remote, but *only* if the target file's attr was also remote.
However, I have a case from the field where the parent dir and the target file both have a context of:
system_u:object_r:samba_share_t:s0
but new files created in the dir have a context of
unconfined_u:object_r:samba_share_t:s0
This means the temp file has a smaller forkoff, and less space in the inode for data, so we fail to swap the extents between the two, because they don't fit.
The following patch fixes this by allowing xfs_fsr to kick the tempfile's attr out of local format even if the target file's attr is local, if this will move the forkoff in the right direction. This does pass all our fsr xfstests, though I'm not sure we have any real coverage of fsr under selinux...
The only functional change is the test at the very end of the patch; the rest is comments, ascii art, and removing the now-extraneous XFS_IOC_FSGETXATTRA ioctl.

Quoted 9 lines

Signed-off-by Eric Sandeen <[email protected]>
---
V2: Fix a whitespace issue.
Brian, I didn't add the debug printf; we don't do it for any other attr operations unless they fail, and I don't think it's needed here either. And I couldn't fit it gracefully into 80 cols. ;)

Ok, no problem. I was originally thinking that this would affect the target file. As pointed out on IRC, the target file xattrs should remain unmodified and the extent information is just swapped into the data area. Looks fine to me:

Reviewed-by Brian Foster <[email protected]>

Quoted 57 lines

fsr/xfs_fsr.c +8 ~~-20~~

index b902acc..2bae1d0 100644
@@ -1115,14 +1115,12 @@ fsr_setup_attr_fork(
 			}
 			continue;
 		} else if (i == 0) {
-			struct fsxattr	fsx;
 			/*
 			 * First pass, and temp file already has an inline
 			 * xattr, probably due to selinux.
 			 *
 			 * It's *possible* that the temp file attr area
-			 * is larger than the target file's, if the 
-			 * target file's attrs are not inline:
+			 * is larger than the target file's:
 			 *
 			 *  Target		 Temp
 			 * +-------+ 0		+-------+ 0

@@ -1132,28 +1130,18 @@ fsr_setup_attr_fork(
 			 * |	   |		v-------v forkoff
 			 * |	   |		|       |
 			 * v-------v forkoff	| Attr  | local
-			 * | Attr  | ext/btree	|       |
+			 * | Attr  | 		|       |
 			 * +-------+		+-------+
-			 *
-			 * FSGETXATTRA will tell us nr of attr extents in
-			 * target, if any.  If none, it's local:
 			 */
 
-			memset(&fsx, 0, sizeof(fsx));
-			if (ioctl(fd, XFS_IOC_FSGETXATTRA, &fsx)) {
-				fsrprintf(_("FSGETXATTRA failed on target\n"));
-				return -1;
-			}
-
 			/*
-			 * If target attr area is less than the temp's (diff < 0)
-			 * and the target is not local, write a big attr to
-			 * the temp file to knock the attr out of local format,
-			 * to match the target.  (This should actually *increase*
-			 * the temp file's forkoffset when the attr moves out
-			 * of the inode)
+			 * If target attr area is less than the temp's
+			 * (diff < 0) write a big attr to the temp file to knock
+			 * the attr out of local format.
+			 * (This should actually *increase* the temp file's
+			 * forkoffset when the attr moves out of the inode)
 			 */
- 			if (diff < 0 && fsx.fsx_nextents > 0) {
+			if (diff < 0) {
 				char val[2048];
 				memset(val, 'X', 2048);
 				if (fsetxattr(tfd, name, val, 2048, 0)) {

Quoted 4 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs