Skip to contentSkip to conversation

autofs: fix autofs_v5_packet dlivery in compat mode

RFC PATCH In review Stanislav Kinsburskiy 2 participants 10 messages Sep 1, 2017 – Sep 14, 2017 Kernel automounter

lore
Share & tools
lore
#0 lore raw

cover autofs: fix autofs_v5_packet dlivery in compat mode

The problem is that in compat mode struct autofs_v5_packet has to have different size (i.e. 4 bytes less).

This is RFC because:
1) This issue is hidden, because autofs pipe has O_DIRECT and the rest of the epacket is truncated when read.
2) X86 arch doesn't have is_compat_task() helper
3) It's now clear, what to do if "pgrp" option is specified.

The following series implements...

---

Stanislav Kinsburskiy (2):
autofs: set compat flag on sbi when daemon uses 32bit addressation
autofs: sent 32-bit sized packet for 32-bit process

 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 fs/autofs4/waitq.c     |    5 +++++
 4 files changed, 14 insertions(+), 1 deletion(-)

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

#1 lore raw

RFC PATCH 1/2 autofs: set compat flag on sbi when daemon uses 32bit addressation

---
 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)
fs/autofs4/autofs_i.h +3 -0
index 4737615..3da105f 100644
@@ -120,6 +120,9 @@ struct autofs_sb_info {
 	struct list_head active_list;
 	struct list_head expiring_list;
 	struct rcu_head rcu;
+#ifdef CONFIG_COMPAT
+	unsigned is32bit:1;
+#endif
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
fs/autofs4/dev-ioctl.c +3 -0
index b7c816f..467d6c4 100644
@@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
+#ifdef CONFIG_COMPAT
+		sbi->is32bit = is_compat_task();
+#endif
 	}
 out:
 	put_pid(new_pid);
fs/autofs4/inode.c +3 -1
index 09e7d68..21d3c0b 100644
@@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	} else {
 		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
 	}
-
+#ifdef CONFIG_COMPAT
+	sbi->is32bit = is_compat_task();
+#endif
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

maintainer
#2 lore raw

Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

Quoted 41 lines · Stanislav Kinsburskiy
---
 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)
fs/autofs4/autofs_i.h +3 -0
index 4737615..3da105f 100644
@@ -120,6 +120,9 @@ struct autofs_sb_info {
 	struct list_head active_list;
 	struct list_head expiring_list;
 	struct rcu_head rcu;
+#ifdef CONFIG_COMPAT
+	unsigned is32bit:1;
+#endif
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
fs/autofs4/dev-ioctl.c +3 -0
index b7c816f..467d6c4 100644
@@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
+#ifdef CONFIG_COMPAT
+		sbi->is32bit = is_compat_task();
+#endif
 	}
 out:
 	put_pid(new_pid);
fs/autofs4/inode.c +0 -0
index 09e7d68..21d3c0b 100644

fs/autofs4/inode.c int autofs4_fill_super(struct super_block *s, void *data, int silent) · line 309

Quoted 10 lines
fs/autofs4/inode.c +3 -1
@@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	} else {
 		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
 	}
-
+#ifdef CONFIG_COMPAT
+	sbi->is32bit = is_compat_task();
+#endif
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 

Not sure about this.

Don't you think it would be better to avoid the in code #ifdefs by doing some checks and defines in the header file and defining what's need to just use is_compat_task().

Not sure 2 patches are needed for this either ......

Ian
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

#3 lore raw

Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

14.09.2017 02:38, Ian Kent пишет:

Quoted 55 lines · Stanislav Kinsburskiy
Quoted 41 lines
---
 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)
fs/autofs4/autofs_i.h +3 -0
index 4737615..3da105f 100644
@@ -120,6 +120,9 @@ struct autofs_sb_info {
 	struct list_head active_list;
 	struct list_head expiring_list;
 	struct rcu_head rcu;
+#ifdef CONFIG_COMPAT
+	unsigned is32bit:1;
+#endif
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
fs/autofs4/dev-ioctl.c +3 -0
index b7c816f..467d6c4 100644
@@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
+#ifdef CONFIG_COMPAT
+		sbi->is32bit = is_compat_task();
+#endif
 	}
 out:
 	put_pid(new_pid);
fs/autofs4/inode.c +0 -0
index 09e7d68..21d3c0b 100644

fs/autofs4/inode.c int autofs4_fill_super(struct super_block *s, void *data, int silent) · line 309

Quoted 10 lines
fs/autofs4/inode.c +3 -1
@@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	} else {
 		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
 	}
