[Quantal,1/3] isofs: Refuse RW mount of the filesystem instead of making it RO
diff mbox

Message ID 1380177614-22952-1-git-send-email-hui.wang@canonical.com
State New
Headers show

Commit Message

Hui Wang Sept. 26, 2013, 6:40 a.m. UTC
From: Jan Kara <jack@suse.cz>

BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1228751

Refuse RW mount of isofs filesystem. So far we just silently changed it
to RO mount but when the media is writeable, block layer won't notice
this change and thus will think device is used RW and will block eject
button of the drive. That is unexpected by users because for
non-writeable media eject button works just fine.

Userspace mount(8) command handles this just fine and retries mounting
with MS_RDONLY set so userspace shouldn't see any regression.  Plus any
tool mounting isofs is likely confronted with the case of read-only
media where block layer already refuses to mount the filesystem without
MS_RDONLY set so our behavior shouldn't be anything new for it.

Reported-by: Hui Wang <hui.wang@canonical.com>
Signed-off-by: Jan Kara <jack@suse.cz>
(cherry picked from commit 17b7f7cf58926844e1dd40f5eb5348d481deca6a)
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
After apply these 3 patches to quantal kernel, i did a building test,
the test passed. Then i used a dvd-rw isofs disc and cd-rw udf ro disc
to test, they worked well and the eject button also work well.

 fs/isofs/inode.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Comments

Tim Gardner Sept. 26, 2013, 3:44 p.m. UTC | #1
Egads, patch 2 is giant, but appears to be corrrect (and mostly
mechanical changes).
Andy Whitcroft Sept. 27, 2013, 9:49 a.m. UTC | #2
On Thu, Sep 26, 2013 at 02:40:06PM +0800, Hui Wang wrote:
> From: Jan Kara <jack@suse.cz>
> 
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1228751
> 
> Refuse RW mount of isofs filesystem. So far we just silently changed it
> to RO mount but when the media is writeable, block layer won't notice
> this change and thus will think device is used RW and will block eject
> button of the drive. That is unexpected by users because for
> non-writeable media eject button works just fine.
> 
> Userspace mount(8) command handles this just fine and retries mounting
> with MS_RDONLY set so userspace shouldn't see any regression.  Plus any
> tool mounting isofs is likely confronted with the case of read-only
> media where block layer already refuses to mount the filesystem without
> MS_RDONLY set so our behavior shouldn't be anything new for it.
> 
> Reported-by: Hui Wang <hui.wang@canonical.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> (cherry picked from commit 17b7f7cf58926844e1dd40f5eb5348d481deca6a)
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> After apply these 3 patches to quantal kernel, i did a building test,
> the test passed. Then i used a dvd-rw isofs disc and cd-rw udf ro disc
> to test, they worked well and the eject button also work well.
> 
>  fs/isofs/inode.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 29037c3..e92a342 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -119,8 +119,8 @@ static void destroy_inodecache(void)
>  
>  static int isofs_remount(struct super_block *sb, int *flags, char *data)
>  {
> -	/* we probably want a lot more here */
> -	*flags |= MS_RDONLY;
> +	if (!(*flags & MS_RDONLY))
> +		return -EROFS;
>  	return 0;
>  }
>  
> @@ -769,15 +769,6 @@ root_found:
>  	 */
>  	s->s_maxbytes = 0x80000000000LL;
>  
> -	/*
> -	 * The CDROM is read-only, has no nodes (devices) on it, and since
> -	 * all of the files appear to be owned by root, we really do not want
> -	 * to allow suid.  (suid or devices will not show up unless we have
> -	 * Rock Ridge extensions)
> -	 */
> -
> -	s->s_flags |= MS_RDONLY /* | MS_NODEV | MS_NOSUID */;
> -
>  	/* Set this for reference. Its not currently used except on write
>  	   which we don't have .. */
>  
> @@ -1536,6 +1527,9 @@ struct inode *isofs_iget(struct super_block *sb,
>  static struct dentry *isofs_mount(struct file_system_type *fs_type,
>  	int flags, const char *dev_name, void *data)
>  {
> +	/* We don't support read-write mounts */
> +	if (!(flags & MS_RDONLY))
> +		return ERR_PTR(-EACCES);
>  	return mount_bdev(fs_type, flags, dev_name, data, isofs_fill_super);
>  }

Ok the first and third are as stated cherry-picks and look fine.  The
second is a bit of a beast, matches upstream in the main and looks ok.
Please can we test this one properly though when it gets to -proposed.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Andy Whitcroft Sept. 27, 2013, 9:53 a.m. UTC | #3
Applied to Quantal.

-apw

Patch
diff mbox

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 29037c3..e92a342 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -119,8 +119,8 @@  static void destroy_inodecache(void)
 
 static int isofs_remount(struct super_block *sb, int *flags, char *data)
 {
-	/* we probably want a lot more here */
-	*flags |= MS_RDONLY;
+	if (!(*flags & MS_RDONLY))
+		return -EROFS;
 	return 0;
 }
 
@@ -769,15 +769,6 @@  root_found:
 	 */
 	s->s_maxbytes = 0x80000000000LL;
 
-	/*
-	 * The CDROM is read-only, has no nodes (devices) on it, and since
-	 * all of the files appear to be owned by root, we really do not want
-	 * to allow suid.  (suid or devices will not show up unless we have
-	 * Rock Ridge extensions)
-	 */
-
-	s->s_flags |= MS_RDONLY /* | MS_NODEV | MS_NOSUID */;
-
 	/* Set this for reference. Its not currently used except on write
 	   which we don't have .. */
 
@@ -1536,6 +1527,9 @@  struct inode *isofs_iget(struct super_block *sb,
 static struct dentry *isofs_mount(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data)
 {
+	/* We don't support read-write mounts */
+	if (!(flags & MS_RDONLY))
+		return ERR_PTR(-EACCES);
 	return mount_bdev(fs_type, flags, dev_name, data, isofs_fill_super);
 }