Re: [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver
Hi Nicholas,
I just noticed this problem in the interface:
+#include linux/vhost.h
+
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: July 2012 version starting point for v3.6-rc merge
candidate +
+ *RFC-v2 vhost-scsi userspace. Add GET_ABI_VERSION ioctl
usage
+ */
+
+#define VHOST_SCSI_ABI_VERSION 0
+
+struct vhost_scsi_target {
+ int abi_version;
+ unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
+ unsigned short vhost_tpgt;
+};
+
Here TRANSPORT_IQN_LEN is 224, which is a multiple of 4.
Since vhost_tpgt is 2 bytes and abi_version is 4, the total size would
be 230. But gcc needs struct size be aligned to first field size, which
is 4 bytes, so it pads the structure by extra 2 bytes to the total of
232.
This padding is very undesirable in an ABI:
- it can not be initialized easily
- it can not be checked easily
- it can leak information between kernel and userspace
Simplest solution is probably just to make the padding
explicit:
+struct vhost_scsi_target {
+ int abi_version;
+ unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
+ unsigned short vhost_tpgt;
+ unsigned short reserved;
+};
+
I think we should fix this buglet before it goes out to users.
--
MST
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver
On Sat, 2012-08-18 at 23:04 +0300, Michael S. Tsirkin wrote:
Hi Nicholas,
I just noticed this problem in the interface:
+#include linux/vhost.h
+
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: July 2012 version starting point for v3.6-rc merge
candidate +
+ *RFC-v2 vhost-scsi userspace. Add GET_ABI_VERSION ioctl
usage
+ */
+
+#define VHOST_SCSI_ABI_VERSION 0
+
+struct vhost_scsi_target {
+ int abi_version;
+ unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
+ unsigned short vhost_tpgt;
+};
+
Here TRANSPORT_IQN_LEN is 224, which is a multiple of 4.
Since vhost_tpgt is 2 bytes and abi_version is 4, the total size would
be 230. But gcc needs struct size be aligned to first field size, which
is 4 bytes, so it pads the structure by extra 2 bytes to the total of
232.
This padding is very undesirable in an ABI:
- it can not be initialized easily
- it can not be checked easily
- it can leak information between kernel and userspace
H, yes. Very good reasons to avoid ABI ambiguity ..
Simplest solution is probably just to make the padding
explicit:
+struct vhost_scsi_target {
+ int abi_version;
+ unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
+ unsigned short vhost_tpgt;
+ unsigned short reserved;
+};
+
I think we should fix this buglet before it goes out to users.
nod, fixing this up in target-pending/master now w/ your reported-by
+signoff, and will change vhost-scsi's copy of these defs for next
week's RFC-v3 posting.
Thanks MST!
--nab
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver
Hi Linus, Ping on the initial tcm_vhost merge for-3.6..? I know it's been a busier than usual merge window, but hopefully this one is still in your PULL queue.. Otherwise if there is something else that you'd like to see different from this PULL request, please let us know. Thank you! --nab On Mon, 2012-07-30 at 18:19 -0700, Nicholas A. Bellinger wrote: Hi Linus, Here is the PULL request for the initial merge of tcm_vhost based on RFC-v5 code with MST's ACK appended to the initial merge commit. As promised, the commit is available from two different branches for you to consider merging as for-3.6 code. The 'for-next-merge' branch based on mainline commit 7409a6657ae using 3.5-rc2 code contains two duplicates of pre-merge vhost patch dependencies that have already been merged into mainline via net-next. This commit is also in the 07302012 -next patchset, and available here: git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next-merge Or the 'for-linus' branch containing an -rc0 head @ commit bdc0077af57: Merge tag 'scsi-misc' of git://git.kernel.org/../jejb/scsi) rebased up to the last commit in scsi-misc required for virtio-scsi client LLD scanning logic to function properly with tcm_vhost fabric ports, is available here: git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-linus Both branches have gotten recent testing and have been running over-night small block random I/O tests connected to raw block flash backends. The same diffstat below will result from pulling either branch. Also, the incremental patch to address MST's last round of post-merge comments has been sent to the lists for feedback this afternoon. This will be included into the usual post -rc1 PULL via 3.6-rc-fixes, along with any other bits that end up changing post-merge. Please let us know if you have any concerns. Thank you! --nab Nicholas Bellinger (1): tcm_vhost: Initial merge for vhost level target fabric driver drivers/vhost/Kconfig |3 + drivers/vhost/Kconfig.tcm |6 + drivers/vhost/Makefile|2 + drivers/vhost/tcm_vhost.c | 1628 + drivers/vhost/tcm_vhost.h | 101 +++ 5 files changed, 1740 insertions(+), 0 deletions(-) create mode 100644 drivers/vhost/Kconfig.tcm create mode 100644 drivers/vhost/tcm_vhost.c create mode 100644 drivers/vhost/tcm_vhost.h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver
Hi Linus, Here is the PULL request for the initial merge of tcm_vhost based on RFC-v5 code with MST's ACK appended to the initial merge commit. As promised, the commit is available from two different branches for you to consider merging as for-3.6 code. The 'for-next-merge' branch based on mainline commit 7409a6657ae using 3.5-rc2 code contains two duplicates of pre-merge vhost patch dependencies that have already been merged into mainline via net-next. This commit is also in the 07302012 -next patchset, and available here: git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next-merge Or the 'for-linus' branch containing an -rc0 head @ commit bdc0077af57: Merge tag 'scsi-misc' of git://git.kernel.org/../jejb/scsi) rebased up to the last commit in scsi-misc required for virtio-scsi client LLD scanning logic to function properly with tcm_vhost fabric ports, is available here: git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-linus Both branches have gotten recent testing and have been running over-night small block random I/O tests connected to raw block flash backends. The same diffstat below will result from pulling either branch. Also, the incremental patch to address MST's last round of post-merge comments has been sent to the lists for feedback this afternoon. This will be included into the usual post -rc1 PULL via 3.6-rc-fixes, along with any other bits that end up changing post-merge. Please let us know if you have any concerns. Thank you! --nab Nicholas Bellinger (1): tcm_vhost: Initial merge for vhost level target fabric driver drivers/vhost/Kconfig |3 + drivers/vhost/Kconfig.tcm |6 + drivers/vhost/Makefile|2 + drivers/vhost/tcm_vhost.c | 1628 + drivers/vhost/tcm_vhost.h | 101 +++ 5 files changed, 1740 insertions(+), 0 deletions(-) create mode 100644 drivers/vhost/Kconfig.tcm create mode 100644 drivers/vhost/tcm_vhost.c create mode 100644 drivers/vhost/tcm_vhost.h -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