-
+#ifdef CONFIG_COMPAT
+	sbi->is32bit = is_compat_task();
+#endif
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 

Not sure about this.

Quoted 4 lines

Don't you think it would be better to avoid the in code #ifdefs by doing some checks and defines in the header file and defining what's need to just use is_compat_task().

Yes, might be...

Quoted 2 lines

Not sure 2 patches are needed for this either ......

Well, I found this issue occasionally.
And, frankly speaking, it's not clear to me, whether this issue is important at all, so I wanted to clarify this first. Thanks to O_DIRECT, the only way to catch the issue is to try to read more, than expected, in compat task (that's how I found it). I don't see any other flaw so far. And if so, that, probably, we shouldn't care about the issue at all.
What do you think?

Quoted 2 lines

Ian

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

maintainer
#4 lore raw

Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

Quoted 63 lines · Stanislav Kinsburskiy

14.09.2017 02:38, Ian Kent пишет:

Quoted 55 lines · Stanislav Kinsburskiy
Quoted 41 lines
---
 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)
fs/autofs4/autofs_i.h +3 -0
index 4737615..3da105f 100644
@@ -120,6 +120,9 @@ struct autofs_sb_info {
 	struct list_head active_list;
 	struct list_head expiring_list;
 	struct rcu_head rcu;
+#ifdef CONFIG_COMPAT
+	unsigned is32bit:1;
+#endif
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
fs/autofs4/dev-ioctl.c +3 -0
index b7c816f..467d6c4 100644
@@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
+#ifdef CONFIG_COMPAT
+		sbi->is32bit = is_compat_task();
+#endif
 	}
 out:
 	put_pid(new_pid);
fs/autofs4/inode.c +0 -0
index 09e7d68..21d3c0b 100644

fs/autofs4/inode.c int autofs4_fill_super(struct super_block *s, void *data, int silent) · line 309

Quoted 10 lines
fs/autofs4/inode.c +3 -1
@@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	} else {
 		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
 	}
-
+#ifdef CONFIG_COMPAT
+	sbi->is32bit = is_compat_task();
+#endif
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 

Not sure about this.

Quoted 4 lines

Don't you think it would be better to avoid the in code #ifdefs by doing some checks and defines in the header file and defining what's need to just use is_compat_task().

Quoted 6 lines

Yes, might be...

Quoted 2 lines

Not sure 2 patches are needed for this either ......

Well, I found this issue occasionally.

I'm wondering what the symptoms are?

Quoted 2 lines

And, frankly speaking, it's not clear to me, whether this issue is important at all, so I wanted to clarify this first. Thanks to O_DIRECT, the only way to catch the issue is to try to read more, than expected, in compat task (that's how I found it).

Right, the O_DIRECT patch from Linus was expected to fix the structure alignment problem. The stuct field offsets are ok aren't they?

Quoted 2 lines

I don't see any other flaw so far. And if so, that, probably, we shouldn't care about the issue at all. What do you think?

If we are seeing hangs, incorrect struct fields or similar something should be done about it but if all is actually working ok then the O_DIRECT fix is doing it's job and further changes aren't necessary.

Ian
--
To unsubscribe from this list: send the line "unsubscribe autofs" in

#5 lore raw

Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

14.09.2017 13:29, Ian Kent пишет:

Quoted 64 lines · Stanislav Kinsburskiy
Quoted 63 lines

14.09.2017 02:38, Ian Kent пишет:

Quoted 59 lines · Stanislav Kinsburskiy
Quoted 41 lines
---
 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)
fs/autofs4/autofs_i.h +3 -0
index 4737615..3da105f 100644
@@ -120,6 +120,9 @@ struct autofs_sb_info {
 	struct list_head active_list;
 	struct list_head expiring_list;
 	struct rcu_head rcu;
+#ifdef CONFIG_COMPAT
+	unsigned is32bit:1;
+#endif
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
fs/autofs4/dev-ioctl.c +3 -0
index b7c816f..467d6c4 100644
@@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
+#ifdef CONFIG_COMPAT
+		sbi->is32bit = is_compat_task();
+#endif
 	}
 out:
 	put_pid(new_pid);
fs/autofs4/inode.c +0 -0
index 09e7d68..21d3c0b 100644

fs/autofs4/inode.c int autofs4_fill_super(struct super_block *s, void *data, int silent) · line 309

Quoted 10 lines
fs/autofs4/inode.c +3 -1
@@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	} else {
 		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
 	}
-
+#ifdef CONFIG_COMPAT
+	sbi->is32bit = is_compat_task();
+#endif
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 

Not sure about this.

