[PATCH] fs: print warning when mount flags was ignored

[PATCH] fs: print warning when mount flags was ignored

[Lukas Czerner]

Some mount flags can conflict with each other so they can not be
handled together. Currently when conflicting flags are specified,
some of them are silently ignored putting user in believe that
they was handled correctly.

Fix this by checking for unhandled flags and print warning when
flags was ignored. I am not sure if it's worth translating the flags
into the name, so it just prints the flag as a hexadecimal number.

Obviously we can not fail when conflicting flags are specified,
because it may break buggy application, but it can change in the
future.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Milan Broz <mbroz@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/namespace.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index e608199..8736a48 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2175,18 +2175,32 @@ long do_mount(char *dev_name, char *dir_name, char *type_page,
 		   MS_NOATIME | MS_NODIRATIME | MS_RELATIME| MS_KERNMOUNT |
 		   MS_STRICTATIME);
 
-	if (flags & MS_REMOUNT)
+	if (flags & MS_REMOUNT) {
 		retval = do_remount(&path, flags & ~MS_REMOUNT, mnt_flags,
 				    data_page);
-	else if (flags & MS_BIND)
+		flags &= ~MS_REMOUNT;
+	} else if (flags & MS_BIND) {
 		retval = do_loopback(&path, dev_name, flags & MS_REC);
-	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
+		flags &= ~MS_BIND;
+	} else if (flags & (MS_SHARED | MS_PRIVATE |
+			    MS_SLAVE | MS_UNBINDABLE)) {
 		retval = do_change_type(&path, flags);
-	else if (flags & MS_MOVE)
+		flags &= ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE);
+	} else if (flags & MS_MOVE) {
 		retval = do_move_mount(&path, dev_name);
-	else
+		flags &= ~MS_MOVE;
+	} else
 		retval = do_new_mount(&path, type_page, flags, mnt_flags,
 				      dev_name, data_page);
+
+	flags &= (MS_REMOUNT | MS_BIND | MS_SHARED | MS_PRIVATE |
+		  MS_SLAVE | MS_UNBINDABLE | MS_MOVE);
+
+	if (!retval && flags)
+		printk(KERN_WARNING "%s(%u): (%s -> %s) Conflicting mount flags"
+				    " specified. These flags has been "
+				    "ignored: %#.8lx\n", __func__, current->pid,
+				    dev_name, dir_name, flags);
 dput_out:
 	path_put(&path);
 	return retval;
-- 
1.7.4.4

Re: [PATCH] fs: print warning when mount flags was ignored

[Karel Zak]

On Thu, Apr 19, 2012 at 11:07:55AM +0200, Lukas Czerner wrote:
> Some mount flags can conflict with each other so they can not be
> handled together. Currently when conflicting flags are specified,
> some of them are silently ignored putting user in believe that
> they was handled correctly.

Unfortunately, it's not so simple ;-)

> -	if (flags & MS_REMOUNT)
> +	if (flags & MS_REMOUNT) {
>  		retval = do_remount(&path, flags & ~MS_REMOUNT, mnt_flags,
>  				    data_page);
> -	else if (flags & MS_BIND)
> +		flags &= ~MS_REMOUNT;

This is incorrect, the flags may also include many others flags. For
example MS_REMOUNT|MS_BIND|MS_RDONLY is valid (see do_remoun() code).

And it's normal that for "mount -o remount" the mount command reads
flags from mtab/fstab so it includes for example MS_RELATIME, ...

> +	} else if (flags & MS_BIND) {
>  		retval = do_loopback(&path, dev_name, flags & MS_REC);
> -	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
> +		flags &= ~MS_BIND;

 what about MS_REC ?

> +	} else if (flags & (MS_SHARED | MS_PRIVATE |
> +			    MS_SLAVE | MS_UNBINDABLE)) {
>  		retval = do_change_type(&path, flags);
> -	else if (flags & MS_MOVE)
> +		flags &= ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE);

 what about MS_REC ?

 Note that do_change_type() already checks for unexpected flags and
 returns -EINVAL if more flags are specified.

