use proper mnt_want_write calls for handle ioctls

PATCH Christoph Hellwig 2 participants 4 messages Sep 12, 2008 – Oct 22, 2008 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Christoph Hellwig

author

2008-09-12 08:43 UTC #1 lore raw

ping? Without this patch the CVS tree won't properly work with read-only bind mounts.

Quoted 67 lines · Christoph Hellwig

Since 2.6.26 all writes to filesystems need to be enclosed by a mnt_want_write / mnt_drop_write pair instead of checking for IS_RDONLY.

XFs was updated for this in mainline but the changes to xfs_ioctl.c were never megred back into the CVS tree.

The original commit introducing this was:

commit 42a74f206b914db13ee1f5ae932dcd91a77c8579

Author: Dave Hansen <[email protected]>
Date:   Fri Feb 15 14:37:46 2008 -0800

[PATCH] r/o bind mounts: elevate write count for ioctls()

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================

fs/xfs/linux-2.6/xfs_ioctl.c +9 -4

@@ -543,8 +543,6 @@ xfs_attrmulti_attr_set(
 	char			*kbuf;
 	int			error = EFAULT;
 
-	if (IS_RDONLY(inode))
-		return -EROFS;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		return EPERM;
 	if (len > XATTR_SIZE_MAX)

@@ -570,8 +568,6 @@ xfs_attrmulti_attr_remove(
 	char			*name,
 	__uint32_t		flags)
 {
-	if (IS_RDONLY(inode))
-		return -EROFS;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		return EPERM;
 	return xfs_attr_remove(XFS_I(inode), name, flags);

@@ -581,6 +577,7 @@ STATIC int
 xfs_attrmulti_by_handle(
 	xfs_mount_t		*mp,
 	void			__user *arg,
+	struct file		*parfilp,
 	struct inode		*parinode)
 {
 	int			error;

@@ -634,13 +631,21 @@ xfs_attrmulti_by_handle(
 					&ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
+			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_set(inode,
 					attr_name, ops[i].am_attrvalue,
 					ops[i].am_length, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		case ATTR_OP_REMOVE:
+			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
 					attr_name, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		default:
 			ops[i].am_error = EINVAL;

fs/xfs/linux-2.6/xfs_ioctl.c xfs_ioctl( · line 1442

Quoted 8 lines

fs/xfs/linux-2.6/xfs_ioctl.c +1 -1

@@ -1431,7 +1436,7 @@ xfs_ioctl(
 		return xfs_attrlist_by_handle(mp, arg, inode);
 
 	case XFS_IOC_ATTRMULTI_BY_HANDLE:
-		return xfs_attrmulti_by_handle(mp, arg, inode);
+		return xfs_attrmulti_by_handle(mp, arg, filp, inode);
 
 	case XFS_IOC_SWAPEXT: {
 		error = xfs_swapext((struct xfs_swapext __user *)arg);

---end quoted text---

Christoph Hellwig

author

2008-09-29 07:41 UTC #2 lore raw

ping^2?

Quoted 73 lines · Christoph Hellwig

ping? Without this patch the CVS tree won't properly work with read-only bind mounts.

Quoted 69 lines · Christoph Hellwig

Since 2.6.26 all writes to filesystems need to be enclosed by a mnt_want_write / mnt_drop_write pair instead of checking for IS_RDONLY.

XFs was updated for this in mainline but the changes to xfs_ioctl.c were never megred back into the CVS tree.

The original commit introducing this was:

commit 42a74f206b914db13ee1f5ae932dcd91a77c8579

Author: Dave Hansen <[email protected]>
Date:   Fri Feb 15 14:37:46 2008 -0800

[PATCH] r/o bind mounts: elevate write count for ioctls()

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================

fs/xfs/linux-2.6/xfs_ioctl.c +9 -4

@@ -543,8 +543,6 @@ xfs_attrmulti_attr_set(
 	char			*kbuf;
 	int			error = EFAULT;
 
-	if (IS_RDONLY(inode))
-		return -EROFS;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		return EPERM;
 	if (len > XATTR_SIZE_MAX)

@@ -570,8 +568,6 @@ xfs_attrmulti_attr_remove(
 	char			*name,
 	__uint32_t		flags)
 {
-	if (IS_RDONLY(inode))
-		return -EROFS;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		return EPERM;
 	return xfs_attr_remove(XFS_I(inode), name, flags);

@@ -581,6 +577,7 @@ STATIC int
 xfs_attrmulti_by_handle(
 	xfs_mount_t		*mp,
 	void			__user *arg,
+	struct file		*parfilp,
 	struct inode		*parinode)
 {
 	int			error;

@@ -634,13 +631,21 @@ xfs_attrmulti_by_handle(
 					&ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
+			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_set(inode,
 					attr_name, ops[i].am_attrvalue,
 					ops[i].am_length, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		case ATTR_OP_REMOVE:
+			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
 					attr_name, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		default:
 			ops[i].am_error = EINVAL;

@@ -1431,7 +1436,7 @@ xfs_ioctl(
 		return xfs_attrlist_by_handle(mp, arg, inode);

Quoted 7 lines

fs/xfs/linux-2.6/xfs_ioctl.c xfs_ioctl( · line 1442

Quoted 6 lines

fs/xfs/linux-2.6/xfs_ioctl.c +1 -1

 	case XFS_IOC_ATTRMULTI_BY_HANDLE:
-		return xfs_attrmulti_by_handle(mp, arg, inode);
+		return xfs_attrmulti_by_handle(mp, arg, filp, inode);
 
 	case XFS_IOC_SWAPEXT: {
 		error = xfs_swapext((struct xfs_swapext __user *)arg);

---end quoted text---

Christoph Hellwig

author

2008-10-18 12:36 UTC #3 lore raw

Ping again. As said before this could cause filesystem corruption with the SGI tree, and it's always been correct in mainline there shouldn't be any reason to delay it.

Quoted 76 lines · Christoph Hellwig

ping^2?

Quoted 73 lines · Christoph Hellwig

ping? Without this patch the CVS tree won't properly work with read-only bind mounts.

Quoted 69 lines · Christoph Hellwig

Since 2.6.26 all writes to filesystems need to be enclosed by a mnt_want_write / mnt_drop_write pair instead of checking for IS_RDONLY.

XFs was updated for this in mainline but the changes to xfs_ioctl.c were never megred back into the CVS tree.

The original commit introducing this was:

commit 42a74f206b914db13ee1f5ae932dcd91a77c8579

Author: Dave Hansen <[email protected]>
Date:   Fri Feb 15 14:37:46 2008 -0800

[PATCH] r/o bind mounts: elevate write count for ioctls()

Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================

fs/xfs/linux-2.6/xfs_ioctl.c +9 -4

@@ -543,8 +543,6 @@ xfs_attrmulti_attr_set(
 	char			*kbuf;
 	int			error = EFAULT;
 
-	if (IS_RDONLY(inode))
-		return -EROFS;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		return EPERM;
 	if (len > XATTR_SIZE_MAX)

@@ -570,8 +568,6 @@ xfs_attrmulti_attr_remove(
 	char			*name,
 	__uint32_t		flags)
 {
-	if (IS_RDONLY(inode))
-		return -EROFS;
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
 		return EPERM;
 	return xfs_attr_remove(XFS_I(inode), name, flags);

@@ -581,6 +577,7 @@ STATIC int
 xfs_attrmulti_by_handle(
 	xfs_mount_t		*mp,
 	void			__user *arg,
+	struct file		*parfilp,
 	struct inode		*parinode)
 {
 	int			error;

@@ -634,13 +631,21 @@ xfs_attrmulti_by_handle(
 					&ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
+			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_set(inode,
 					attr_name, ops[i].am_attrvalue,
 					ops[i].am_length, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		case ATTR_OP_REMOVE:
+			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
 					attr_name, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		default:
 			ops[i].am_error = EINVAL;

@@ -1431,7 +1436,7 @@ xfs_ioctl(
 		return xfs_attrlist_by_handle(mp, arg, inode);

Quoted 8 lines

Quoted 7 lines
fs/xfs/linux-2.6/xfs_ioctl.c xfs_ioctl( · line 1442
Quoted 6 lines
fs/xfs/linux-2.6/xfs_ioctl.c +1 -1
 	case XFS_IOC_ATTRMULTI_BY_HANDLE:
-		return xfs_attrmulti_by_handle(mp, arg, inode);
+		return xfs_attrmulti_by_handle(mp, arg, filp, inode);
 
 	case XFS_IOC_SWAPEXT: {
 		error = xfs_swapext((struct xfs_swapext __user *)arg);
---end quoted text---
---end quoted text---

---end quoted text---

Donald Douwsma

2008-10-22 05:22 UTC #4 lore raw

Quoted 3 lines · Christoph Hellwig

Ping again. As said before this could cause filesystem corruption with the SGI tree, and it's always been correct in mainline there shouldn't be any reason to delay it.

Done