[PATCH] Alpha: unbreak osf_setsysinfo(SSI_NVPAIRS, [SSIN_UACPROC, UAC_SIGBUS])

[PATCH] Alpha: unbreak osf_setsysinfo(SSI_NVPAIRS, [SSIN_UACPROC, UAC_SIGBUS])

[slyich]

From: Sergei Trofimovich <slyfox@gentoo.org>

Update 'UAC_SHIFT' to match 'ALPHA_UAC_SHIFT'.

I don't know why kernel maintains 2 copies of the same constant.
One (UAC_SHIFT) is used to shift bits in syscall definition, and
another is in trap handler (ALPHA_UAC_SHIFT).

Was broken by
> commit 745dd2405e281d96c0a449103bdf6a895048f28c
> Author: Michael Cree <mcree@orcon.net.nz>
> Date:   Mon Nov 30 22:44:40 2009 -0500
>
>    Alpha: Rearrange thread info flags fixing two regressions
>
>    Both regressions fixed by (1) rearranging TIF_NOTIFY_RESUME flag to be
>    in lower 8 bits of the thread info flags, and (2) making sure that
>    ALPHA_UAC_SHIFT matches the rearrangement of the thread info flags.
>
>    Signed-off-by: Michael Cree <mcree@orcon.net.nz>
>    Cc: Richard Henderson <rth@twiddle.net>
>    Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
>    Cc: David Howells <dhowells@redhat.com>,
>    Signed-off-by: Matt Turner <mattst88@gmail.com>

Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
Cc: Michael Cree <mcree@orcon.net.nz>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: David Howells <dhowells@redhat.com>,
Cc: Matt Turner <mattst88@gmail.com>
---
 arch/alpha/include/asm/sysinfo.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/alpha/include/asm/sysinfo.h b/arch/alpha/include/asm/sysinfo.h
index 086aba2..42b3edc 100644
--- a/arch/alpha/include/asm/sysinfo.h
+++ b/arch/alpha/include/asm/sysinfo.h
@@ -32,7 +32,7 @@
 
 /* This is the shift that is applied to the UAC bits as stored in the
    per-thread flags.  See thread_info.h.  */
-#define UAC_SHIFT			6
+#define UAC_SHIFT			10
 
 #endif
 
-- 
1.7.3.4

Re: [PATCH] Alpha: unbreak osf_setsysinfo(SSI_NVPAIRS, [SSIN_UACPROC, UAC_SIGBUS])

[Michael Cree]

On 12/08/11 03:25, slyich@gmail.com wrote:
> From: Sergei Trofimovich <slyfox@gentoo.org>
> 
> Update 'UAC_SHIFT' to match 'ALPHA_UAC_SHIFT'.
> 
> I don't know why kernel maintains 2 copies of the same constant.
> One (UAC_SHIFT) is used to shift bits in syscall definition, and
> another is in trap handler (ALPHA_UAC_SHIFT).
> 
> Was broken by
>> commit 745dd2405e281d96c0a449103bdf6a895048f28c
>> Author: Michael Cree <mcree@orcon.net.nz>

Bollocks.

The shift for the UAC flags was well and truly broken before that commit
as the TIF_UAC_NOPRINT, etc., flags did not match the definition of
either the UAC_ALPHA_SHIFT or the UAC_SHIFT flag.  I fixed
UAC_ALPHA_SHIFT to match the actual UAC flag definitions.

What I didn't realise was that UAC_SHIFT was also defined and used
elsewhere in the Alpha architecture code (so UAC_SHIFT _remained_ broken).

> diff --git a/arch/alpha/include/asm/sysinfo.h b/arch/alpha/include/asm/sysinfo.h
> index 086aba2..42b3edc 100644
> --- a/arch/alpha/include/asm/sysinfo.h
> +++ b/arch/alpha/include/asm/sysinfo.h
> @@ -32,7 +32,7 @@
>  
>  /* This is the shift that is applied to the UAC bits as stored in the
>     per-thread flags.  See thread_info.h.  */
> -#define UAC_SHIFT			6
> +#define UAC_SHIFT			10

But you haven't really fixed it.  If indeed UAC_SHIFT is exactly the
same as UAC_ALPHA_SHIFT then two definitions remain and that leaves the
possibility of someone changing one thereby screwing up the other.

Surely it would be better to unify UAC_ALPHA_SHIFT and UAC_SHIFT and use
one macro in all relevant places.

I am happy for you to reference my commit but I would prefer the commit
message saying something more along the line that it left part of the
fix undone which you are now completing.

With the above changes I will be happy to give my acknowledgement.

Thanks
Michael.

Re: [PATCH] Alpha: unbreak osf_setsysinfo(SSI_NVPAIRS, [SSIN_UACPROC, UAC_SIGBUS])

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

> > Was broken by
> >> commit 745dd2405e281d96c0a449103bdf6a895048f28c
> >> Author: Michael Cree <mcree@orcon.net.nz>
> 
> Bollocks.

Ugh, my apologies! Didn't intend to insult you.
I failed to read your changelog (and patch) properly.
Now I see The a583f1b54249b was the first in flag skew.

I'll build alpha toolchain / qemu root and will try to cook
nicer patch with compile-time safety checks.

Sorry.

-- 

  Sergei

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Alpha: unbreak osf_setsysinfo(SSI_NVPAIRS, [SSIN_UACPROC, UAC_SIGBUS])

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 2453 bytes --]

> I'll build alpha toolchain / qemu root and will try to cook
> nicer patch with compile-time safety checks.

Sent reworked patch: http://marc.info/?l=linux-alpha&m=131318270531656&w=2

