Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

[James Bottomley]

[adding autofs list and maintainer for their perusal and ack, although
this is an obvious fix to me

James]
On Thu, 2013-01-31 at 21:01 +0100, Helge Deller wrote:
> Similiar to other 64 bit Linux targets autofs_wqt_t needs to be of type int
> which has a size of 32 bits on 32- and 64-bit parisc kernels.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/include/uapi/linux/auto_fs.h b/include/uapi/linux/auto_fs.h
> index 77cdba9..d551754 100644
> --- a/include/uapi/linux/auto_fs.h
> +++ b/include/uapi/linux/auto_fs.h
> @@ -43,7 +43,7 @@
>   */
>  
>  #if defined(__sparc__) || defined(__mips__) || defined(__x86_64__) \
> - || defined(__powerpc__) || defined(__s390__)
> + || defined(__powerpc__) || defined(__s390__) || defined(__hppa__)
>  typedef unsigned int autofs_wqt_t;
>  #else
>  typedef unsigned long autofs_wqt_t;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

[Rolf Eike Beer]

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

James Bottomley wrote:
> [adding autofs list and maintainer for their perusal and ack, although
> this is an obvious fix to me
> 
> James]
> 
> On Thu, 2013-01-31 at 21:01 +0100, Helge Deller wrote:
> > Similiar to other 64 bit Linux targets autofs_wqt_t needs to be of type
> > int
> > which has a size of 32 bits on 32- and 64-bit parisc kernels.
> > 
> > Signed-off-by: Helge Deller <deller@gmx.de>
> > 
> > diff --git a/include/uapi/linux/auto_fs.h b/include/uapi/linux/auto_fs.h
> > index 77cdba9..d551754 100644
> > --- a/include/uapi/linux/auto_fs.h
> > +++ b/include/uapi/linux/auto_fs.h
> > @@ -43,7 +43,7 @@
> > 
> >   */
> >  
> >  #if defined(__sparc__) || defined(__mips__) || defined(__x86_64__) \
> > 
> > - || defined(__powerpc__) || defined(__s390__)
> > + || defined(__powerpc__) || defined(__s390__) || defined(__hppa__)
> > 
> >  typedef unsigned int autofs_wqt_t;
> >  #else
> >  typedef unsigned long autofs_wqt_t;

Wouldn't it make sense to make it an unsigned int on all platforms? This 
wouldn't make any difference on 32 bit platforms and would prevent that to 
happen on the next 32-on-64 bit platform again.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

[H. Peter Anvin]

On 01/31/2013 12:26 PM, James Bottomley wrote:
> [adding autofs list and maintainer for their perusal and ack, although
> this is an obvious fix to me
> 
> James]

The #if list probably should be inverted, and only specific platforms
should have unsigned long...

	-hpa

Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

[James Bottomley]

On Thu, 2013-01-31 at 13:29 -0800, H. Peter Anvin wrote:
> On 01/31/2013 12:26 PM, James Bottomley wrote:
> > [adding autofs list and maintainer for their perusal and ack, although
> > this is an obvious fix to me
> > 
> > James]
> 
> The #if list probably should be inverted, and only specific platforms
> should have unsigned long...

The whitelist is 64 bit only platforms, like ia64, isn't it?  So yes,
they're far less numerous.

James

Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

[H. Peter Anvin]

On 01/31/2013 01:42 PM, James Bottomley wrote:
> On Thu, 2013-01-31 at 13:29 -0800, H. Peter Anvin wrote:
>> On 01/31/2013 12:26 PM, James Bottomley wrote:
>>> [adding autofs list and maintainer for their perusal and ack, although
>>> this is an obvious fix to me
>>>
>>> James]
>>
>> The #if list probably should be inverted, and only specific platforms
>> should have unsigned long...
> 
> The whitelist is 64 bit only platforms, like ia64, isn't it?  So yes,
> they're far less numerous.
> 

Correct.  It is basically the 64-bit platforms without any 32-bit
userspace, like Alpha and IA64.

	-hpa

Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

[Rolf Eike Beer]

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