Don't you think it would be better to avoid the in code #ifdefs by doing some checks and defines in the header file and defining what's need to just use is_compat_task().

Quoted 9 lines
Quoted 6 lines

Yes, might be...

Quoted 2 lines

Not sure 2 patches are needed for this either ......

Well, I found this issue occasionally.

I'm wondering what the symptoms are?

Size of struct autofs_v5_packet is 300 bytes for x86 and 304 bytes for x86_64. Which means, that 32bit task can read more than size of autofs_v5_packet on 64bit kernel.

Quoted 6 lines
Quoted 2 lines

And, frankly speaking, it's not clear to me, whether this issue is important at all, so I wanted to clarify this first. Thanks to O_DIRECT, the only way to catch the issue is to try to read more, than expected, in compat task (that's how I found it).

Right, the O_DIRECT patch from Linus was expected to fix the structure alignment problem. The stuct field offsets are ok aren't they?

Yes, they are ok.

Quoted 7 lines
Quoted 2 lines

I don't see any other flaw so far. And if so, that, probably, we shouldn't care about the issue at all. What do you think?

If we are seeing hangs, incorrect struct fields or similar something should be done about it but if all is actually working ok then the O_DIRECT fix is doing it's job and further changes aren't necessary.

Well, yes. O_DIRECT fix covers the issue.
Ok then.
Thanks for the clarification!

Quoted 2 lines

Ian

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

maintainer
#6 lore raw

Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

Quoted 72 lines · Stanislav Kinsburskiy

14.09.2017 13:29, Ian Kent пишет:

Quoted 69 lines · Stanislav Kinsburskiy
Quoted 68 lines

14.09.2017 02:38, Ian Kent пишет:

Quoted 55 lines · Stanislav Kinsburskiy
Quoted 41 lines
---
 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)
fs/autofs4/autofs_i.h +3 -0
index 4737615..3da105f 100644
@@ -120,6 +120,9 @@ struct autofs_sb_info {
 	struct list_head active_list;
 	struct list_head expiring_list;
 	struct rcu_head rcu;
+#ifdef CONFIG_COMPAT
+	unsigned is32bit:1;
+#endif
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
fs/autofs4/dev-ioctl.c +3 -0
index b7c816f..467d6c4 100644
@@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
+#ifdef CONFIG_COMPAT
+		sbi->is32bit = is_compat_task();
+#endif
 	}
 out:
 	put_pid(new_pid);
fs/autofs4/inode.c +0 -0
index 09e7d68..21d3c0b 100644

fs/autofs4/inode.c int autofs4_fill_super(struct super_block *s, void *data, int silent) · line 309

Quoted 10 lines
fs/autofs4/inode.c +3 -1
@@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	} else {
 		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
 	}
-
+#ifdef CONFIG_COMPAT
+	sbi->is32bit = is_compat_task();
+#endif
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 

Not sure about this.

Quoted 4 lines

Don't you think it would be better to avoid the in code #ifdefs by doing some checks and defines in the header file and defining what's need to just use is_compat_task().

Yes, might be...

Quoted 2 lines

Not sure 2 patches are needed for this either ......

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Well, I found this issue occasionally.

I'm wondering what the symptoms are?

Size of struct autofs_v5_packet is 300 bytes for x86 and 304 bytes for x86_64. Which means, that 32bit task can read more than size of autofs_v5_packet on 64bit kernel.

Are you sure?

Shouldn't that be a short read on the x86 side of a 4 bytes longer structure on the x86_64 side.

I didn't think you could have a 64 bit client on a 32 bit kernel so the converse (the read past end of struct) doesn't apply.

Ian

#7 lore raw

Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation

14.09.2017 13:45, Ian Kent пишет:

Quoted 81 lines · Stanislav Kinsburskiy
Quoted 72 lines

14.09.2017 13:29, Ian Kent пишет:

Quoted 69 lines · Stanislav Kinsburskiy
Quoted 68 lines

14.09.2017 02:38, Ian Kent пишет:

Quoted 55 lines · Stanislav Kinsburskiy
Quoted 41 lines
---
 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 3 files changed, 9 insertions(+), 1 deletion(-)
fs/autofs4/autofs_i.h +3 -0
index 4737615..3da105f 100644
@@ -120,6 +120,9 @@ struct autofs_sb_info {
 	struct list_head active_list;
 	struct list_head expiring_list;
 	struct rcu_head rcu;
+#ifdef CONFIG_COMPAT
+	unsigned is32bit:1;
+#endif
 };
 
 static inline struct autofs_sb_info *autofs4_sbi(struct super_block *sb)
