| Message ID | 20210422154427.13038-1-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/s390x: fix s390_probe_access to check PAGE_WRITE_ORG for writeability | expand |
On 4/22/21 8:44 AM, Alex Bennée wrote: > We can remove PAGE_WRITE when (internally) marking a page read-only > because it contains translated code. This can get confused when we are > executing signal return code on signal stacks. > > Fixes: e56552cf07 ("target/s390x: Implement the MVPG condition-code-option bit") > Found-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Cornelia Huck <cohuck@redhat.com> > Cc: Thomas Huth <thuth@redhat.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Laurent Vivier <laurent@vivier.eu> > --- > target/s390x/mem_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 22.04.21 17:44, Alex Bennée wrote: > We can remove PAGE_WRITE when (internally) marking a page read-only > because it contains translated code. This can get confused when we are > executing signal return code on signal stacks. > > Fixes: e56552cf07 ("target/s390x: Implement the MVPG condition-code-option bit") > Found-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Cornelia Huck <cohuck@redhat.com> > Cc: Thomas Huth <thuth@redhat.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Laurent Vivier <laurent@vivier.eu> > --- > target/s390x/mem_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 12e84a4285..f6a7d29273 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -145,7 +145,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, > > #if defined(CONFIG_USER_ONLY) > flags = page_get_flags(addr); > - if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE))) { > + if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE_ORG))) { > env->__excp_addr = addr; > flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING; > if (nonfault) { > Reviewed-by: David Hildenbrand <david@redhat.com>
On Thu, 22 Apr 2021 16:44:27 +0100 Alex Bennée <alex.bennee@linaro.org> wrote: > We can remove PAGE_WRITE when (internally) marking a page read-only > because it contains translated code. This can get confused when we are > executing signal return code on signal stacks. > > Fixes: e56552cf07 ("target/s390x: Implement the MVPG condition-code-option bit") > Found-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Cornelia Huck <cohuck@redhat.com> > Cc: Thomas Huth <thuth@redhat.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Laurent Vivier <laurent@vivier.eu> > --- > target/s390x/mem_helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 12e84a4285..f6a7d29273 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -145,7 +145,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, > > #if defined(CONFIG_USER_ONLY) > flags = page_get_flags(addr); > - if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE))) { > + if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE_ORG))) { > env->__excp_addr = addr; > flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING; > if (nonfault) { What's the verdict on this one? I plan to queue this to s390-next; but if we end up doing an -rc5, it might qualify as a regression fix. If this is going to be in 6.1, I'll add cc:stable when queuing.
On Fri, 23 Apr 2021 at 13:22, Cornelia Huck <cohuck@redhat.com> wrote: > > On Thu, 22 Apr 2021 16:44:27 +0100 > Alex Bennée <alex.bennee@linaro.org> wrote: > > > We can remove PAGE_WRITE when (internally) marking a page read-only > > because it contains translated code. This can get confused when we are > > executing signal return code on signal stacks. > > > > Fixes: e56552cf07 ("target/s390x: Implement the MVPG condition-code-option bit") > > Found-by: Richard Henderson <richard.henderson@linaro.org> > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > Cc: Cornelia Huck <cohuck@redhat.com> > > Cc: Thomas Huth <thuth@redhat.com> > > Cc: David Hildenbrand <david@redhat.com> > > Cc: Laurent Vivier <laurent@vivier.eu> > > --- > > target/s390x/mem_helper.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > > index 12e84a4285..f6a7d29273 100644 > > --- a/target/s390x/mem_helper.c > > +++ b/target/s390x/mem_helper.c > > @@ -145,7 +145,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, > > > > #if defined(CONFIG_USER_ONLY) > > flags = page_get_flags(addr); > > - if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE))) { > > + if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE_ORG))) { > > env->__excp_addr = addr; > > flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING; > > if (nonfault) { > > What's the verdict on this one? I plan to queue this to s390-next; but > if we end up doing an -rc5, it might qualify as a regression fix. What's your opinion? I think we do need an rc5 for the network backend hotplug crash. I don't want to open the doors for lots of new fixes just because we've got another rc, but on the other hand this one does look like it's a pretty small and safe fix, and letting intermittent crash bugs out into the wild seems like it could lead to a lot of annoying re-investigation of the same bug if it's reported by users later... So I kind of lean towards putting it in rc5. thanks -- PMM
On 23/04/2021 15.06, Peter Maydell wrote: > On Fri, 23 Apr 2021 at 13:22, Cornelia Huck <cohuck@redhat.com> wrote: >> >> On Thu, 22 Apr 2021 16:44:27 +0100 >> Alex Bennée <alex.bennee@linaro.org> wrote: >> >>> We can remove PAGE_WRITE when (internally) marking a page read-only >>> because it contains translated code. This can get confused when we are >>> executing signal return code on signal stacks. >>> >>> Fixes: e56552cf07 ("target/s390x: Implement the MVPG condition-code-option bit") >>> Found-by: Richard Henderson <richard.henderson@linaro.org> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> Cc: Cornelia Huck <cohuck@redhat.com> >>> Cc: Thomas Huth <thuth@redhat.com> >>> Cc: David Hildenbrand <david@redhat.com> >>> Cc: Laurent Vivier <laurent@vivier.eu> >>> --- >>> target/s390x/mem_helper.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c >>> index 12e84a4285..f6a7d29273 100644 >>> --- a/target/s390x/mem_helper.c >>> +++ b/target/s390x/mem_helper.c >>> @@ -145,7 +145,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, >>> >>> #if defined(CONFIG_USER_ONLY) >>> flags = page_get_flags(addr); >>> - if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE))) { >>> + if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE_ORG))) { >>> env->__excp_addr = addr; >>> flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING; >>> if (nonfault) { >> >> What's the verdict on this one? I plan to queue this to s390-next; but >> if we end up doing an -rc5, it might qualify as a regression fix. > > What's your opinion? I think we do need an rc5 for the network backend > hotplug crash. I don't want to open the doors for lots of new fixes > just because we've got another rc, but on the other hand this one > does look like it's a pretty small and safe fix, and letting intermittent > crash bugs out into the wild seems like it could lead to a lot of > annoying re-investigation of the same bug if it's reported by users > later... So I kind of lean towards putting it in rc5. IMHO: It's in a s390x-only file, within a #ifdef CONFIG_USER_ONLY ... so the damage this could do is very, very limited, indeed. Thus I'd also suggest to include it in a rc5. Thomas
On Fri, 23 Apr 2021 15:28:19 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 23/04/2021 15.06, Peter Maydell wrote: > > On Fri, 23 Apr 2021 at 13:22, Cornelia Huck <cohuck@redhat.com> wrote: > >> > >> On Thu, 22 Apr 2021 16:44:27 +0100 > >> Alex Bennée <alex.bennee@linaro.org> wrote: > >> > >>> We can remove PAGE_WRITE when (internally) marking a page read-only > >>> because it contains translated code. This can get confused when we are > >>> executing signal return code on signal stacks. > >>> > >>> Fixes: e56552cf07 ("target/s390x: Implement the MVPG condition-code-option bit") > >>> Found-by: Richard Henderson <richard.henderson@linaro.org> > >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >>> Cc: Cornelia Huck <cohuck@redhat.com> > >>> Cc: Thomas Huth <thuth@redhat.com> > >>> Cc: David Hildenbrand <david@redhat.com> > >>> Cc: Laurent Vivier <laurent@vivier.eu> > >>> --- > >>> target/s390x/mem_helper.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > >>> index 12e84a4285..f6a7d29273 100644 > >>> --- a/target/s390x/mem_helper.c > >>> +++ b/target/s390x/mem_helper.c > >>> @@ -145,7 +145,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, > >>> > >>> #if defined(CONFIG_USER_ONLY) > >>> flags = page_get_flags(addr); > >>> - if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE))) { > >>> + if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE_ORG))) { > >>> env->__excp_addr = addr; > >>> flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING; > >>> if (nonfault) { > >> > >> What's the verdict on this one? I plan to queue this to s390-next; but > >> if we end up doing an -rc5, it might qualify as a regression fix. > > > > What's your opinion? I think we do need an rc5 for the network backend > > hotplug crash. I don't want to open the doors for lots of new fixes > > just because we've got another rc, but on the other hand this one > > does look like it's a pretty small and safe fix, and letting intermittent > > crash bugs out into the wild seems like it could lead to a lot of > > annoying re-investigation of the same bug if it's reported by users > > later... So I kind of lean towards putting it in rc5. > > IMHO: It's in a s390x-only file, within a #ifdef CONFIG_USER_ONLY ... so the > damage this could do is very, very limited, indeed. Thus I'd also suggest to > include it in a rc5. Exactly, the benefits outweigh the risk IMHO. Peter, do you want to pick this one directly, or should I send you a pull req?
On Fri, 23 Apr 2021 at 14:52, Cornelia Huck <cohuck@redhat.com> wrote: > > On Fri, 23 Apr 2021 15:28:19 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > On 23/04/2021 15.06, Peter Maydell wrote: > > > On Fri, 23 Apr 2021 at 13:22, Cornelia Huck <cohuck@redhat.com> wrote: > > >> What's the verdict on this one? I plan to queue this to s390-next; but > > >> if we end up doing an -rc5, it might qualify as a regression fix. > > > > > > What's your opinion? I think we do need an rc5 for the network backend > > > hotplug crash. I don't want to open the doors for lots of new fixes > > > just because we've got another rc, but on the other hand this one > > > does look like it's a pretty small and safe fix, and letting intermittent > > > crash bugs out into the wild seems like it could lead to a lot of > > > annoying re-investigation of the same bug if it's reported by users > > > later... So I kind of lean towards putting it in rc5. > > > > IMHO: It's in a s390x-only file, within a #ifdef CONFIG_USER_ONLY ... so the > > damage this could do is very, very limited, indeed. Thus I'd also suggest to > > include it in a rc5. > > Exactly, the benefits outweigh the risk IMHO. > > Peter, do you want to pick this one directly, or should I send you a pull req? I'll pick it directly, thanks. -- PMM
On Fri, 23 Apr 2021 at 14:56, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 23 Apr 2021 at 14:52, Cornelia Huck <cohuck@redhat.com> wrote: > > > > On Fri, 23 Apr 2021 15:28:19 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > > > On 23/04/2021 15.06, Peter Maydell wrote: > > > > On Fri, 23 Apr 2021 at 13:22, Cornelia Huck <cohuck@redhat.com> wrote: > > > >> What's the verdict on this one? I plan to queue this to s390-next; but > > > >> if we end up doing an -rc5, it might qualify as a regression fix. > > > > > > > > What's your opinion? I think we do need an rc5 for the network backend > > > > hotplug crash. I don't want to open the doors for lots of new fixes > > > > just because we've got another rc, but on the other hand this one > > > > does look like it's a pretty small and safe fix, and letting intermittent > > > > crash bugs out into the wild seems like it could lead to a lot of > > > > annoying re-investigation of the same bug if it's reported by users > > > > later... So I kind of lean towards putting it in rc5. > > > > > > IMHO: It's in a s390x-only file, within a #ifdef CONFIG_USER_ONLY ... so the > > > damage this could do is very, very limited, indeed. Thus I'd also suggest to > > > include it in a rc5. > > > > Exactly, the benefits outweigh the risk IMHO. > > > > Peter, do you want to pick this one directly, or should I send you a pull req? > > I'll pick it directly, thanks. ...applied to target-arm.next, thanks. -- PMM
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 12e84a4285..f6a7d29273 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -145,7 +145,7 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size, #if defined(CONFIG_USER_ONLY) flags = page_get_flags(addr); - if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE))) { + if (!(flags & (access_type == MMU_DATA_LOAD ? PAGE_READ : PAGE_WRITE_ORG))) { env->__excp_addr = addr; flags = (flags & PAGE_VALID) ? PGM_PROTECTION : PGM_ADDRESSING; if (nonfault) {
We can remove PAGE_WRITE when (internally) marking a page read-only because it contains translated code. This can get confused when we are executing signal return code on signal stacks. Fixes: e56552cf07 ("target/s390x: Implement the MVPG condition-code-option bit") Found-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Cornelia Huck <cohuck@redhat.com> Cc: Thomas Huth <thuth@redhat.com> Cc: David Hildenbrand <david@redhat.com> Cc: Laurent Vivier <laurent@vivier.eu> --- target/s390x/mem_helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)