Linux Virtualization - Jul 2012 - [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

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 majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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