H. Peter Anvin wrote:
> On 01/31/2013 01:42 PM, James Bottomley wrote:
> > On Thu, 2013-01-31 at 13:29 -0800, H. Peter Anvin wrote:
> >> On 01/31/2013 12:26 PM, James Bottomley wrote:
> >>> [adding autofs list and maintainer for their perusal and ack, although
> >>> this is an obvious fix to me
> >>> 
> >>> James]
> >> 
> >> The #if list probably should be inverted, and only specific platforms
> >> should have unsigned long...
> > 
> > The whitelist is 64 bit only platforms, like ia64, isn't it?  So yes,
> > they're far less numerous.
> 
> Correct.  It is basically the 64-bit platforms without any 32-bit
> userspace, like Alpha and IA64.

Arm64? Should this be a CONFIG_ARCH_CAN_HAVE_32BIT_USERSPACE that would cover 
this? And may e.g. be used to remove compat_ioctl on those archs, too?

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

[H. Peter Anvin]

On 01/31/2013 02:10 PM, Rolf Eike Beer wrote:
> H. Peter Anvin wrote:
>> On 01/31/2013 01:42 PM, James Bottomley wrote:
>>> On Thu, 2013-01-31 at 13:29 -0800, H. Peter Anvin wrote:
>>>> On 01/31/2013 12:26 PM, James Bottomley wrote:
>>>>> [adding autofs list and maintainer for their perusal and
>>>>> ack, although this is an obvious fix to me
>>>>> 
>>>>> James]
>>>> 
>>>> The #if list probably should be inverted, and only specific
>>>> platforms should have unsigned long...
>>> 
>>> The whitelist is 64 bit only platforms, like ia64, isn't it?
>>> So yes, they're far less numerous.
>> 
>> Correct.  It is basically the 64-bit platforms without any
>> 32-bit userspace, like Alpha and IA64.
> 
> Arm64? Should this be a CONFIG_ARCH_CAN_HAVE_32BIT_USERSPACE that
> would cover this? And may e.g. be used to remove compat_ioctl on
> those archs, too?
> 

compat_ioctl should be CONFIG_COMPAT.

	-hpa

Re: [PATCH] parisc: unbreak automounter support on 64-bit kernel with 32-bit userspace

[Catalin Marinas]

On 31 January 2013 22:10, Rolf Eike Beer <eike-kernel@sf-tec.de> wrote:
> H. Peter Anvin wrote:
>> On 01/31/2013 01:42 PM, James Bottomley wrote:
>> > On Thu, 2013-01-31 at 13:29 -0800, H. Peter Anvin wrote:
>> >> On 01/31/2013 12:26 PM, James Bottomley wrote:
>> >>> [adding autofs list and maintainer for their perusal and ack, although
>> >>> this is an obvious fix to me
>> >>>
>> >>> James]
>> >>
>> >> The #if list probably should be inverted, and only specific platforms
>> >> should have unsigned long...
>> >
>> > The whitelist is 64 bit only platforms, like ia64, isn't it?  So yes,
>> > they're far less numerous.
>>
>> Correct.  It is basically the 64-bit platforms without any 32-bit
>> userspace, like Alpha and IA64.
>
> Arm64?

It supports 32-bit user-space (CONFIG_COMPAT). But we missed this
#ifdef for arm64. I'm not familiar with the autofs subsystem but at a
first look it seems that autofs_wqt_t types are not shared with user
space as part of a structure. It can come as an argument for an ioctl
command but as long as we don't expect this type to hold data larger
than 32-bit it should be ok.

-- 
Catalin

[PATCH] unbreak automounter support on 64-bit kernel with 32-bit userspace (v2)

[Helge Deller]

automount-support is broken on the parisc architecture, because the existing
#if list does not include a check for defined(__hppa__). The HPPA (parisc)
architecture is similiar to other 64bit Linux targets where we have to define
autofs_wqt_t (which is passed back and forth to user space) as int type which
has a size of 32bit across 32 and 64bit kernels.

During the discussion on the mailing list, H. Peter Anvin suggested to invert
the #if list since only specific platforms (specifically those who do not have
a 32bit userspace, like IA64 and Alpha) should have autofs_wqt_t as unsigned
long type.

This suggestion is probably the best way to go, since Arm64 (and maybe others?)
seems to have a non-working automounter. So in the long run even for other new
upcoming architectures this inverted check seem to be the best solution, since
it will not require them to change this #if again (unless they are 64bit only).

