* [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode
@ 2017-09-01 11:21 Stanislav Kinsburskiy
2017-09-01 11:21 ` [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation Stanislav Kinsburskiy
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Stanislav Kinsburskiy @ 2017-09-01 11:21 UTC (permalink / raw)
To: raven; +Cc: autofs, linux-kernel, devel, ldv
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation 2017-09-01 11:21 [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode Stanislav Kinsburskiy @ 2017-09-01 11:21 ` Stanislav Kinsburskiy 2017-09-14 0:38 ` Ian Kent 2017-09-01 11:21 ` [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process Stanislav Kinsburskiy 2017-09-14 0:32 ` [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode Ian Kent 2 siblings, 1 reply; 10+ messages in thread From: Stanislav Kinsburskiy @ 2017-09-01 11:21 UTC (permalink / raw) To: raven; +Cc: autofs, linux-kernel, devel, ldv Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> --- fs/autofs4/autofs_i.h | 3 +++ fs/autofs4/dev-ioctl.c | 3 +++ fs/autofs4/inode.c | 4 +++- 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h index 4737615..3da105f 100644 --- a/fs/autofs4/autofs_i.h +++ b/fs/autofs4/autofs_i.h @@ -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) diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c index b7c816f..467d6c4 100644 --- a/fs/autofs4/dev-ioctl.c +++ b/fs/autofs4/dev-ioctl.c @@ -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); diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c index 09e7d68..21d3c0b 100644 --- a/fs/autofs4/inode.c +++ b/fs/autofs4/inode.c @@ -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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation 2017-09-01 11:21 ` [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation Stanislav Kinsburskiy @ 2017-09-14 0:38 ` Ian Kent 2017-09-14 9:24 ` Stanislav Kinsburskiy 0 siblings, 1 reply; 10+ messages in thread From: Ian Kent @ 2017-09-14 0:38 UTC (permalink / raw) To: Stanislav Kinsburskiy; +Cc: autofs, linux-kernel, devel, ldv On 01/09/17 19:21, Stanislav Kinsburskiy wrote: > Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> > --- > fs/autofs4/autofs_i.h | 3 +++ > fs/autofs4/dev-ioctl.c | 3 +++ > fs/autofs4/inode.c | 4 +++- > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h > index 4737615..3da105f 100644 > --- a/fs/autofs4/autofs_i.h > +++ b/fs/autofs4/autofs_i.h > @@ -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) > diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c > index b7c816f..467d6c4 100644 > --- a/fs/autofs4/dev-ioctl.c > +++ b/fs/autofs4/dev-ioctl.c > @@ -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); > diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c > index 09e7d68..21d3c0b 100644 > --- a/fs/autofs4/inode.c > +++ b/fs/autofs4/inode.c > @@ -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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation 2017-09-14 0:38 ` Ian Kent @ 2017-09-14 9:24 ` Stanislav Kinsburskiy 2017-09-14 11:29 ` Ian Kent 0 siblings, 1 reply; 10+ messages in thread From: Stanislav Kinsburskiy @ 2017-09-14 9:24 UTC (permalink / raw) To: Ian Kent; +Cc: autofs, linux-kernel, devel, ldv 14.09.2017 02:38, Ian Kent пишет: > On 01/09/17 19:21, Stanislav Kinsburskiy wrote: >> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> >> --- >> fs/autofs4/autofs_i.h | 3 +++ >> fs/autofs4/dev-ioctl.c | 3 +++ >> fs/autofs4/inode.c | 4 +++- >> 3 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h >> index 4737615..3da105f 100644 >> --- a/fs/autofs4/autofs_i.h >> +++ b/fs/autofs4/autofs_i.h >> @@ -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) >> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c >> index b7c816f..467d6c4 100644 >> --- a/fs/autofs4/dev-ioctl.c >> +++ b/fs/autofs4/dev-ioctl.c >> @@ -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); >> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c >> index 09e7d68..21d3c0b 100644 >> --- a/fs/autofs4/inode.c >> +++ b/fs/autofs4/inode.c >> @@ -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(). > Yes, might be... > 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? > Ian > -- To unsubscribe from this list: send the line "unsubscribe autofs" in ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation 2017-09-14 9:24 ` Stanislav Kinsburskiy @ 2017-09-14 11:29 ` Ian Kent 2017-09-14 11:39 ` Stanislav Kinsburskiy 0 siblings, 1 reply; 10+ messages in thread From: Ian Kent @ 2017-09-14 11:29 UTC (permalink / raw) To: skinsbursky; +Cc: autofs, linux-kernel, devel, ldv On 14/09/17 17:24, Stanislav Kinsburskiy wrote: > > > 14.09.2017 02:38, Ian Kent пишет: >> On 01/09/17 19:21, Stanislav Kinsburskiy wrote: >>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> >>> --- >>> fs/autofs4/autofs_i.h | 3 +++ >>> fs/autofs4/dev-ioctl.c | 3 +++ >>> fs/autofs4/inode.c | 4 +++- >>> 3 files changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h >>> index 4737615..3da105f 100644 >>> --- a/fs/autofs4/autofs_i.h >>> +++ b/fs/autofs4/autofs_i.h >>> @@ -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) >>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c >>> index b7c816f..467d6c4 100644 >>> --- a/fs/autofs4/dev-ioctl.c >>> +++ b/fs/autofs4/dev-ioctl.c >>> @@ -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); >>> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c >>> index 09e7d68..21d3c0b 100644 >>> --- a/fs/autofs4/inode.c >>> +++ b/fs/autofs4/inode.c >>> @@ -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(). >> > > Yes, might be... > >> Not sure 2 patches are needed for this either ...... >> > > Well, I found this issue occasionally. I'm wondering what the symptoms are? > 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? > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation 2017-09-14 11:29 ` Ian Kent @ 2017-09-14 11:39 ` Stanislav Kinsburskiy 2017-09-14 11:45 ` Ian Kent 0 siblings, 1 reply; 10+ messages in thread From: Stanislav Kinsburskiy @ 2017-09-14 11:39 UTC (permalink / raw) To: Ian Kent; +Cc: autofs, linux-kernel, devel, ldv 14.09.2017 13:29, Ian Kent пишет: > On 14/09/17 17:24, Stanislav Kinsburskiy wrote: >> >> >> 14.09.2017 02:38, Ian Kent пишет: >>> On 01/09/17 19:21, Stanislav Kinsburskiy wrote: >>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> >>>> --- >>>> fs/autofs4/autofs_i.h | 3 +++ >>>> fs/autofs4/dev-ioctl.c | 3 +++ >>>> fs/autofs4/inode.c | 4 +++- >>>> 3 files changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h >>>> index 4737615..3da105f 100644 >>>> --- a/fs/autofs4/autofs_i.h >>>> +++ b/fs/autofs4/autofs_i.h >>>> @@ -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) >>>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c >>>> index b7c816f..467d6c4 100644 >>>> --- a/fs/autofs4/dev-ioctl.c >>>> +++ b/fs/autofs4/dev-ioctl.c >>>> @@ -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); >>>> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c >>>> index 09e7d68..21d3c0b 100644 >>>> --- a/fs/autofs4/inode.c >>>> +++ b/fs/autofs4/inode.c >>>> @@ -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(). >>> >> >> Yes, might be... >> >>> 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. >> 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. >> 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! > Ian > -- To unsubscribe from this list: send the line "unsubscribe autofs" in ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation 2017-09-14 11:39 ` Stanislav Kinsburskiy @ 2017-09-14 11:45 ` Ian Kent 2017-09-14 11:51 ` Stanislav Kinsburskiy 0 siblings, 1 reply; 10+ messages in thread From: Ian Kent @ 2017-09-14 11:45 UTC (permalink / raw) To: skinsbursky; +Cc: autofs, linux-kernel, devel, ldv On 14/09/17 19:39, Stanislav Kinsburskiy wrote: > > > 14.09.2017 13:29, Ian Kent пишет: >> On 14/09/17 17:24, Stanislav Kinsburskiy wrote: >>> >>> >>> 14.09.2017 02:38, Ian Kent пишет: >>>> On 01/09/17 19:21, Stanislav Kinsburskiy wrote: >>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> >>>>> --- >>>>> fs/autofs4/autofs_i.h | 3 +++ >>>>> fs/autofs4/dev-ioctl.c | 3 +++ >>>>> fs/autofs4/inode.c | 4 +++- >>>>> 3 files changed, 9 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h >>>>> index 4737615..3da105f 100644 >>>>> --- a/fs/autofs4/autofs_i.h >>>>> +++ b/fs/autofs4/autofs_i.h >>>>> @@ -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) >>>>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c >>>>> index b7c816f..467d6c4 100644 >>>>> --- a/fs/autofs4/dev-ioctl.c >>>>> +++ b/fs/autofs4/dev-ioctl.c >>>>> @@ -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); >>>>> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c >>>>> index 09e7d68..21d3c0b 100644 >>>>> --- a/fs/autofs4/inode.c >>>>> +++ b/fs/autofs4/inode.c >>>>> @@ -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(). >>>> >>> >>> Yes, might be... >>> >>>> 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. 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation 2017-09-14 11:45 ` Ian Kent @ 2017-09-14 11:51 ` Stanislav Kinsburskiy 0 siblings, 0 replies; 10+ messages in thread From: Stanislav Kinsburskiy @ 2017-09-14 11:51 UTC (permalink / raw) To: Ian Kent; +Cc: autofs, linux-kernel, devel, ldv 14.09.2017 13:45, Ian Kent пишет: > On 14/09/17 19:39, Stanislav Kinsburskiy wrote: >> >> >> 14.09.2017 13:29, Ian Kent пишет: >>> On 14/09/17 17:24, Stanislav Kinsburskiy wrote: >>>> >>>> >>>> 14.09.2017 02:38, Ian Kent пишет: >>>>> On 01/09/17 19:21, Stanislav Kinsburskiy wrote: >>>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> >>>>>> --- >>>>>> fs/autofs4/autofs_i.h | 3 +++ >>>>>> fs/autofs4/dev-ioctl.c | 3 +++ >>>>>> fs/autofs4/inode.c | 4 +++- >>>>>> 3 files changed, 9 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/fs/autofs4/autofs_i.h b/fs/autofs4/autofs_i.h >>>>>> index 4737615..3da105f 100644 >>>>>> --- a/fs/autofs4/autofs_i.h >>>>>> +++ b/fs/autofs4/autofs_i.h >>>>>> @@ -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) >>>>>> diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c >>>>>> index b7c816f..467d6c4 100644 >>>>>> --- a/fs/autofs4/dev-ioctl.c >>>>>> +++ b/fs/autofs4/dev-ioctl.c >>>>>> @@ -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); >>>>>> diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c >>>>>> index 09e7d68..21d3c0b 100644 >>>>>> --- a/fs/autofs4/inode.c >>>>>> +++ b/fs/autofs4/inode.c >>>>>> @@ -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(). >>>>> >>>> >>>> Yes, might be... >>>> >>>>> 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. > > 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: > 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. > Ian > -- To unsubscribe from this list: send the line "unsubscribe autofs" in ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process 2017-09-01 11:21 [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode Stanislav Kinsburskiy 2017-09-01 11:21 ` [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation Stanislav Kinsburskiy @ 2017-09-01 11:21 ` Stanislav Kinsburskiy 2017-09-14 0:32 ` [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode Ian Kent 2 siblings, 0 replies; 10+ messages in thread From: Stanislav Kinsburskiy @ 2017-09-01 11:21 UTC (permalink / raw) To: raven; +Cc: autofs, linux-kernel, devel, ldv 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. Suggested-by: Dmitry V. Levin <ldv@altlinux.org> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com> --- fs/autofs4/waitq.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c index 24a58bf..1f9b7d8 100644 --- a/fs/autofs4/waitq.c +++ b/fs/autofs4/waitq.c @@ -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; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode 2017-09-01 11:21 [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode Stanislav Kinsburskiy 2017-09-01 11:21 ` [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation Stanislav Kinsburskiy 2017-09-01 11:21 ` [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process Stanislav Kinsburskiy @ 2017-09-14 0:32 ` Ian Kent 2 siblings, 0 replies; 10+ messages in thread From: Ian Kent @ 2017-09-14 0:32 UTC (permalink / raw) To: Stanislav Kinsburskiy; +Cc: autofs, linux-kernel, devel, ldv On 01/09/17 19:21, Stanislav Kinsburskiy wrote: > 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. > > 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? > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-09-14 11:51 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-01 11:21 [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode Stanislav Kinsburskiy 2017-09-01 11:21 ` [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation Stanislav Kinsburskiy 2017-09-14 0:38 ` Ian Kent 2017-09-14 9:24 ` Stanislav Kinsburskiy 2017-09-14 11:29 ` Ian Kent 2017-09-14 11:39 ` Stanislav Kinsburskiy 2017-09-14 11:45 ` Ian Kent 2017-09-14 11:51 ` Stanislav Kinsburskiy 2017-09-01 11:21 ` [RFC PATCH 2/2] autofs: sent 32-bit sized packet for 32-bit process Stanislav Kinsburskiy 2017-09-14 0:32 ` [RFC PATCH 0/2] autofs: fix autofs_v5_packet dlivery in compat mode Ian Kent
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).