[PATCH 1/3] autofs: drop dentry reference only when it is never used

[PATCH 1/3] autofs: drop dentry reference only when it is never used

[Ian Kent]

From: Pan Bian <bianpan2016@163.com>

The function autofs_expire_run calls dput(dentry) to drop the reference
count of dentry. However, dentry is read via autofs_dentry_ino(dentry)
after that. This may result in a use-free-bug. The patch drops the
reference count of dentry only when it is never used.

Signed-off-by: Pan Bian <bianpan2016@163.com>
Acked-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/expire.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/autofs/expire.c b/fs/autofs/expire.c
index d441244b79df..28d9c2b1b3bb 100644
--- a/fs/autofs/expire.c
+++ b/fs/autofs/expire.c
@@ -596,7 +596,6 @@ int autofs_expire_run(struct super_block *sb,
 	pkt.len = dentry->d_name.len;
 	memcpy(pkt.name, dentry->d_name.name, pkt.len);
 	pkt.name[pkt.len] = '\0';
-	dput(dentry);
 
 	if (copy_to_user(pkt_p, &pkt, sizeof(struct autofs_packet_expire)))
 		ret = -EFAULT;
@@ -609,6 +608,8 @@ int autofs_expire_run(struct super_block *sb,
 	complete_all(&ino->expire_complete);
 	spin_unlock(&sbi->fs_lock);
 
+	dput(dentry);
+
 	return ret;
 }
 

[PATCH 1/3] autofs: drop dentry reference only when it is never used

[Ian Kent]

From: Pan Bian <bianpan2016@163.com>

The function autofs_expire_run calls dput(dentry) to drop the reference
count of dentry. However, dentry is read via autofs_dentry_ino(dentry)
after that. This may result in a use-free-bug. The patch drops the
reference count of dentry only when it is never used.

Signed-off-by: Pan Bian <bianpan2016@163.com>
Acked-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/expire.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/autofs/expire.c b/fs/autofs/expire.c
index d441244b79df..28d9c2b1b3bb 100644
--- a/fs/autofs/expire.c
+++ b/fs/autofs/expire.c
@@ -596,7 +596,6 @@ int autofs_expire_run(struct super_block *sb,
 	pkt.len = dentry->d_name.len;
 	memcpy(pkt.name, dentry->d_name.name, pkt.len);
 	pkt.name[pkt.len] = '\0';
-	dput(dentry);
 
 	if (copy_to_user(pkt_p, &pkt, sizeof(struct autofs_packet_expire)))
 		ret = -EFAULT;
@@ -609,6 +608,8 @@ int autofs_expire_run(struct super_block *sb,
 	complete_all(&ino->expire_complete);
 	spin_unlock(&sbi->fs_lock);
 
+	dput(dentry);
+
 	return ret;
 }
 

[PATCH 2/3] autofs - fix error return in autofs_fill_super()

[Ian Kent]

In autofs_fill_super() on error of get inode/make root dentry the
return should be ENOMEM as this is the only failure case of the
called functions.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/inode.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index 0e8ea2d9a2bb..078992eee299 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -266,8 +266,10 @@ int autofs_fill_super(struct super_block *s, void *data, int silent)
 	}
 	root_inode = autofs_get_inode(s, S_IFDIR | 0755);
 	root = d_make_root(root_inode);
-	if (!root)
+	if (!root) {
+		ret = -ENOMEM;
 		goto fail_ino;
+	}
 	pipe = NULL;
 
 	root->d_fsdata = ino;

[PATCH 2/3] autofs - fix error return in autofs_fill_super()

[Ian Kent]

In autofs_fill_super() on error of get inode/make root dentry the
return should be ENOMEM as this is the only failure case of the
called functions.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/inode.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index 0e8ea2d9a2bb..078992eee299 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -266,8 +266,10 @@ int autofs_fill_super(struct super_block *s, void *data, int silent)
 	}
 	root_inode = autofs_get_inode(s, S_IFDIR | 0755);
 	root = d_make_root(root_inode);
-	if (!root)
+	if (!root) {
+		ret = -ENOMEM;
 		goto fail_ino;
+	}
 	pipe = NULL;
 
 	root->d_fsdata = ino;

