Bug with read only handling in mount

Bug with read only handling in mount

[Kent Overstreet]

sys-utils/mount.c, mk_exit_code()

If the mount syscall returns EACCESS, the code treats this as meaning that RW
access to the block device wasn't allowed - it switches to RO for all future
mount attempts.

This is incorrect though, because EACCESS could just mean that that particular
filesystem doesn't support RW: iso9600 returns EACCESS if you try to mount RW.

The end result is that if we're trying to mount by trying every filesystem type
(your libblkid doesn't know about your filesystem yet..), and the correct
filesystem was listed after iso9600 in /proc/filesystems, mount will always
mount RO (unless you specify the filesystem type with -t).

Re: Bug with read only handling in mount

[Karel Zak]

On Mon, Oct 03, 2016 at 11:41:32PM -0800, Kent Overstreet wrote:
> sys-utils/mount.c, mk_exit_code()
> 
> If the mount syscall returns EACCESS, the code treats this as meaning that RW
> access to the block device wasn't allowed - it switches to RO for all future
> mount attempts.

This is pretty old (>10years) mount behavior, util-linux 2.13:

    case EACCES:  /* pre-linux 1.1.38, 1.1.41 and later */
    case EROFS:   /* linux 1.1.38 and later */

> This is incorrect though, because EACCESS could just mean that that particular
> filesystem doesn't support RW: iso9600 returns EACCESS if you try to mount RW.

So, remount RO makes sense, right? I don't think we want to change
this behavior, all CDROM/DVD users depend on this.

> The end result is that if we're trying to mount by trying every filesystem type
> (your libblkid doesn't know about your filesystem yet..), and the correct
> filesystem was listed after iso9600 in /proc/filesystems, mount will always
> mount RO (unless you specify the filesystem type with -t).

Not sure if I understand. Does it mean that iso9600 driver returns
EACCES for all devices although there is no this FS on the device? Or
your FS shares the device with iso9600?

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Bug with read only handling in mount

[Kent Overstreet]

On Tue, Oct 04, 2016 at 10:29:09AM +0200, Karel Zak wrote:
> On Mon, Oct 03, 2016 at 11:41:32PM -0800, Kent Overstreet wrote:
> > sys-utils/mount.c, mk_exit_code()
> > 
> > If the mount syscall returns EACCESS, the code treats this as meaning that RW
> > access to the block device wasn't allowed - it switches to RO for all future
> > mount attempts.
> 
> This is pretty old (>10years) mount behavior, util-linux 2.13:
> 
>     case EACCES:  /* pre-linux 1.1.38, 1.1.41 and later */
>     case EROFS:   /* linux 1.1.38 and later */
> 
> > This is incorrect though, because EACCESS could just mean that that particular
> > filesystem doesn't support RW: iso9600 returns EACCESS if you try to mount RW.
> 
> So, remount RO makes sense, right? I don't think we want to change
> this behavior, all CDROM/DVD users depend on this.

Yes - what I'm saying is that we shouldn't quit trying to mount RW with _other_
filesystem types. Or alternatively, we should only attempt to mount RO after
that _particular_ driver has returned EACCES/EROFS.

The bug is that the global context is flipped to RO, not just for attempting
with that filesystem type.

> 
> > The end result is that if we're trying to mount by trying every filesystem type
> > (your libblkid doesn't know about your filesystem yet..), and the correct
> > filesystem was listed after iso9600 in /proc/filesystems, mount will always
> > mount RO (unless you specify the filesystem type with -t).
> 
> Not sure if I understand. Does it mean that iso9600 driver returns
> EACCES for all devices although there is no this FS on the device? Or
> your FS shares the device with iso9600?

Yes, iso9660 return EACCES when no iso9600 filesystem is present.

Re: Bug with read only handling in mount

[Karel Zak]

On Tue, Oct 04, 2016 at 01:18:23AM -0800, Kent Overstreet wrote:
> On Tue, Oct 04, 2016 at 10:29:09AM +0200, Karel Zak wrote:
> > On Mon, Oct 03, 2016 at 11:41:32PM -0800, Kent Overstreet wrote:
> > > sys-utils/mount.c, mk_exit_code()
> > > 
> > > If the mount syscall returns EACCESS, the code treats this as meaning that RW
> > > access to the block device wasn't allowed - it switches to RO for all future
> > > mount attempts.
> > 
> > This is pretty old (>10years) mount behavior, util-linux 2.13:
> > 
> >     case EACCES:  /* pre-linux 1.1.38, 1.1.41 and later */
> >     case EROFS:   /* linux 1.1.38 and later */
> > 
> > > This is incorrect though, because EACCESS could just mean that that particular
> > > filesystem doesn't support RW: iso9600 returns EACCESS if you try to mount RW.
> > 
> > So, remount RO makes sense, right? I don't think we want to change
> > this behavior, all CDROM/DVD users depend on this.
> 
> Yes - what I'm saying is that we shouldn't quit trying to mount RW with _other_
> filesystem types. Or alternatively, we should only attempt to mount RO after
> that _particular_ driver has returned EACCES/EROFS.
> 
> The bug is that the global context is flipped to RO, not just for attempting
> with that filesystem type.

Hmm.. I will try to improve it. The problem is that mount(8) interprets 
EACCES/EROFS as information about the device, then flip to RO makes sense 
for all next mount(2) attempts.

> > > The end result is that if we're trying to mount by trying every filesystem type
> > > (your libblkid doesn't know about your filesystem yet..), and the correct
> > > filesystem was listed after iso9600 in /proc/filesystems, mount will always
> > > mount RO (unless you specify the filesystem type with -t).
> > 
> > Not sure if I understand. Does it mean that iso9600 driver returns
> > EACCES for all devices although there is no this FS on the device? Or
> > your FS shares the device with iso9600?
> 
> Yes, iso9660 return EACCES when no iso9600 filesystem is present.


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);
}

This is crazy... iso9600 driver starts analyze mount options although
the mount request is maybe completely irrelevant for the driver and 
there is no iso9600 on the device. 

If we will write FS drivers in this way then old good "try all from /{proc,etc}/filesystems"
will be useless...

See another filesystems, for example ext4, first be sure there is
superblock and magic string (or return EINVAL) and then try 
validate mount options.

CC to Jan Kara (he did the kernel change in Jun 2013).

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Bug with read only handling in mount

[Kent Overstreet]

On Tue, Oct 04, 2016 at 12:02:40PM +0200, Karel Zak wrote:
> On Tue, Oct 04, 2016 at 01:18:23AM -0800, Kent Overstreet wrote:
> > Yes - what I'm saying is that we shouldn't quit trying to mount RW with _other_
> > filesystem types. Or alternatively, we should only attempt to mount RO after
> > that _particular_ driver has returned EACCES/EROFS.
> > 
> > The bug is that the global context is flipped to RO, not just for attempting
> > with that filesystem type.
> 
> Hmm.. I will try to improve it. The problem is that mount(8) interprets 
> EACCES/EROFS as information about the device, then flip to RO makes sense 
> for all next mount(2) attempts.
> 
> > > > The end result is that if we're trying to mount by trying every filesystem type
> > > > (your libblkid doesn't know about your filesystem yet..), and the correct
> > > > filesystem was listed after iso9600 in /proc/filesystems, mount will always
> > > > mount RO (unless you specify the filesystem type with -t).
> > > 
> > > Not sure if I understand. Does it mean that iso9600 driver returns
> > > EACCES for all devices although there is no this FS on the device? Or
> > > your FS shares the device with iso9600?
> > 
> > Yes, iso9660 return EACCES when no iso9600 filesystem is present.
> 
> 
> 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);
> }
> 
> This is crazy... iso9600 driver starts analyze mount options although
> the mount request is maybe completely irrelevant for the driver and 
> there is no iso9600 on the device. 
> 
> If we will write FS drivers in this way then old good "try all from /{proc,etc}/filesystems"
> will be useless...
> 
> See another filesystems, for example ext4, first be sure there is
> superblock and magic string (or return EINVAL) and then try 
> validate mount options.
> 
> CC to Jan Kara (he did the kernel change in Jun 2013).

Given that the error code is coming from driver code, I think taking it to mean
anything about the underlying device is going to be flakey at best - we could
fix iso9660, sure, but there's a crap ton of filesystems in the kernel and I'm
certainly not going to try to audit all their error paths. Even if a driver
waits until after it verifies its magic string before returnig an error, you
have cases where multiple drivers may be able to mount the filesystem or - even
more fun - different filesystem types may have superblocks that don't live at
the same offset, so the presense of ext4's magic number doesn't mean that not
actually a completely different filesystem type (don't laugh! I've actually been
bit by this).

What's wrong with just changing mount(8) to only flip to RO for further attempts
with that particular filesystem type?

Re: Bug with read only handling in mount

[Karel Zak]

On Tue, Oct 04, 2016 at 02:10:39AM -0800, Kent Overstreet wrote:
> > with that filesystem type.
> > 
> > Hmm.. I will try to improve it. The problem is that mount(8) interprets 
> > EACCES/EROFS as information about the device, then flip to RO makes sense 
> > for all next mount(2) attempts.
> > 
> > > > > The end result is that if we're trying to mount by trying every filesystem type
> > > > > (your libblkid doesn't know about your filesystem yet..), and the correct
> > > > > filesystem was listed after iso9600 in /proc/filesystems, mount will always
> > > > > mount RO (unless you specify the filesystem type with -t).
> > > > 
> > > > Not sure if I understand. Does it mean that iso9600 driver returns
> > > > EACCES for all devices although there is no this FS on the device? Or
> > > > your FS shares the device with iso9600?
> > > 
> > > Yes, iso9660 return EACCES when no iso9600 filesystem is present.
> > 
> > 
> > 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);
> > }
> > 
> > This is crazy... iso9600 driver starts analyze mount options although
> > the mount request is maybe completely irrelevant for the driver and 
> > there is no iso9600 on the device. 
> > 
> > If we will write FS drivers in this way then old good "try all from /{proc,etc}/filesystems"
> > will be useless...
> > 
> > See another filesystems, for example ext4, first be sure there is
> > superblock and magic string (or return EINVAL) and then try 
> > validate mount options.
> > 
> > CC to Jan Kara (he did the kernel change in Jun 2013).
> 
> Given that the error code is coming from driver code, I think taking it to mean
> anything about the underlying device is going to be flakey at best - we could

