Re: [PATCH] bfa: fix access to bfad_im_port_s

[James Bottomley]

On Tue, 2017-11-28 at 16:26 +0100, Johannes Thumshirn wrote:
> Commit 'cd21c605b2cf ("scsi: fc: provide fc_bsg_to_shost() helper")'
> changed access to bfa's 'struct bfad_im_port_s' by using shost_priv()
> instead of shost->hostdata[0].
> 
> This lead to crashes like in the following back-trace:
> 
> task: ffff880046375300 ti: ffff8800a2ef8000 task.ti: ffff8800a2ef8000
> RIP: e030:[<ffffffffa04c8252>]ÂÂ[<ffffffffa04c8252>]
> bfa_fcport_get_attr+0x82/0x260 [bfa]
> RSP: e02b:ffff8800a2efba10ÂÂEFLAGS: 00010046
> RAX: 575f415441536432 RBX: ffff8800a2efba28 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff8800a2efba28 RDI: ffff880004dc31d8
> RBP: ffff880004dc31d8 R08: 0000000000000000 R09: 0000000000000001
> R10: ffff88011fadc468 R11: 0000000000000001 R12: ffff880004dc31f0
> R13: 0000000000000200 R14: ffff880004dc61d0 R15: ffff880004947a10
> FS:ÂÂ00007feb1e489700(0000) GS:ffff88011fac0000(0000)
> knlGS:0000000000000000
> CS:ÂÂe033 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 00007ffe14e46c10 CR3: 00000000957b8000 CR4: 0000000000000660
> Stack:
> Âffff88001d4da000 ffff880004dc31c0 ffffffffa048a9df ffffffff81e56380
> Â0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [] bfad_iocmd_ioc_get_info+0x4f/0x220 [bfa]
> [] bfad_iocmd_handler+0xa00/0xd40 [bfa]
> [] bfad_im_bsg_request+0xee/0x1b0 [bfa]
> [] fc_bsg_dispatch+0x10b/0x1b0 [scsi_transport_fc]
> [] bsg_request_fn+0x11d/0x1c0
> [] __blk_run_queue+0x2f/0x40
> [] blk_execute_rq_nowait+0xa8/0x160
> [] blk_execute_rq+0x77/0x120
> [] bsg_ioctl+0x1b6/0x200
> [] do_vfs_ioctl+0x2cd/0x4a0
> [] SyS_ioctl+0x74/0x80
> [] entry_SYSCALL_64_fastpath+0x12/0x6d
> 
> Fixes: cd21c605b2cf ("scsi: fc: provide fc_bsg_to_shost() helper")
> Signed-off-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
> Cc: Michal Koutnà <mkoutny@xxxxxxxx>
> ---
> Âdrivers/scsi/bfa/bfad_bsg.c | 6 ++++--
> Â1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/bfa/bfad_bsg.c
> b/drivers/scsi/bfa/bfad_bsg.c
> index 72ca2a2e08e2..09ef68c8225f 100644
> --- a/drivers/scsi/bfa/bfad_bsg.c
> +++ b/drivers/scsi/bfa/bfad_bsg.c
> @@ -3135,7 +3135,8 @@ bfad_im_bsg_vendor_request(struct bsg_job *job)
> Â	struct fc_bsg_request *bsg_request = job->request;
> Â	struct fc_bsg_reply *bsg_reply = job->reply;
> Â	uint32_t vendor_cmd = bsg_request-
> >rqst_data.h_vendor.vendor_cmd[0];
> -	struct bfad_im_port_s *im_port =
> shost_priv(fc_bsg_to_shost(job));
> +	struct Scsi_Host *shost = fc_bsg_to_shost(job);
> +	struct bfad_im_port_s *im_port = shost->hostdata[0];
> Â	struct bfad_s *bfad = im_port->bfad;
> Â	void *payload_kbuf;
> Â	int rc = -EINVAL;
> @@ -3350,7 +3351,8 @@ int
> Âbfad_im_bsg_els_ct_request(struct bsg_job *job)
> Â{
> Â	struct bfa_bsg_data *bsg_data;
> -	struct bfad_im_port_s *im_port =
> shost_priv(fc_bsg_to_shost(job));
> +	struct Scsi_Host *shost = fc_bsg_to_shost(job);
> +	struct bfad_im_port_s *im_port = shost->hostdata[0];
> Â	struct bfad_s *bfad = im_port->bfad;
> Â	bfa_bsg_fcpt_t *bsg_fcpt;
> Â	struct bfad_fcxpÂÂÂÂ*drv_fcxp;

OK, so we had a linux next failure with this:

    After merging the scsi tree, today's linux-next build (x86_64
    allmodconfig) produced these warnings:

    drivers/scsi/bfa/bfad_bsg.c: In function 'bfad_im_bsg_vendor_request':
    drivers/scsi/bfa/bfad_bsg.c:3137:35: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
      struct bfad_im_port_s *im_port = shost->hostdata[0];
                                       ^
    drivers/scsi/bfa/bfad_bsg.c: In function 'bfad_im_bsg_els_ct_request':
    drivers/scsi/bfa/bfad_bsg.c:3353:35: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
      struct bfad_im_port_s *im_port = shost->hostdata[0];
                                       ^

    Introduced by commit

      45349821ab3a ("scsi: bfa: fix access to bfad_im_port_s")

    -- 
    Cheers,
    Stephen Rothwell

The reason is you should be using shost_priv(shost) not shost->hostdata[0].

James