linux-kernel - Re: [PATCH] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1465885432.5365.214.camel@haakon3.risingtidesystems.com>
Date:	Mon, 13 Jun 2016 23:23:52 -0700
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	Bart Van Assche <bart.vanassche@...disk.com>
Cc:	Bryant G Ly <bryantly@...ux.vnet.ibm.com>,
	James Bottomley <James.Bottomley@...senPartnership.com>,
	martin.petersen@...cle.com, tyreld@...ux.vnet.ibm.com,
	akpm@...ux-foundation.org, kvalo@...eaurora.org,
	davem@...emloft.net, gregkh@...uxfoundation.org,
	mchehab@....samsung.com, jslaby@...e.com, joe@...ches.com,
	bp@...e.de, linux-kernel@...r.kernel.org,
	linux-scsi@...r.kernel.org, target-devel@...r.kernel.org,
	bgly <bgly@...ibm.com>
Subject: Re: [PATCH] ibmvscsis: Initial commit of IBM VSCSI Tgt Driver

On Fri, 2016-06-10 at 12:03 -0700, Bart Van Assche wrote:
> On 05/24/2016 01:00 PM, Bryant G Ly wrote:
> > Quoting Bart Van Assche <bart.vanassche@...disk.com>:
> >> On 05/24/2016 06:52 AM, Bryant G. Ly wrote:
> >>> +static uint64_t ibmvscsis_unpack_lun(const uint8_t *lun, int len)
> >>> +{
> >>> +    uint64_t res = NO_SUCH_LUN;
> >>> +    int addressing_method;
> >>> +
> >>> +    if (unlikely(len < 2)) {
> >>> +        pr_err("Illegal LUN length %d, expected 2 bytes or more\n",
> >>> +            len);
> >>> +        goto out;
> >>> +    }
> >>> +
> >>> +    switch (len) {
> >>> +    case 8:
> >>> +        if ((*((__be64 *)lun) & cpu_to_be64(0x0000FFFFFFFFFFFFLL))
> >>> != 0)
> >>> +            goto out_err;
> >>> +        break;
> >>> +    case 4:
> >>> +        if (*((__be16 *)&lun[2]) != 0)
> >>> +            goto out_err;
> >>> +        break;
> >>> +    case 6:
> >>> +        if (*((__be32 *)&lun[2]) != 0)
> >>> +            goto out_err;
> >>> +        break;
> >>> +    case 2:
> >>> +        break;
> >>> +    default:
> >>> +        goto out_err;
> >>> +    }
> >>> +
> >>> +    addressing_method = (*lun) >> 6; /* highest two bits of byte 0 */
> >>> +    switch (addressing_method) {
> >>> +    case SCSI_LUN_ADDR_METHOD_PERIPHERAL:
> >>> +    case SCSI_LUN_ADDR_METHOD_FLAT:
> >>> +    case SCSI_LUN_ADDR_METHOD_LUN:
> >>> +        res = *(lun + 1) | (((*lun) & 0x3f) << 8);
> >>> +        break;
> >>> +
> >>> +    case SCSI_LUN_ADDR_METHOD_EXTENDED_LUN:
> >>> +    default:
> >>> +        pr_err("Unimplemented LUN addressing method %u\n",
> >>> +            addressing_method);
> >>> +        break;
> >>> +    }
> >>> +
> >>> +out:
> >>> +    return res;
> >>> +out_err:
> >>> +    pr_err("Support for multi-level LUNs has not yet been
> >>> implemented\n");
> >>> +    goto out;
> >>> +}
> >>
> >> In the above function there is nothing that is specific to the VIO
> >> mechanism. Please consider to merge this function with
> >> scsilun_to_int(), e.g. by introducing a new function that accepts a
> >> SCSI LUN and the addressing method as arguments and by making
> >> scsilun_to_int() call that function with
> >> SCSI_LUN_ADDR_METHOD_PERIPHERAL as one of its arguments.
> >>
> >
> > We are going to keep this here atm, until we can make sure canonical can
> > pick up
> > these changes, since we have a deliverable for a 3rd party vendor who is
> > going to
> > fully utilize this driver. Once they can pick it up in 4.4 kernel I will
> > make the
> > change you recommended / move the scsi_lun_addr_method addressed below.
> > I will also
> > address the changes that ib_srpt needs to utilize this new function too.
> 
> A much better approach would be to move this function into the SCSI core 
> and to add a copy of that function to the backported driver that will be 
> provided to Canonical.

AFAICT, the problem with this improvement is that it's not stable
material, which means that Canonical would have to carry this separately
beyond linux-4.4.y, in order to support ibmvscsi-tgt.

As-is, it looks like we'll have two target-core fixes to support
ibmvscsi-tgt, both of which are considered stable material at this point
and will eventually make it to linux-4.4.y.

That said Bryant, I still don't have an objection for the initial commit
to use it's own internal ibmvscsis_unpack_lun(), but it would be nice in
the patch series containing the initial ibmvscsi-tgt commit to include
an incremental patch to make this common code, as Bart has highlighted.