fs/autofs4/dev-ioctl.c +3 -0
index b7c816f..467d6c4 100644
@@ -397,6 +397,9 @@ static int autofs_dev_ioctl_setpipefd(struct file *fp,
 		sbi->pipefd = pipefd;
 		sbi->pipe = pipe;
 		sbi->catatonic = 0;
+#ifdef CONFIG_COMPAT
+		sbi->is32bit = is_compat_task();
+#endif
 	}
 out:
 	put_pid(new_pid);
fs/autofs4/inode.c +0 -0
index 09e7d68..21d3c0b 100644

fs/autofs4/inode.c int autofs4_fill_super(struct super_block *s, void *data, int silent) · line 309

Quoted 10 lines
fs/autofs4/inode.c +3 -1
@@ -301,7 +301,9 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent)
 	} else {
 		sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
 	}
-
+#ifdef CONFIG_COMPAT
+	sbi->is32bit = is_compat_task();
+#endif
 	if (autofs_type_trigger(sbi->type))
 		__managed_dentry_set_managed(root);
 

Not sure about this.

Quoted 4 lines

Don't you think it would be better to avoid the in code #ifdefs by doing some checks and defines in the header file and defining what's need to just use is_compat_task().

Yes, might be...

Quoted 2 lines

Not sure 2 patches are needed for this either ......

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Well, I found this issue occasionally.

I'm wondering what the symptoms are?

Size of struct autofs_v5_packet is 300 bytes for x86 and 304 bytes for x86_64. Which means, that 32bit task can read more than size of autofs_v5_packet on 64bit kernel.

Quoted 8 lines

Are you sure?

Shouldn't that be a short read on the x86 side of a 4 bytes longer structure on the x86_64 side.

I didn't think you could have a 64 bit client on a 32 bit kernel so the converse (the read past end of struct) doesn't apply.

Sorry for the confusion, I had to add brackets like this:

Quoted 1 line

Which means, that 32bit task can read more than size of autofs_v5_packet (on 64bit kernel).

IOW, 32bit task expects to read 300 bytes (size of struct autofs_v5_packet) while there are 304 bytes available on the "wire" from the 64bit kernel.

Quoted 2 lines

Ian

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

#8 lore raw

RFC PATCH 2/2 autofs: sent 32-bit sized packet for 32-bit process

In reply to Stanislav Kinsburskiy

The structure autofs_v5_packet (except name) is not aligned by 8 bytes, which leads to different sizes in 32 and 64-bit architectures.
Let's form 32-bit compatible packet when daemon has 32-bit addressation.

---
 fs/autofs4/waitq.c |    5 +++++
 1 file changed, 5 insertions(+)
fs/autofs4/waitq.c +5 -0
index 24a58bf..1f9b7d8 100644
@@ -151,6 +151,11 @@ static void autofs4_notify_daemon(struct autofs_sb_info *sbi,
 		struct autofs_v5_packet *packet = &pkt.v5_pkt.v5_packet;
 		struct user_namespace *user_ns = sbi->pipe->f_cred->user_ns;
 
+#ifdef CONFIG_COMPAT
+		if (sbi->is32bit)
+			pktsz = offsetofend(struct autofs_v5_packet, name);
+		else
+#endif
 		pktsz = sizeof(*packet);
 
 		packet->wait_queue_token = wq->wait_queue_token;
maintainer
#9 lore raw
In reply to Stanislav Kinsburskiy
Quoted 2 lines · Stanislav Kinsburskiy

The problem is that in compat mode struct autofs_v5_packet has to have different size (i.e. 4 bytes less).

I regret (several times over) my original decision to not make v5 packets packed ....

I have to say the description of the problem is not all that good.

Quoted 6 lines

This is RFC because:
1) This issue is hidden, because autofs pipe has O_DIRECT and the rest of the epacket is truncated when read.
2) X86 arch doesn't have is_compat_task() helper
3) It's now clear, what to do if "pgrp" option is specified.

I don't understand what the "pgrp" option has to do with this?

Quoted 10 lines

The following series implements...

---

Stanislav Kinsburskiy (2):
autofs: set compat flag on sbi when daemon uses 32bit addressation
autofs: sent 32-bit sized packet for 32-bit process

Quoted 6 lines
 fs/autofs4/autofs_i.h  |    3 +++
 fs/autofs4/dev-ioctl.c |    3 +++
 fs/autofs4/inode.c     |    4 +++-
 fs/autofs4/waitq.c     |    5 +++++
 4 files changed, 14 insertions(+), 1 deletion(-)

--
To unsubscribe from this list: send the line "unsubscribe autofs" in