Signed-off-by: Helge Deller <deller@gmx.de>
CC: James Bottomley <James.Bottomley@HansenPartnership.com>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Rolf Eike Beer <eike-kernel@sf-tec.de>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Ian Kent <raven@themaw.net>


diff --git a/include/uapi/linux/auto_fs.h b/include/uapi/linux/auto_fs.h
index 77cdba9..bb991df 100644
--- a/include/uapi/linux/auto_fs.h
+++ b/include/uapi/linux/auto_fs.h
@@ -28,25 +28,16 @@
 #define AUTOFS_MIN_PROTO_VERSION	AUTOFS_PROTO_VERSION
 
 /*
- * Architectures where both 32- and 64-bit binaries can be executed
- * on 64-bit kernels need this.  This keeps the structure format
- * uniform, and makes sure the wait_queue_token isn't too big to be
- * passed back down to the kernel.
- *
- * This assumes that on these architectures:
- * mode     32 bit    64 bit
- * -------------------------
- * int      32 bit    32 bit
- * long     32 bit    64 bit
- *
- * If so, 32-bit user-space code should be backwards compatible.
+ * The wait_queue_token (autofs_wqt_t) is part of a structure which is passed
+ * back to the kernel via ioctl from userspace. On architectures where 32- and
+ * 64-bit userspace binaries can be executed it's important that the size of
+ * autofs_wqt_t stays constant between 32- and 64-bit Linux kernels so that we
+ * do not break the binary ABI interface by changing the structure size.
  */

Re: [PATCH] unbreak automounter support on 64-bit kernel with 32-bit userspace (v2)

[H. Peter Anvin]

On 02/04/2013 11:39 AM, Helge Deller wrote:
> 
> This suggestion is probably the best way to go, since Arm64 (and maybe others?)
> seems to have a non-working automounter. So in the long run even for other new
> upcoming architectures this inverted check seem to be the best solution, since
> it will not require them to change this #if again (unless they are 64bit only).
> 

... and even then they will generally work just fine.

	-hpa

Re: [PATCH] unbreak automounter support on 64-bit kernel with 32-bit userspace (v2)

[Ian Kent]

On Mon, 2013-02-04 at 20:39 +0100, Helge Deller wrote:
> automount-support is broken on the parisc architecture, because the existing
> #if list does not include a check for defined(__hppa__). The HPPA (parisc)
> architecture is similiar to other 64bit Linux targets where we have to define
> autofs_wqt_t (which is passed back and forth to user space) as int type which
> has a size of 32bit across 32 and 64bit kernels.
> 
> During the discussion on the mailing list, H. Peter Anvin suggested to invert
> the #if list since only specific platforms (specifically those who do not have
> a 32bit userspace, like IA64 and Alpha) should have autofs_wqt_t as unsigned
> long type.
> 
> This suggestion is probably the best way to go, since Arm64 (and maybe others?)
> seems to have a non-working automounter. So in the long run even for other new
> upcoming architectures this inverted check seem to be the best solution, since
> it will not require them to change this #if again (unless they are 64bit only).
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> CC: James Bottomley <James.Bottomley@HansenPartnership.com>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Rolf Eike Beer <eike-kernel@sf-tec.de>
> CC: H. Peter Anvin <hpa@zytor.com>
> CC: Ian Kent <raven@themaw.net>
> 
> 
> diff --git a/include/uapi/linux/auto_fs.h b/include/uapi/linux/auto_fs.h
> index 77cdba9..bb991df 100644
> --- a/include/uapi/linux/auto_fs.h
> +++ b/include/uapi/linux/auto_fs.h
> @@ -28,25 +28,16 @@
>  #define AUTOFS_MIN_PROTO_VERSION	AUTOFS_PROTO_VERSION
>  
>  /*
> - * Architectures where both 32- and 64-bit binaries can be executed
> - * on 64-bit kernels need this.  This keeps the structure format
> - * uniform, and makes sure the wait_queue_token isn't too big to be
> - * passed back down to the kernel.
> - *
> - * This assumes that on these architectures:
> - * mode     32 bit    64 bit
> - * -------------------------
> - * int      32 bit    32 bit
> - * long     32 bit    64 bit
> - *
> - * If so, 32-bit user-space code should be backwards compatible.
> + * The wait_queue_token (autofs_wqt_t) is part of a structure which is passed
> + * back to the kernel via ioctl from userspace. On architectures where 32- and
> + * 64-bit userspace binaries can be executed it's important that the size of
> + * autofs_wqt_t stays constant between 32- and 64-bit Linux kernels so that we
> + * do not break the binary ABI interface by changing the structure size.
>   */
> -
> -#if defined(__sparc__) || defined(__mips__) || defined(__x86_64__) \
> - || defined(__powerpc__) || defined(__s390__)
> -typedef unsigned int autofs_wqt_t;
> -#else
> +#if defined(__ia64__) || defined(__alpha__) /* pure 64bit architectures */
>  typedef unsigned long autofs_wqt_t;
> +#else
> +typedef unsigned int autofs_wqt_t;
>  #endif
>  
>  /* Packet types */

