iSCSI timeouts on login with 32/64bit user/kernel

[Lisa Marie]

Hello,

I've been trying to run open-iscsi with a 32bit userspce and a 64bit
kernel with no luck. I found references to this issue in the following
links:
https://groups.google.com/forum/?fromgroups=#!searchin/open-iscsi/64bit/open-iscsi/CQXCBczvIrE/NdXbde__HFIJ
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=502845

This issue is caused by two structs in the union iscsi_uevent.u in include/iscsi_if.h and linux/scsi/iscsi_if.h differning in
size on 32bit and 64bit systems. When this happens the union iscsi_uevent.r is offset by 4 bytes, causing reads in the 32bit
userspace to read in the wrong values.

I've attached the below patches which can be applied to both the kernel and the open-iscsi client. They have been tested on
x86/x86 user/kernel, and a x86/x86_64 user/kernel. This still needs to be tested on an x86_64/x86_64 user/kernel as well as any
other system that has a 32bit userspace and a 64bit kernel.

Thanks,
~Lisa M

open-iscsi.-alignment.patch:
--- a/include/iscsi_if.h
+++ b/include/iscsi_if.h
@@ -3,6 +3,7 @@
*
* Copyright (C) 2005 Dmitry Yusupov
* Copyright (C) 2005 Alex Aizman
+ * Copyright (C) 2012 Lisa Maginnis
* maintained by open-...@googlegroups.com
*
* This program is free software; you can redistribute it and/or modify
@@ -106,6 +107,14 @@ struct iscsi_uevent {
uint32_t iferror; /* carries interface or resource errors */
uint64_t transport_handle;

+ /* Structs in this union can differ on size between 32bit and 64bit
+ * systems. This will break systems running a 32bit userspace with a
+ * 64bit kernel. It is important to specify an alignment for the
+ * struct(s) so that their sizes match on both in both 32 and 64bit
+ * systems. Currently the two largest structs are:
+ * msg_bind_conn: 20 bytes, 24 aligned to the sizeof(uint64_t)
+ * stop_conn: 20 bytes, 24 aligned to the sizeof(uint64_t)
+ */
union {
/* messages u -> k */
struct msg_create_session {
@@ -131,7 +140,7 @@ struct iscsi_uevent {
uint32_t cid;
uint64_t transport_eph;
uint32_t is_leading;
- } b_conn;
+ } b_conn __attribute__((aligned (sizeof(uint64_t))));
struct msg_destroy_conn {
uint32_t sid;
uint32_t cid;
@@ -157,7 +166,7 @@ struct iscsi_uevent {
uint32_t cid;
uint64_t conn_handle;
uint32_t flag;
- } stop_conn;
+ } stop_conn __attribute__((aligned (sizeof(uint64_t))));
struct msg_get_stats {
uint32_t sid;
uint32_t cid;

linux-3.2-iscsi_if_alginment.patch:
--- a/linux-source-3.2/include/scsi/iscsi_if.h
+++ b/linux-source-3.2-patched/include/scsi/iscsi_if.h
@@ -3,6 +3,7 @@
*
* Copyright (C) 2005 Dmitry Yusupov
* Copyright (C) 2005 Alex Aizman
+ * Copyright (C) 2012 Lisa Maginnis
* maintained by open-...@googlegroups.com
*
* This program is free software; you can redistribute it and/or modify
@@ -85,6 +86,14 @@ struct iscsi_uevent {
uint32_t iferror; /* carries interface or resource errors */
uint64_t transport_handle;

+ /* Structs in this union can differ on size between 32bit and 64bit
+ * systems. This will break systems running a 32bit userspace with a
+ * 64bit kernel. It is important to specify an alignment for the
+ * struct(s) so that their sizes match on both in both 32 and 64bit
+ * systems. Currently the two largest structs are:
+ * msg_bind_conn: 20 bytes, 24 aligned to the sizeof(uint64_t)
+ * stop_conn: 20 bytes, 24 aligned to the sizeof(uint64_t)
+ */
union {
/* messages u -> k */
struct msg_create_session {
@@ -110,7 +119,7 @@ struct iscsi_uevent {
uint32_t cid;
uint64_t transport_eph;
uint32_t is_leading;
- } b_conn;
+ } b_conn __attribute__((aligned (sizeof(uint64_t))));
struct msg_destroy_conn {
uint32_t sid;
uint32_t cid;
@@ -136,7 +145,7 @@ struct iscsi_uevent {
uint32_t cid;
uint64_t conn_handle;
uint32_t flag;
- } stop_conn;
+ } stop_conn __attribute__((aligned (sizeof(uint64_t))));
struct msg_get_stats {
uint32_t sid;
uint32_t cid;

[Mike Christie]

On 10/28/2012 11:42 PM, Lisa Marie wrote:
>
> I've attached the below patches which can be applied to both the kernel
> and the open-iscsi client. They have been tested on
> x86/x86 user/kernel, and a x86/x86_64 user/kernel. This still needs to
> be tested on an x86_64/x86_64 user/kernel as well as any
> other system that has a 32bit userspace and a 64bit kernel.

Thanks a lot for the patch.

The problem with these type of patches has not been that we cannot find
a solution that works on new setups with the patches. The problem is
that we are not allowed to merge patches in the upstream kernel that
break existing setups. For example if the user has only upgraded the
kernel or only upgraded userspace tools, then we cannot break their
setups. This patch does not work on 32 bit kernels/userpsace if the
users only updates the tools.

[Michael Christie]

On Nov 1, 2012, at 12:46 AM, Lisa Marie <null...@SDF.ORG> wrote:

>> The problem with these type of patches has not been that we cannot find
>> a solution that works on new setups with the patches. The problem is
>> that we are not allowed to merge patches in the upstream kernel that
>> break existing setups. For example if the user has only upgraded the
>> kernel or only upgraded userspace tools, then we cannot break their
>> setups. This patch does not work on 32 bit kernels/userpsace if the
>> users only updates the tools.
>

> Per your request I've written a patch to allow for 32bit machines to work with un-patched 32bit and 64bit kernels. It should be trivial to sunset this patch with a kernel version number check when the kernel has the previous patch I submitted applied.
>
> I've tested this patch on 32/32 and 32/64 with an unpatched kernel.

Does it work if the kernel is updated but you are using old tools?

[Lisa Marie]

> The problem with these type of patches has not been that we cannot find
> a solution that works on new setups with the patches. The problem is
> that we are not allowed to merge patches in the upstream kernel that
> break existing setups. For example if the user has only upgraded the
> kernel or only upgraded userspace tools, then we cannot break their
> setups. This patch does not work on 32 bit kernels/userpsace if the
> users only updates the tools.

Per your request I've written a patch to allow for 32bit machines to work
with un-patched 32bit and 64bit kernels. It should be trivial to sunset
this patch with a kernel version number check when the kernel has the
previous patch I submitted applied.

I've tested this patch on 32/32 and 32/64 with an unpatched kernel.

Thanks,
~Lisa M

diff --git a/include/iscsi_if.h b/include/iscsi_if.h index
dad9fd8..db97332 100644 --- a/include/iscsi_if.h +++ b/include/iscsi_if.h

diff --git a/usr/iscsi_netlink.h b/usr/iscsi_netlink.h
index 25b41db..6b482ad 100644
--- a/usr/iscsi_netlink.h
+++ b/usr/iscsi_netlink.h
@@ -2,6 +2,7 @@
* iSCSI Netlink attr helpers
*
* Copyright (C) 2011 Red Hat, Inc. All rights reserved.

+ * Copyright (C) 2012 Lisa Maginnis
*

* This program is free software; you can redistribute it and/or modify

* it under the terms of the GNU General Public License as published
@@ -30,4 +31,10 @@ struct iovec;

extern struct nlattr *iscsi_nla_alloc(uint16_t type, uint16_t len);

+#ifdef __i386__
+int fix_32bit_kernel_structs; /* If set to 1, will cause reads from the kernel to be
+ * padded with an extra 4 bytes between iscsi_uevent.u and
+ * iscsi_uevent.r */
+#endif
+
#endif
diff --git a/usr/iscsid.c b/usr/iscsid.c
index b4bb65b..53381bf 100644
--- a/usr/iscsid.c
+++ b/usr/iscsid.c
@@ -4,6 +4,7 @@
* Copyright (C) 2004 Dmitry Yusupov, Alex Aizman
* Copyright (C) 2006 Mike Christie
* Copyright (C) 2006 Red Hat, Inc. All rights reserved.

+ * Copyright (C) 2012 Lisa Maginnis

* maintained by open-...@googlegroups.com
*

@@ -52,6 +53,10 @@
#include "iscsid_req.h"
#include "iscsi_err.h"

+#ifdef __i386__
+extern int fix_32bit_kernel_structs; /* For i386 compatibility. */
+#endif
+
/* global config info */
struct iscsi_daemon_config daemon_config;
struct iscsi_daemon_config *dconfig = &daemon_config;
@@ -336,6 +341,28 @@ static void missing_iname_warn(char *initiatorname_file)
"ignored.\n", initiatorname_file, initiatorname_file);
}

+#ifdef __i386__
+/* check_kernel_compatibility()
+ * This function detects when we are built in i386 and running a x86_64 (64bit) kernel and
+ * sets a flag. This flag is used to to signal the nlpayload_read() and nl_read() functions
+ * in netlink.c
+ */
+void check_kernel_compatibility()
+{
+ struct utsname uname_data;
+ fix_32bit_kernel_structs = 0;
+
+ /* Get our kernel and machine type for compatibility */
+ uname(&uname_data);
+
+ /* If we are not running a x86_64 kernel, enable the kernel struct fix.
+ * TODO: When/if kernel patches are made, a kernel version check should be done here. */
+ if(strcmp(uname_data.machine, "x86_64") != 0) {
+ fix_32bit_kernel_structs = 1;
+ }
+}
+#endif
+
int main(int argc, char *argv[])
{
struct utsname host_info; /* will use to compound initiator alias */
@@ -409,6 +436,11 @@ int main(int argc, char *argv[])
exit(ISCSI_ERR);
}

+#ifdef __i386__
+ /* Check and set kernel compatibility options (x86/x86_64 user/kernel). */
+ check_kernel_compatibility();
+#endif
+
umask(0177);

mgmt_ipc_fd = -1;
diff --git a/usr/netlink.c b/usr/netlink.c
index d65cd7c..b363439 100644
--- a/usr/netlink.c
+++ b/usr/netlink.c
@@ -4,6 +4,7 @@
* Copyright (C) 2004 Dmitry Yusupov, Alex Aizman
* Copyright (C) 2006 Mike Christie
* Copyright (C) 2006 Red Hat, Inc. All rights reserved.

+ * Copyright (C) 2012 Lisa Maginnis
* maintained by open-...@googlegroups.com
*
* This program is free software; you can redistribute it and/or modify

@@ -90,6 +91,38 @@ struct nlattr *iscsi_nla_alloc(uint16_t type, uint16_t len)
return attr;
}

+#ifdef __i386__
+#define OLD_32BIT_UEVENT_U_END_BYTE 35 /* Location of the iscsi_uevent struct */
+/* fix_32bit_uevent_struct(char*)
+ * Aligns the iscsi_uevent struct sent from an i386 (32bit) kernel to match our
+ * alignment in iscsi_if.h by adding a 4 byte (sizeof(uint32)) block of padding
+ * between two unions (u/r) in the struct.
+ * data Pointer to the uevent_iscsi struct.
+ * count Total number of bytes to copy.
+ * peek Set to 1 if uevent_iscsi was pulled from nl_read()/MSG_PEEK, else
+ * set to 0.
+*/
+void fix_32bit_uevent_struct(char *data, size_t count, int peek) {
+
+ log_debug(7, "in %s", __FUNCTION__);
+
+ /* Skip creating the offset if we are a PDU, CHAP, or STATS unless it is a peek
+ * (which is called from nl_read()) */
+ if(((uint8_t)data[0] != ISCSI_KEVENT_RECV_PDU
+ && (uint8_t)data[0] != ISCSI_UEVENT_GET_STATS
+ && (uint8_t)data[0] != ISCSI_UEVENT_GET_CHAP) || peek) {
+
+ /* Move iscsi_uevent.r four bytes (sizeof(uint32_t)) higher */
+ memmove(&data[OLD_32BIT_UEVENT_U_END_BYTE+sizeof(uint32_t)],
+ &data[OLD_32BIT_UEVENT_U_END_BYTE],
+ count-OLD_32BIT_UEVENT_U_END_BYTE-sizeof(uint32_t));
+
+ /* Clear the `uint32_t' padding between iscsi_uevent.u and iscsi_uevent.r */
+ memset(&data[OLD_32BIT_UEVENT_U_END_BYTE], 0, sizeof(uint32_t));
+ }
+}
+#endif
+
static int
kread(char *data, int count)
{
@@ -176,8 +209,17 @@ nlpayload_read(int ctrl_fd, char *data, int count, int flags)
*/
rc = recvmsg(ctrl_fd, &msg, flags);

- if (data)
- memcpy(data, NLMSG_DATA(iov.iov_base), count);
+ if (data) {
+ memcpy(data, NLMSG_DATA(iov.iov_base), count);
+
+#ifdef __i386__
+ /* Check if we need to clean this data for compatibility. */
+ if(fix_32bit_kernel_structs == 1) {
+ fix_32bit_uevent_struct(data, count, 0);
+ }
+#endif
+
+ }

return rc;
}
@@ -1281,6 +1323,13 @@ static int ctldev_handle(void)
nlh = (struct nlmsghdr *)nlm_ev;
ev = (struct iscsi_uevent *)NLMSG_DATA(nlm_ev);

+#ifdef __i386__
+ /* Check if we need to clean this data for compatibility. */
+ if(fix_32bit_kernel_structs == 1) {
+ fix_32bit_uevent_struct((char*)ev, sizeof(struct iscsi_uevent), 1);
+ }
+#endif
+
log_debug(7, "%s got event type %u\n", __FUNCTION__, ev->type);
/* drivers like qla4xxx can be inserted after iscsid is started */
switch (ev->type) {


On Mon, 29 Oct 2012, Mike Christie wrote:

> Date: Mon, 29 Oct 2012 14:48:52 -0500
> From: Mike Christie <mich...@cs.wisc.edu>
> To: open-...@googlegroups.com
> Cc: Lisa Marie <null...@SDF.ORG>
> Subject: Re: iSCSI timeouts on login with 32/64bit user/kernel

null...@sdf.org
SDF Public Access UNIX System - http://sdf.org