Yelling at the header I've found minor nit:
...
#define SET_UNALIGN_CTL(task,value)     ({                                   \
        task_thread_info(task)->flags = ((task_thread_info(task)->flags &    \
                ~ALPHA_UAC_MASK)
                | (((value) << ALPHA_UAC_SHIFT)       & (1<<TIF_UAC_NOPRINT))\
                | (((value) << (ALPHA_UAC_SHIFT + 1)) & (1<<TIF_UAC_SIGBUS)) \
                | (((value) << (ALPHA_UAC_SHIFT - 1)) & (1<<TIF_UAC_NOFIX)));\
        0; })

#define GET_UNALIGN_CTL(task,value)     ({                              \
        put_user((task_thread_info(task)->flags & (1 << TIF_UAC_NOPRINT))\
                  >> ALPHA_UAC_SHIFT                                    \
                 | (task_thread_info(task)->flags & (1 << TIF_UAC_SIGBUS))\
                 >> (ALPHA_UAC_SHIFT + 1)                               \
                 | (task_thread_info(task)->flags & (1 << TIF_UAC_NOFIX))\
                 >> (ALPHA_UAC_SHIFT - 1),                              \
                 (int __user *)(value));                                \
        })

The macros SET_UNALIGN_CTL/GET_UNALIGN_CTL are used
only in kernel/sys.c:prctl syscall.

> | (((value) << ALPHA_UAC_SHIFT)       & (1<<TIF_UAC_NOPRINT))
prctl.h: # define PR_UNALIGN_NOPRINT     1

> |  (((value) << (ALPHA_UAC_SHIFT + 1)) & (1<<TIF_UAC_SIGBUS))
prctl.h: # define PR_UNALIGN_SIGBUS      2

> | (((value) << (ALPHA_UAC_SHIFT - 1)) & (1<<TIF_UAC_NOFIX)));

prctl.h: no '4' value

Do you think it's worth adding to userspace interface
or should one just remove it from handled by prctl flags [ABI change]?

Thanks!

diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index a3baeb2..f66539a 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -15,6 +15,7 @@
 #define PR_SET_UNALIGN   6
 # define PR_UNALIGN_NOPRINT    1       /* silently fix up unaligned user accesses */
 # define PR_UNALIGN_SIGBUS     2       /* generate SIGBUS on unaligned user access */
+# define PR_UNALIGN_NOFIX      4       /* skip offending instruction and do nothing */

 /* Get/set whether or not to drop capabilities on setuid() away from
  * uid 0 (as per security/commoncap.c) */

-- 

  Sergei

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] Alpha: unbreak osf_setsysinfo(SSI_NVPAIRS, [SSIN_UACPROC, UAC_SIGBUS])

[Michael Cree]

On 13/08/11 09:51, Sergei Trofimovich wrote:
>> I'll build alpha toolchain / qemu root and will try to cook
>> nicer patch with compile-time safety checks.
> 
> Sent reworked patch: http://marc.info/?l=linux-alpha&m=131318270531656&w=2
> 
> Yelling at the header I've found minor nit:
> ...
> #define SET_UNALIGN_CTL(task,value)     ({                                   \
>         task_thread_info(task)->flags = ((task_thread_info(task)->flags &    \
>                 ~ALPHA_UAC_MASK)
>                 | (((value) << ALPHA_UAC_SHIFT)       & (1<<TIF_UAC_NOPRINT))\
>                 | (((value) << (ALPHA_UAC_SHIFT + 1)) & (1<<TIF_UAC_SIGBUS)) \
>                 | (((value) << (ALPHA_UAC_SHIFT - 1)) & (1<<TIF_UAC_NOFIX)));\
>         0; })
> 
> #define GET_UNALIGN_CTL(task,value)     ({                              \
>         put_user((task_thread_info(task)->flags & (1 << TIF_UAC_NOPRINT))\
>                   >> ALPHA_UAC_SHIFT                                    \
>                  | (task_thread_info(task)->flags & (1 << TIF_UAC_SIGBUS))\
>                  >> (ALPHA_UAC_SHIFT + 1)                               \
>                  | (task_thread_info(task)->flags & (1 << TIF_UAC_NOFIX))\
>                  >> (ALPHA_UAC_SHIFT - 1),                              \
>                  (int __user *)(value));                                \
>         })
> 
> The macros SET_UNALIGN_CTL/GET_UNALIGN_CTL are used
> only in kernel/sys.c:prctl syscall.
> 
>> | (((value) << ALPHA_UAC_SHIFT)       & (1<<TIF_UAC_NOPRINT))
> prctl.h: # define PR_UNALIGN_NOPRINT     1
> 
>> |  (((value) << (ALPHA_UAC_SHIFT + 1)) & (1<<TIF_UAC_SIGBUS))
> prctl.h: # define PR_UNALIGN_SIGBUS      2
> 
>> | (((value) << (ALPHA_UAC_SHIFT - 1)) & (1<<TIF_UAC_NOFIX)));
> 
> prctl.h: no '4' value
> 
> Do you think it's worth adding to userspace interface
> or should one just remove it from handled by prctl flags [ABI change]?

My understanding is that user-space ABI changes/removals are
unacceptable.  It has been there for some time thus must remain.

But I do note that the NOFIX option is not documented in the prctl
syscall man page or in the prctl application man page, so only someone
acquainted with the kernel code would know it exists.

My feeling is that it should stay the way it is.  I'm not aware of a use
for the NOFIX option, and I have no idea why it was originally
implemented -- its implementation predates git history.

But others more familiar with early development of Alpha arch code may
disagree with me.

Cheers
Michael.