Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
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 >> --- >> 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 > ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
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 > --- > 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 ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
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 --- 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 > ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
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 >>> --- >>> 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 ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
14.09.2017 02:38, Ian Kent пишет: > On 01/09/17 19:21, Stanislav Kinsburskiy wrote: >> Signed-off-by: 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(-) >> >> 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 > ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
Re: [Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
On 01/09/17 19:21, Stanislav Kinsburskiy wrote: > Signed-off-by: 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(-) > > 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 ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
[Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
Signed-off-by: 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(-) 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); ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
[Devel] [RFC PATCH 1/2] autofs: set compat flag on sbi when daemon uses 32bit addressation
Signed-off-by: Stanislav Kinsburskiy --- fs/autofs4/inode.c | 16 1 file changed, 16 insertions(+) diff --git a/fs/autofs4/inode.c b/fs/autofs4/inode.c index b23cf2a..989ac38 100644 --- a/fs/autofs4/inode.c +++ b/fs/autofs4/inode.c @@ -217,6 +217,7 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent) int pgrp; bool pgrp_set = false; int ret = -EINVAL; + struct task_struct *tsk; sbi = kzalloc(sizeof(*sbi), GFP_KERNEL); if (!sbi) @@ -281,10 +282,25 @@ int autofs4_fill_super(struct super_block *s, void *data, int silent) pgrp); goto fail_dput; } + tsk = get_pid_task(sbi->oz_pgrp, PIDTYPE_PGID); + if (!tsk) { + pr_warn("autofs: could not find process group leader %d\n", + pgrp); + goto fail_put_pid; + } } else { sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID); + get_task_struct(current); + tsk = current; } + if (test_tsk_thread_flag(tsk, TIF_ADDR32)) + sbi->is32bit = 1; + else + sbi->is32bit = 0; + + put_task_struct(tsk); + if (autofs_type_trigger(sbi->type)) __managed_dentry_set_managed(root); ___ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel