Re: [RFC] scsi: target: tcmu: running 32bit userspace on 64bit kernel

[Bodo Stroesser <bstroesser@ts.fujitsu.com>]

On 2020-06-30 18:52, James Bottomley wrote:
> On Tue, 2020-06-30 at 18:49 +0200, Bodo Stroesser wrote:
>> Hi,
>>
>> When using tcmu it might happen, that userspace application cannot be
>> built as 64 bit program even on a 64 bit host due to existing 32 bit
>> libraries that must be used, e.g. for compression, encryption,
>> deduplication, ...
>>
>> Currently this only works with manual changes in userspace include
>> file target_core_user.h due to a missing padding field in
>> struct tcmu_cmd_entry.
>>
>> Here are field offsets printed by a small program on a 64 bit host,
>> compiled as 64 bit program and as 32 bit:
>>
>> Devel:~ # gcc -o print_offsets print_offsets.c
>> Devel:~ # ./print_offsets
>> Offset of tcmu_cmd_entry.hdr.len_op          = 0000
>> Offset of tcmu_cmd_entry.hdr.cmd_id          = 0004
>> Offset of tcmu_cmd_entry.hdr.kflags          = 0006
>> Offset of tcmu_cmd_entry.hdr.uflags          = 0007
>>
>> Offset of tcmu_cmd_entry.req.iov_cnt         = 0008
>> Offset of tcmu_cmd_entry.req.iov_bidi_cnt    = 000c
>> Offset of tcmu_cmd_entry.req.iov_dif_cnt     = 0010
>> Offset of tcmu_cmd_entry.req.cdb_off         = 0018
>> Offset of tcmu_cmd_entry.req.iov[0]          = 0030
>>
>> Offset of tcmu_cmd_entry.rsp.scsi_status     = 0008
>> Offset of tcmu_cmd_entry.rsp.read_len        = 000c
>> Offset of tcmu_cmd_entry.rsp.sense_buffer[0] = 0010
>>
>> Size of struct tcmu_cmd_entry = 0070
>>
>>
>> Devel:~ # gcc -m32 -o print_offsets print_offsets.c
>> Devel:~ # ./print_offsets
>> Offset of tcmu_cmd_entry.hdr.len_op          = 0000
>> Offset of tcmu_cmd_entry.hdr.cmd_id          = 0004
>> Offset of tcmu_cmd_entry.hdr.kflags          = 0006
>> Offset of tcmu_cmd_entry.hdr.uflags          = 0007
>>
>> Offset of tcmu_cmd_entry.req.iov_cnt         = 0008
>> Offset of tcmu_cmd_entry.req.iov_bidi_cnt    = 000c
>> Offset of tcmu_cmd_entry.req.iov_dif_cnt     = 0010
>> Offset of tcmu_cmd_entry.req.cdb_off         = 0014
>> Offset of tcmu_cmd_entry.req.iov[0]          = 002c
>>
>> Offset of tcmu_cmd_entry.rsp.scsi_status     = 0008
>> Offset of tcmu_cmd_entry.rsp.read_len        = 000c
>> Offset of tcmu_cmd_entry.rsp.sense_buffer[0] = 0010
>>
>> Size of struct tcmu_cmd_entry = 0070
>>
>>
>> The offset of the fields req.cdb_off and req.iov differ for 64-bit
>> and 32-bit compilation.
>>
>> That means:
>>   - 64-bit application on 64-bit host works well
>>   - 32-bit application on 32-bit host works well
>>   - 32-bit application on 64-bit host fails.
>>
>> Unfortunately I don't see a way to fix this problem such, that
>> 32-bit application runs fine on 32-bit and 64-bit host without
>> breaking compatibility.
>>
>> So I'm wondering whether the following change would be a viable
>> solution:
>>
>> diff --git a/include/uapi/linux/target_core_user.h
>> b/include/uapi/linux/target_core_user.h
>> --- a/include/uapi/linux/target_core_user.h
>> +++ b/include/uapi/linux/target_core_user.h
>> @@ -114,6 +114,9 @@ struct tcmu_cmd_entry {
>>   			__u32 iov_cnt;
>>   			__u32 iov_bidi_cnt;
>>   			__u32 iov_dif_cnt;
>> +#ifdef APPL32BIT_ON_KERNEL64BIT
>> +			__u32 __pad9;
>> +#endif
>>   			__u64 cdb_off;
>>   			__u64 __pad1;
>>   			__u64 __pad2;
>>
>>
>> Using this change we can do:
>>
>> Devel:~ # gcc -m32 -DAPPL32BIT_ON_KERNEL64BIT -o print_offsets
>> print_offsets.c
>> Devel:~ # ./print_offsets
>> Offset of tcmu_cmd_entry.hdr.len_op          = 0000
>> Offset of tcmu_cmd_entry.hdr.cmd_id          = 0004
>> Offset of tcmu_cmd_entry.hdr.kflags          = 0006
>> Offset of tcmu_cmd_entry.hdr.uflags          = 0007
>>
>> Offset of tcmu_cmd_entry.req.iov_cnt         = 0008
>> Offset of tcmu_cmd_entry.req.iov_bidi_cnt    = 000c
>> Offset of tcmu_cmd_entry.req.iov_dif_cnt     = 0010
>> Offset of tcmu_cmd_entry.req.cdb_off         = 0018
>> Offset of tcmu_cmd_entry.req.iov[0]          = 0030
>>
>> Offset of tcmu_cmd_entry.rsp.scsi_status     = 0008
>> Offset of tcmu_cmd_entry.rsp.read_len        = 000c
>> Offset of tcmu_cmd_entry.rsp.sense_buffer[0] = 0010
>>
>> Size of struct tcmu_cmd_entry = 0070
>>
>>
>> So we can compile a 32-bit application that works on 64-bit kernel
>> without need to manipulate the include file prepared by the kernel.
>>
>> What do you think? Do you know a better solution?
> 
> Can you not use something similar to the compat_ioctl mechanism?  the
> job of the compat layer is to re-layout the input and output structures
> to impedance match between 32 and 64 bit.
> 
> James
> 

struct tcmu_cmd_entry is prepared by kernel and exposed to userspace via
an mmap()ed uio device. From tcmu module point of view it is vmalloc
area.

So there is no 'glue' code that could do conversion.

I'm not sure whether there is a way for tcmu to find out, what kind of
userspace application is running. If it would know, it probably could
prepare the entries accordingly.

But worst case it could happen that entries prepared for 32-bit, after
application stop and start, then are read by a new application version
that is 64-bit ...

Maybe we could add an configFS attribute allowing userspace to configure
tcmu accordingly. A change of this attribute could also flush existing
tcmu_cmd_entries. But that would mean that userspace application has to
take care.

Isn't the compiler definition easier to use?

Bodo