Acked-by: Ian Kent <raven@themaw.net>

Re: [PATCH] unbreak automounter support on 64-bit kernel with 32-bit userspace (v2)

[H. Peter Anvin]

On 02/05/2013 09:58 PM, Ian Kent wrote:
>>
>> diff --git a/include/uapi/linux/auto_fs.h b/include/uapi/linux/auto_fs.h
>> index 77cdba9..bb991df 100644
>> --- a/include/uapi/linux/auto_fs.h
>> +++ b/include/uapi/linux/auto_fs.h
>> @@ -28,25 +28,16 @@
>>   #define AUTOFS_MIN_PROTO_VERSION	AUTOFS_PROTO_VERSION
>>
>>   /*
>> - * Architectures where both 32- and 64-bit binaries can be executed
>> - * on 64-bit kernels need this.  This keeps the structure format
>> - * uniform, and makes sure the wait_queue_token isn't too big to be
>> - * passed back down to the kernel.
>> - *
>> - * This assumes that on these architectures:
>> - * mode     32 bit    64 bit
>> - * -------------------------
>> - * int      32 bit    32 bit
>> - * long     32 bit    64 bit
>> - *
>> - * If so, 32-bit user-space code should be backwards compatible.
>> + * The wait_queue_token (autofs_wqt_t) is part of a structure which is passed
>> + * back to the kernel via ioctl from userspace. On architectures where 32- and
>> + * 64-bit userspace binaries can be executed it's important that the size of
>> + * autofs_wqt_t stays constant between 32- and 64-bit Linux kernels so that we
>> + * do not break the binary ABI interface by changing the structure size.
>>    */
>> -
>> -#if defined(__sparc__) || defined(__mips__) || defined(__x86_64__) \
>> - || defined(__powerpc__) || defined(__s390__)
>> -typedef unsigned int autofs_wqt_t;
>> -#else
>> +#if defined(__ia64__) || defined(__alpha__) /* pure 64bit architectures */
>>   typedef unsigned long autofs_wqt_t;
>> +#else
>> +typedef unsigned int autofs_wqt_t;
>>   #endif
>>
>>   /* Packet types */
>
> Acked-by: Ian Kent <raven@themaw.net>
>

Acked-by: H. Peter Anvin <hpa@zytor.com>

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] unbreak automounter support on 64-bit kernel with 32-bit userspace (v2)

[Catalin Marinas]

On Mon, Feb 04, 2013 at 07:39:52PM +0000, Helge Deller wrote:
> automount-support is broken on the parisc architecture, because the existing
> #if list does not include a check for defined(__hppa__). The HPPA (parisc)
> architecture is similiar to other 64bit Linux targets where we have to define
> autofs_wqt_t (which is passed back and forth to user space) as int type which
> has a size of 32bit across 32 and 64bit kernels.
> 
> During the discussion on the mailing list, H. Peter Anvin suggested to invert
> the #if list since only specific platforms (specifically those who do not have
> a 32bit userspace, like IA64 and Alpha) should have autofs_wqt_t as unsigned
> long type.
> 
> This suggestion is probably the best way to go, since Arm64 (and maybe others?)
> seems to have a non-working automounter. So in the long run even for other new
> upcoming architectures this inverted check seem to be the best solution, since
> it will not require them to change this #if again (unless they are 64bit only).
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> CC: James Bottomley <James.Bottomley@HansenPartnership.com>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Rolf Eike Beer <eike-kernel@sf-tec.de>
> CC: H. Peter Anvin <hpa@zytor.com>
> CC: Ian Kent <raven@themaw.net>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>