[PATCH 3/3] autofs: add ignore mount option

[Ian Kent]

Add an autofs file system mount option that can be used to provide
a generic indicator to applications that the mount entry should be
ignored when displaying mount information.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/autofs_i.h         |    1 +
 fs/autofs/inode.c            |    9 ++++++++-
 include/uapi/linux/auto_fs.h |    2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/autofs/autofs_i.h b/fs/autofs/autofs_i.h
index 3e59f0ed777b..b735f2b1e462 100644
--- a/fs/autofs/autofs_i.h
+++ b/fs/autofs/autofs_i.h
@@ -105,6 +105,7 @@ struct autofs_wait_queue {
 
 #define AUTOFS_SBI_CATATONIC	0x0001
 #define AUTOFS_SBI_STRICTEXPIRE 0x0002
+#define AUTOFS_SBI_IGNORE	0x0004
 
 struct autofs_sb_info {
 	u32 magic;
diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index 078992eee299..8647ecaa89fc 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -89,6 +89,8 @@ static int autofs_show_options(struct seq_file *m, struct dentry *root)
 		seq_printf(m, ",indirect");
 	if (sbi->flags & AUTOFS_SBI_STRICTEXPIRE)
 		seq_printf(m, ",strictexpire");
+	if (sbi->flags & AUTOFS_SBI_IGNORE)
+		seq_printf(m, ",ignore");
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	if (sbi->pipe)
 		seq_printf(m, ",pipe_ino=%ld", file_inode(sbi->pipe)->i_ino);
@@ -111,7 +113,8 @@ static const struct super_operations autofs_sops = {
 };
 
 enum {Opt_err, Opt_fd, Opt_uid, Opt_gid, Opt_pgrp, Opt_minproto, Opt_maxproto,
-	Opt_indirect, Opt_direct, Opt_offset, Opt_strictexpire};
+	Opt_indirect, Opt_direct, Opt_offset, Opt_strictexpire,
+	Opt_ignore};
 
 static const match_table_t tokens = {
 	{Opt_fd, "fd=%u"},
@@ -124,6 +127,7 @@ static const match_table_t tokens = {
 	{Opt_direct, "direct"},
 	{Opt_offset, "offset"},
 	{Opt_strictexpire, "strictexpire"},
+	{Opt_ignore, "ignore"},
 	{Opt_err, NULL}
 };
 
@@ -206,6 +210,9 @@ static int parse_options(char *options,
 		case Opt_strictexpire:
 			sbi->flags |= AUTOFS_SBI_STRICTEXPIRE;
 			break;
+		case Opt_ignore:
+			sbi->flags |= AUTOFS_SBI_IGNORE;
+			break;
 		default:
 			return 1;
 		}
diff --git a/include/uapi/linux/auto_fs.h b/include/uapi/linux/auto_fs.h
index 082119630b49..1f7925afad2d 100644
--- a/include/uapi/linux/auto_fs.h
+++ b/include/uapi/linux/auto_fs.h
@@ -23,7 +23,7 @@
 #define AUTOFS_MIN_PROTO_VERSION	3
 #define AUTOFS_MAX_PROTO_VERSION	5
 
-#define AUTOFS_PROTO_SUBVERSION		4
+#define AUTOFS_PROTO_SUBVERSION		5
 
 /*
  * The wait_queue_token (autofs_wqt_t) is part of a structure which is passed

[PATCH 3/3] autofs: add ignore mount option

[Ian Kent]

Add an autofs file system mount option that can be used to provide
a generic indicator to applications that the mount entry should be
ignored when displaying mount information.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fs/autofs/autofs_i.h         |    1 +
 fs/autofs/inode.c            |    9 ++++++++-
 include/uapi/linux/auto_fs.h |    2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/autofs/autofs_i.h b/fs/autofs/autofs_i.h
index 3e59f0ed777b..b735f2b1e462 100644
--- a/fs/autofs/autofs_i.h
+++ b/fs/autofs/autofs_i.h
@@ -105,6 +105,7 @@ struct autofs_wait_queue {
 
 #define AUTOFS_SBI_CATATONIC	0x0001
 #define AUTOFS_SBI_STRICTEXPIRE 0x0002
+#define AUTOFS_SBI_IGNORE	0x0004
 
 struct autofs_sb_info {
 	u32 magic;
diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index 078992eee299..8647ecaa89fc 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -89,6 +89,8 @@ static int autofs_show_options(struct seq_file *m, struct dentry *root)
 		seq_printf(m, ",indirect");
 	if (sbi->flags & AUTOFS_SBI_STRICTEXPIRE)
 		seq_printf(m, ",strictexpire");
+	if (sbi->flags & AUTOFS_SBI_IGNORE)
+		seq_printf(m, ",ignore");
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	if (sbi->pipe)
 		seq_printf(m, ",pipe_ino=%ld", file_inode(sbi->pipe)->i_ino);
@@ -111,7 +113,8 @@ static const struct super_operations autofs_sops = {
 };
 
 enum {Opt_err, Opt_fd, Opt_uid, Opt_gid, Opt_pgrp, Opt_minproto, Opt_maxproto,
-	Opt_indirect, Opt_direct, Opt_offset, Opt_strictexpire};
+	Opt_indirect, Opt_direct, Opt_offset, Opt_strictexpire,
+	Opt_ignore};
 
 static const match_table_t tokens = {
 	{Opt_fd, "fd=%u"},
@@ -124,6 +127,7 @@ static const match_table_t tokens = {
 	{Opt_direct, "direct"},
 	{Opt_offset, "offset"},
 	{Opt_strictexpire, "strictexpire"},
+	{Opt_ignore, "ignore"},
 	{Opt_err, NULL}
 };
 
@@ -206,6 +210,9 @@ static int parse_options(char *options,
 		case Opt_strictexpire:
 			sbi->flags |= AUTOFS_SBI_STRICTEXPIRE;
 			break;
+		case Opt_ignore:
+			sbi->flags |= AUTOFS_SBI_IGNORE;
+			break;
 		default:
 			return 1;
 		}
diff --git a/include/uapi/linux/auto_fs.h b/include/uapi/linux/auto_fs.h
index 082119630b49..1f7925afad2d 100644
--- a/include/uapi/linux/auto_fs.h
+++ b/include/uapi/linux/auto_fs.h
@@ -23,7 +23,7 @@
 #define AUTOFS_MIN_PROTO_VERSION	3
 #define AUTOFS_MAX_PROTO_VERSION	5
 
-#define AUTOFS_PROTO_SUBVERSION		4
+#define AUTOFS_PROTO_SUBVERSION		5
 
 /*
  * The wait_queue_token (autofs_wqt_t) is part of a structure which is passed

Re: [PATCH 3/3] autofs: add ignore mount option

[Andrew Morton]

On Sat, 12 Jan 2019 08:00:40 +0800 Ian Kent <raven@themaw.net> wrote:

> Add an autofs file system mount option that can be used to provide
> a generic indicator to applications that the mount entry should be
> ignored when displaying mount information.

What is the reason for adding this feature?

Re: [PATCH 3/3] autofs: add ignore mount option

[Ian Kent]

On Tue, 2019-01-29 at 17:16 -0800, Andrew Morton wrote:
> On Sat, 12 Jan 2019 08:00:40 +0800 Ian Kent <raven@themaw.net> wrote:
> 
> > Add an autofs file system mount option that can be used to provide
> > a generic indicator to applications that the mount entry should be
> > ignored when displaying mount information.
> 
> What is the reason for adding this feature?

In other OSes that provide autofs and that provide a mount list
to user space based on the kernel mount list a no-op mount option
("ignore" is the one use on the most common OS) is allowed so that
autofs file system users can optionally use it.

The idea is that it be used by user space programs to exclude
autofs mounts from consideration when reading the mounts list.

Prior to the change to link /etc/mtab to /proc/self/mounts all
I needed to do to achieve this was to use mount(2) and not update
the mtab but now that no longer works.

I know the symlinking happened a long time ago and I considered
doing this then but, at the time I couldn't remember the commonly
used option name and thought persuading the various utility
maintainers would be too hard.

But now I have a RHEL request to do this for compatibility for a
widely used product so I want to go ahead with it and try and
enlist the help of some utility package maintainers.

Clearly, without the option nothing can be done so it's at least
a start.

Ian

Re: [PATCH 3/3] autofs: add ignore mount option

[Ian Kent]

On Wed, 2019-01-30 at 10:07 +0800, Ian Kent wrote:
> On Tue, 2019-01-29 at 17:16 -0800, Andrew Morton wrote:
> > On Sat, 12 Jan 2019 08:00:40 +0800 Ian Kent <raven@themaw.net> wrote:
> > 
> > > Add an autofs file system mount option that can be used to provide
> > > a generic indicator to applications that the mount entry should be
> > > ignored when displaying mount information.
> > 
> > What is the reason for adding this feature?
> 
> In other OSes that provide autofs and that provide a mount list
> to user space based on the kernel mount list a no-op mount option
> ("ignore" is the one use on the most common OS) is allowed so that
> autofs file system users can optionally use it.
> 
> The idea is that it be used by user space programs to exclude
> autofs mounts from consideration when reading the mounts list.
> 
> Prior to the change to link /etc/mtab to /proc/self/mounts all
> I needed to do to achieve this was to use mount(2) and not update
> the mtab but now that no longer works.
> 
> I know the symlinking happened a long time ago and I considered
> doing this then but, at the time I couldn't remember the commonly
> used option name and thought persuading the various utility
> maintainers would be too hard.
> 
> But now I have a RHEL request to do this for compatibility for a
> widely used product so I want to go ahead with it and try and
> enlist the help of some utility package maintainers.

Al,

On a different note the above request also raised another
question about statvfs(3) automount behaviour.

In glibc statvfs(3) uses statfs(2) and translates the return
to a statvfs structure.

I wasn't aware but apparently statvfs(3) (and presumably statfs(2))
doesn't trigger an automount on Solaris whereas we do. I think
statfs() is probably the only exception to the convention that
stat family system calls don't trigger an automount.

So far I've said that this is a long standing behaviour in the
Linux kernel and changing it could lead to unpleasant surprises
for those that have come to expect this behaviour so such a change
would not be well received.

But I do need to ask your opinion, so what are your thoughts about
changing this?

Ian

Re: [PATCH 3/3] autofs: add ignore mount option

[Al Viro]

On Wed, Jan 30, 2019 at 10:44:15AM +0800, Ian Kent wrote:

> Al,
> 
> On a different note the above request also raised another
> question about statvfs(3) automount behaviour.
> 
> In glibc statvfs(3) uses statfs(2) and translates the return
> to a statvfs structure.
> 
> I wasn't aware but apparently statvfs(3) (and presumably statfs(2))
> doesn't trigger an automount on Solaris whereas we do. I think
> statfs() is probably the only exception to the convention that
> stat family system calls don't trigger an automount.
> 
> So far I've said that this is a long standing behaviour in the
> Linux kernel and changing it could lead to unpleasant surprises
> for those that have come to expect this behaviour so such a change
> would not be well received.
> 
> But I do need to ask your opinion, so what are your thoughts about
> changing this?

Probably should've done it that way, but I'm afraid it's much too
late by now...

Re: [PATCH 3/3] autofs: add ignore mount option

[Ian Kent]

On Wed, 2019-01-30 at 04:18 +0000, Al Viro wrote:
> On Wed, Jan 30, 2019 at 10:44:15AM +0800, Ian Kent wrote:
> 
> > Al,
> > 
> > On a different note the above request also raised another
> > question about statvfs(3) automount behaviour.
> > 
> > In glibc statvfs(3) uses statfs(2) and translates the return
> > to a statvfs structure.
> > 
> > I wasn't aware but apparently statvfs(3) (and presumably statfs(2))
> > doesn't trigger an automount on Solaris whereas we do. I think
> > statfs() is probably the only exception to the convention that
> > stat family system calls don't trigger an automount.
> > 
> > So far I've said that this is a long standing behaviour in the
> > Linux kernel and changing it could lead to unpleasant surprises
> > for those that have come to expect this behaviour so such a change
> > would not be well received.
> > 
> > But I do need to ask your opinion, so what are your thoughts about
> > changing this?
> 
> Probably should've done it that way, but I'm afraid it's much too
> late by now...

Yeah, thought you'd say that.
Thanks for confirming.

Ian

Re: [PATCH 3/3] autofs: add ignore mount option

[Andrew Morton]

On Wed, 30 Jan 2019 10:07:15 +0800 Ian Kent <raven@themaw.net> wrote:

> On Tue, 2019-01-29 at 17:16 -0800, Andrew Morton wrote:
> > On Sat, 12 Jan 2019 08:00:40 +0800 Ian Kent <raven@themaw.net> wrote:
> > 
> > > Add an autofs file system mount option that can be used to provide
> > > a generic indicator to applications that the mount entry should be
> > > ignored when displaying mount information.
> > 
> > What is the reason for adding this feature?
> 
> In other OSes that provide autofs and that provide a mount list
> to user space based on the kernel mount list a no-op mount option
> ("ignore" is the one use on the most common OS) is allowed so that
> autofs file system users can optionally use it.
> 
> The idea is that it be used by user space programs to exclude
> autofs mounts from consideration when reading the mounts list.
> 
> Prior to the change to link /etc/mtab to /proc/self/mounts all
> I needed to do to achieve this was to use mount(2) and not update
> the mtab but now that no longer works.
> 
> I know the symlinking happened a long time ago and I considered
> doing this then but, at the time I couldn't remember the commonly
> used option name and thought persuading the various utility
> maintainers would be too hard.
> 
> But now I have a RHEL request to do this for compatibility for a
> widely used product so I want to go ahead with it and try and
> enlist the help of some utility package maintainers.
> 
> Clearly, without the option nothing can be done so it's at least
> a start.

OK.  I guess I can just paste the above into the changelog.

Also, Documentation/filesystems/autofs*.txt are owed an update?

Re: [PATCH 3/3] autofs: add ignore mount option

[Ian Kent]

On Tue, 2019-01-29 at 20:58 -0800, Andrew Morton wrote:
> On Wed, 30 Jan 2019 10:07:15 +0800 Ian Kent <raven@themaw.net> wrote:
> 
> > On Tue, 2019-01-29 at 17:16 -0800, Andrew Morton wrote:
> > > On Sat, 12 Jan 2019 08:00:40 +0800 Ian Kent <raven@themaw.net> wrote:
> > > 
> > > > Add an autofs file system mount option that can be used to provide
> > > > a generic indicator to applications that the mount entry should be
> > > > ignored when displaying mount information.
> > > 
> > > What is the reason for adding this feature?
> > 
> > In other OSes that provide autofs and that provide a mount list
> > to user space based on the kernel mount list a no-op mount option
> > ("ignore" is the one use on the most common OS) is allowed so that
> > autofs file system users can optionally use it.
> > 
> > The idea is that it be used by user space programs to exclude
> > autofs mounts from consideration when reading the mounts list.
> > 
> > Prior to the change to link /etc/mtab to /proc/self/mounts all
> > I needed to do to achieve this was to use mount(2) and not update
> > the mtab but now that no longer works.
> > 
> > I know the symlinking happened a long time ago and I considered
> > doing this then but, at the time I couldn't remember the commonly
> > used option name and thought persuading the various utility
> > maintainers would be too hard.
> > 
> > But now I have a RHEL request to do this for compatibility for a
> > widely used product so I want to go ahead with it and try and
> > enlist the help of some utility package maintainers.
> > 
> > Clearly, without the option nothing can be done so it's at least
> > a start.
> 
> OK.  I guess I can just paste the above into the changelog.

I thought this description would be too long but, by all means,
replace or add it to the changelog.

Or, if you prefer, I could try and come up with something more
succinct, the above explanation probably goes into more detail
than is really needed to get the message across.

> 
> Also, Documentation/filesystems/autofs*.txt are owed an update?

Yes, I think so, I'll have a look at it and get onto it, thanks
for the reminder.

Ian