That's what we do in last 20 years, good to know it's wrong now.

> fix iso9660, sure, but there's a crap ton of filesystems in the kernel and I'm
> certainly not going to try to audit all their error paths. Even if a driver
> waits until after it verifies its magic string before returnig an error, you
> have cases where multiple drivers may be able to mount the filesystem or - even
> more fun - different filesystem types may have superblocks that don't live at
> the same offset, so the presense of ext4's magic number doesn't mean that not
> actually a completely different filesystem type (don't laugh! I've actually been
> bit by this).

:-)

> What's wrong with just changing mount(8) to only flip to RO for further attempts
> with that particular filesystem type?
 
As I said, I'll improve mount(8).

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Bug with read only handling in mount

[Jan Kara]

On Tue 04-10-16 12:02:40, Karel Zak wrote:
> On Tue, Oct 04, 2016 at 01:18:23AM -0800, Kent Overstreet wrote:
> > On Tue, Oct 04, 2016 at 10:29:09AM +0200, Karel Zak wrote:
> > > On Mon, Oct 03, 2016 at 11:41:32PM -0800, Kent Overstreet wrote:
> > > > sys-utils/mount.c, mk_exit_code()
> > > > 
> > > > If the mount syscall returns EACCESS, the code treats this as meaning that RW
> > > > access to the block device wasn't allowed - it switches to RO for all future
> > > > mount attempts.
> > > 
> > > This is pretty old (>10years) mount behavior, util-linux 2.13:
> > > 
> > >     case EACCES:  /* pre-linux 1.1.38, 1.1.41 and later */
> > >     case EROFS:   /* linux 1.1.38 and later */
> > > 
> > > > This is incorrect though, because EACCESS could just mean that that particular
> > > > filesystem doesn't support RW: iso9600 returns EACCESS if you try to mount RW.
> > > 
> > > So, remount RO makes sense, right? I don't think we want to change
> > > this behavior, all CDROM/DVD users depend on this.
> > 
> > Yes - what I'm saying is that we shouldn't quit trying to mount RW with _other_
> > filesystem types. Or alternatively, we should only attempt to mount RO after
> > that _particular_ driver has returned EACCES/EROFS.
> > 
> > The bug is that the global context is flipped to RO, not just for attempting
> > with that filesystem type.
> 
> Hmm.. I will try to improve it. The problem is that mount(8) interprets 
> EACCES/EROFS as information about the device, then flip to RO makes sense 
> for all next mount(2) attempts.
> 
> > > > The end result is that if we're trying to mount by trying every filesystem type
> > > > (your libblkid doesn't know about your filesystem yet..), and the correct
> > > > filesystem was listed after iso9600 in /proc/filesystems, mount will always
> > > > mount RO (unless you specify the filesystem type with -t).
> > > 
> > > Not sure if I understand. Does it mean that iso9600 driver returns
> > > EACCES for all devices although there is no this FS on the device? Or
> > > your FS shares the device with iso9600?
> > 
> > Yes, iso9660 return EACCES when no iso9600 filesystem is present.
> 
> 
> 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);
> }
> 
> This is crazy... iso9600 driver starts analyze mount options although
> the mount request is maybe completely irrelevant for the driver and 
> there is no iso9600 on the device. 
> 
> If we will write FS drivers in this way then old good "try all from
> /{proc,etc}/filesystems" will be useless...
> 
> See another filesystems, for example ext4, first be sure there is
> superblock and magic string (or return EINVAL) and then try 
> validate mount options.
> 
> CC to Jan Kara (he did the kernel change in Jun 2013).

Good point. I'll fix iso9660.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: Bug with read only handling in mount

[Karel Zak]

On Tue, Oct 04, 2016 at 12:49:43PM +0200, Jan Kara wrote:
> > 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);
> > }
> > 
> > This is crazy... iso9600 driver starts analyze mount options although
> > the mount request is maybe completely irrelevant for the driver and 
> > there is no iso9600 on the device. 
> > 
> > If we will write FS drivers in this way then old good "try all from
> > /{proc,etc}/filesystems" will be useless...
> > 
> > See another filesystems, for example ext4, first be sure there is
> > superblock and magic string (or return EINVAL) and then try 
> > validate mount options.
> > 
> > CC to Jan Kara (he did the kernel change in Jun 2013).
> 
> Good point. I'll fix iso9660.

Thanks! 

I have a patch for mount(8), but it's rather complex, because the
/{proc,etc}/filesystems loop is hidden in libmount. I guess move the
MS_RDONLY check in the iso9660 kernel driver will be easy fix.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com