autofs: fix autofs_v5_packet dlivery in compat mode
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
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
@@ -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
@@ -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
@@ -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
Ian Kent
maintainerRe: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
Quoted 41 lines · Stanislav Kinsburskiy
Signed-off-by Stanislav Kinsburskiy <[email protected]>--- 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@@ -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@@ -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
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
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
Signed-off-by Stanislav Kinsburskiy <[email protected]>--- 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@@ -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@@ -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
-0fs/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
Ian Kent
maintainerRe: [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
Signed-off-by Stanislav Kinsburskiy <[email protected]>--- 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@@ -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@@ -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
-0fs/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
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
Signed-off-by Stanislav Kinsburskiy <[email protected]>--- 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@@ -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@@ -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
-0fs/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_DIRECTpatch 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_DIRECTfix 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
Ian Kent
maintainerRe: [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
Signed-off-by Stanislav Kinsburskiy <[email protected]>--- 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@@ -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@@ -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
-0fs/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_packetis 300 bytes for x86 and 304 bytes forx86_64. Which means, that 32bit task can read more than size ofautofs_v5_packeton 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
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
Signed-off-by Stanislav Kinsburskiy <[email protected]>--- 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@@ -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@@ -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
-0fs/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_packetis 300 bytes for x86 and 304 bytes forx86_64. Which means, that 32bit task can read more than size ofautofs_v5_packeton 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_64side.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
RFC PATCH 2/2 autofs: sent 32-bit sized packet for 32-bit process
In reply to Stanislav KinsburskiyThe 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
@@ -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;
Ian Kent
maintainerQuoted 2 lines · Stanislav Kinsburskiy
The problem is that in compat mode struct
autofs_v5_packethas 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 hasO_DIRECTand the rest of the epacket is truncated when read.
2) X86 arch doesn't haveis_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