> +	} else if (flags & MS_MOVE) {
>  		retval = do_move_mount(&path, dev_name);
> -	else
> +		flags &= ~MS_MOVE;
> +	} else
>  		retval = do_new_mount(&path, type_page, flags, mnt_flags,
>  				      dev_name, data_page);
> +
> +	flags &= (MS_REMOUNT | MS_BIND | MS_SHARED | MS_PRIVATE |
> +		  MS_SLAVE | MS_UNBINDABLE | MS_MOVE);
> +
> +	if (!retval && flags)
> +		printk(KERN_WARNING "%s(%u): (%s -> %s) Conflicting mount flags"
> +				    " specified. These flags has been "
> +				    "ignored: %#.8lx\n", __func__, current->pid,
> +				    dev_name, dir_name, flags);


    Karel

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

Re: [PATCH] fs: print warning when mount flags was ignored

[Lukas Czerner]

On Thu, 19 Apr 2012, Karel Zak wrote:

> On Thu, Apr 19, 2012 at 11:07:55AM +0200, Lukas Czerner wrote:
> > Some mount flags can conflict with each other so they can not be
> > handled together. Currently when conflicting flags are specified,
> > some of them are silently ignored putting user in believe that
> > they was handled correctly.
> 
> Unfortunately, it's not so simple ;-)
> 
> > -	if (flags & MS_REMOUNT)
> > +	if (flags & MS_REMOUNT) {
> >  		retval = do_remount(&path, flags & ~MS_REMOUNT, mnt_flags,
> >  				    data_page);
> > -	else if (flags & MS_BIND)
> > +		flags &= ~MS_REMOUNT;
> 
> This is incorrect, the flags may also include many others flags. For
> example MS_REMOUNT|MS_BIND|MS_RDONLY is valid (see do_remoun() code).

ah, right we can specify more flags to be consumed by do_remount and
others. Well, then we can just mask them all out like this:

	flags &= ~(MS_REMOUNT | MS_BIND );

> 
> And it's normal that for "mount -o remount" the mount command reads
> flags from mtab/fstab so it includes for example MS_RELATIME, ...

Yes, but MS_RELATIME is masked out from flags and it is used in
mnt_flags which are handled by remount. That makes me wonder why we even
have MS_RELATIME flag, since it is not used anywhere.

> 
> > +	} else if (flags & MS_BIND) {
> >  		retval = do_loopback(&path, dev_name, flags & MS_REC);
> > -	else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE))
> > +		flags &= ~MS_BIND;
> 
>  what about MS_REC ?

Ok, we should put MS_REC into the mix since the do_remount does not use
it.

	flags &= ~(MS_BIND | MS_REC);

> 
> > +	} else if (flags & (MS_SHARED | MS_PRIVATE |
> > +			    MS_SLAVE | MS_UNBINDABLE)) {
> >  		retval = do_change_type(&path, flags);
> > -	else if (flags & MS_MOVE)
> > +		flags &= ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE);
> 
>  what about MS_REC ?

	flags &= ~(MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE |
		   MS_REC);
> 
>  Note that do_change_type() already checks for unexpected flags and
>  returns -EINVAL if more flags are specified.

I am not sure how this is related. Each of those function should
probably do that but do_change_type() is actually the only one
doing so.

> 
> > +	} else if (flags & MS_MOVE) {
> >  		retval = do_move_mount(&path, dev_name);
> > -	else
> > +		flags &= ~MS_MOVE;
> > +	} else
> >  		retval = do_new_mount(&path, type_page, flags, mnt_flags,
> >  				      dev_name, data_page);
> > +
> > +	flags &= (MS_REMOUNT | MS_BIND | MS_SHARED | MS_PRIVATE |
> > +		  MS_SLAVE | MS_UNBINDABLE | MS_MOVE);

	flags &= (MS_REMOUNT | MS_BIND | MS_SHARED | MS_PRIVATE |
		  MS_SLAVE | MS_UNBINDABLE | MS_MOVE | MS_REC);

> > +
> > +	if (!retval && flags)
> > +		printk(KERN_WARNING "%s(%u): (%s -> %s) Conflicting mount flags"
> > +				    " specified. These flags has been "
> > +				    "ignored: %#.8lx\n", __func__, current->pid,
> > +				    dev_name, dir_name, flags);

But then, we'll might ignoring mnt_flags silently since they are masked
out earlier. Well, maybe we could mas them out only do_remount() and
do_new_mount().

>     Karel

Thanks!
-Lukas