Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Johannes Thumshirn]

On Fri, Oct 28, 2016 at 11:53:46AM +0200, Steffen Maier wrote:

[...]

> > > 
> > > > @@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue *q, 
> > > > struct Scsi_Host *shost,
> > > > struct request *req;
> > > > struct fc_bsg_job *job;
> > > > enum fc_dispatch_result ret;
> > > > +   struct fc_bsg_reply *bsg_reply;
> > > > 
> > > > if (!get_device(dev))
> > > > return;
> > > > @@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue *q, 
> > > > struct Scsi_Host *shost,
> > > > /* check if we have the msgcode value at least */
> > > > if (job->request_len < sizeof(uint32_t)) {
> > > > BUG_ON(job->reply_len < sizeof(uint32_t));
> > > > -   job->reply->reply_payload_rcv_len = 0;
> > > > -   job->reply->result = -ENOMSG;
> > > > +   bsg_reply = job->reply;
> > > > +   bsg_reply->reply_payload_rcv_len = 0;
> > > > +   bsg_reply->result = -ENOMSG;
> 
> Compiler optimization re-ordered above two lines and the first pointer
> derefence is bsg_reply->result [field offset 0] where bsg_reply is NULL.
> The assignment tries to write to memory at address NULL causing the kernel
> page fault.
> 
> Does your suggested change for [PATCH v3 02/16], shuffling the
> job->request_len checks, address above kernel page fault?

This is what I hope at least. I'm sorry but I don't have any experience with 
s390
and zfcp at all. I still need to get a test environment set up, but all the
people knowing how to do are rather busy at the moment. All my tests on x86_64
with FCoE and lpfc haven't had a problem so far.

Thanks,
Johannes
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Andreas Krebbel1]

> On 10/28/2016 01:31 PM, Hannes Reinecke wrote:
> > On 10/28/2016 11:53 AM, Steffen Maier wrote:
> >> On 10/13/2016 06:24 PM, Johannes Thumshirn wrote:
> >>> On Thu, Oct 13, 2016 at 05:15:25PM +0200, Steffen Maier wrote:
...
> fc_bsg_request_handler()
>  req->errors = -ENXIO;
> 
> > 0x7c8e6e : mvhi260(%r12),-6
> 
> crash> struct -od request.errors
> struct request {
>[260] int errors;
> }
> 
> 
> 
> BUT this seems the first time %r12 is used in fc_bsg_request_handler(),
> especially I seem to miss %r12 being initalized with anything.
> But then again I'm not at all well versed in disassembly.
> Maybe fc_bsg_request_handler() is itself in turn inlined and I would 
> need to start disassembling even earlier to get to %r12 init?
> s390x ELF ABI says %r12:
> usage: Local variable, commonly used as GOT pointer;
> call effect: saved.
> Even if it wasn't initialized and remained NULL below why did it not 
> already page fault at above instruction? Silly me, we did not execute 
> this instruction as it's "if" conditional. This makes me wonder even 
> more where the content of %r12 comes from.
> 
> Ulli, Andreas, could you please shed some light on this?
> 
> 

r12 holds variable req for that access. It is initialized here:

  req = blk_fetch_request(q);
  if (!req)   
break;

The asm code ends up down below in the function and loads the return value 
into r12. The code
invoking blk_fetch_request got duplicated and there are three jumps before 
the r12 access to these
locations.

  7c8e48:   ec a8 02 14 00 7c   cgije   %r10,0,7c9270 
  <--- x
  7c8e4e:   d5 03 d0 04 a0 28   clc 4(4,%r13),40(%r10)
  7c8e54:   a7 74 02 02 jne 7c9258 
 <--- y
  7c8e58:   91 04 a0 48 tm  72(%r10),4
  7c8e5c:   a7 74 01 fe jne 7c9258 
 <--- y
  7c8e60:   a7 f4 01 d5 j   7c920a 

  7c8e64:   d5 03 d0 00 a0 28   clc 0(4,%r13),40(%r10)
  7c8e6a:   a7 84 00 1a je  7c8e9e 

  7c8e6e:   e5 4c c1 04 ff fa   mvhi260(%r12),-6
...

  7c9258:   b9 04 00 29 lgr %r2,%r9
  7c925c:   c0 e5 ff f7 b3 a6   brasl   %r14,6bf9a8 

  7c9262:   b9 04 00 c2 lgr %r12,%r2
  7c9266:   ec 26 fd ff 00 7c   cgijne  %r2,0,7c8e64 

  7c926c:   a7 f4 ff cf j   7c920a 


  7c9270:   b9 04 00 29 lgr %r2,%r9
  7c9274:   c0 e5 ff f7 b3 9a   brasl   %r14,6bf9a8 

  7c927a:   b9 04 00 c2 lgr %r12,%r2
  7c927e:   ec 26 fe 10 00 7c   cgijne  %r2,0,7c8e9e 


-Andreas-

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Steffen Maier]

On 10/28/2016 01:31 PM, Hannes Reinecke wrote:

On 10/28/2016 11:53 AM, Steffen Maier wrote:

On 10/13/2016 06:24 PM, Johannes Thumshirn wrote:

On Thu, Oct 13, 2016 at 05:15:25PM +0200, Steffen Maier wrote:

I'm puzzled.

$ git bisect start fc_bsg master



3087864ce3d7282f59021245d8a5f83ef1caef18 is the first bad commit
commit 3087864ce3d7282f59021245d8a5f83ef1caef18
Author: Johannes Thumshirn 
Date:   Wed Oct 12 15:06:28 2016 +0200

scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

Don't use fc_bsg_job::request and fc_bsg_job::reply directly,
but use
helper variables bsg_request and bsg_reply. This will be
helpfull  when
transitioning to bsg-lib.

Signed-off-by: Johannes Thumshirn 

:04 04 140c4b6829d5cfaec4079716e0795f63f8bc3bd2
0d9fe225615679550be91fbd9f84c09ab1e280fc Mdrivers


From there (on the reverse bisect path) I get the following Oops,
except for the full patch set having another stack trace as in my
previous
mail (dying in zfcp code).


[...]


@@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue
*q, struct Scsi_Host *shost,
 struct request *req;
 struct fc_bsg_job *job;
 enum fc_dispatch_result ret;
+struct fc_bsg_reply *bsg_reply;

 if (!get_device(dev))
 return;
@@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue
*q, struct Scsi_Host *shost,
 /* check if we have the msgcode value at least */
 if (job->request_len < sizeof(uint32_t)) {
 BUG_ON(job->reply_len < sizeof(uint32_t));
-job->reply->reply_payload_rcv_len = 0;
-job->reply->result = -ENOMSG;
+bsg_reply = job->reply;
+bsg_reply->reply_payload_rcv_len = 0;
+bsg_reply->result = -ENOMSG;


Compiler optimization re-ordered above two lines and the first pointer
derefence is bsg_reply->result [field offset 0] where bsg_reply is NULL.
The assignment tries to write to memory at address NULL causing the
kernel page fault.


I spoke to our compiler people, and they strongly believed this not to
be the case. Or, put it the other way round, if such a thing would
happen it would be a compiler issue.

Have you checked the compiler output?


I just mentioned the compiler optimization to explain why the assembler 
code visible in the panic dies at bsg_reply->result = -ENOMSG and not at 
bsg_reply->reply_payload_rcv_len = 0. I don't think it makes a 
difference regarding the issue, which remains a NULL pointer dereference 
with bsg_reply either way, which I doubt is caused by compiler output. 
But then again, see further down below.



[   46.942560] Krnl PSW : 0704e0018000 007c91ec[   46.942574]  
(fc_bsg_request_handler+0x404/0x4b0)
[   46.942579]
[   46.942583]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:000:
[   46.942598]  RI:0 EA:3
[   46.942601]
[   46.942601] Krnl GPRS:  ffcb  
8001
[   46.942603]007c8fe8 64398c68 69f967e8 
6a3d8008
[   46.942605]6a5e02c8 698b5490  




   %r11 is NULL


[   46.942607]6a9ef5f8 00a36840 007c8fe8 
5d2efa00
[   46.942619] Krnl Code: 007c91de: e55dc08c0003clfhsi  
140(%r12),3[   46.942622]
[   46.942622]007c91e4: a7240004brc 2,7c91ec
 #007c91e8: a7f40001   brc 
15,7c91ea[   46.942629]
[   46.942629]   >007c91ec: 5010b000st  
%r1,0(%r11)
  007c91f0: e54cb004   mvhi
4(%r11),0[   46.942635]
[   46.942635]007c91f6: e54cc08c0004mvhi
140(%r12),4
  007c91fc: b904002c   lgr 
%r2,%r12[   46.942643]
[   46.942643]007c9200: c0e5e2c0brasl   
%r14,7c5780
[   46.942646]
[   46.942647] Call Trace:
[   46.942650] ([<007c8fe8>] fc_bsg_request_handler+0x200/0x4b0)
[   46.942656] ([<006b8e0a>] __blk_run_queue+0x52/0x68)
[   46.942661] ([<006c549a>] blk_execute_rq_nowait+0xf2/0x110)
[   46.942664] ([<006c557a>] blk_execute_rq+0xa2/0x110)
[   46.942668] ([<006de0ee>] bsg_ioctl+0x1f6/0x268)
[   46.942675] ([<0036ca20>] do_vfs_ioctl+0x680/0x6d8)
[   46.942677] ([<0036caf4>] SyS_ioctl+0x7c/0xb0)
[   46.942685] ([<009a541e>] system_call+0xd6/0x270)
[   46.942687] INFO: lockdep is turned off.
[   46.942688] Last Breaking-Event-Address:
[   46.942692]  [<007c91e4>] fc_bsg_request_handler+0x3fc/0x4b0
[   46.942696]  [   46.942698] Kernel panic - not syncing: Fatal exception: 
panic_on_oops


all the following was written from bottom to top:


crash> dis -l fc_bsg_request_handler

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Hannes Reinecke]

On 10/28/2016 11:53 AM, Steffen Maier wrote:
> 
> 
> On 10/13/2016 06:24 PM, Johannes Thumshirn wrote:
>> On Thu, Oct 13, 2016 at 05:15:25PM +0200, Steffen Maier wrote:
>>> I'm puzzled.
>>>
>>> $ git bisect start fc_bsg master
> 
 3087864ce3d7282f59021245d8a5f83ef1caef18 is the first bad commit
 commit 3087864ce3d7282f59021245d8a5f83ef1caef18
 Author: Johannes Thumshirn 
 Date:   Wed Oct 12 15:06:28 2016 +0200

 scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

 Don't use fc_bsg_job::request and fc_bsg_job::reply directly,
 but use
 helper variables bsg_request and bsg_reply. This will be
 helpfull  when
 transitioning to bsg-lib.

 Signed-off-by: Johannes Thumshirn 

 :04 04 140c4b6829d5cfaec4079716e0795f63f8bc3bd2
 0d9fe225615679550be91fbd9f84c09ab1e280fc Mdrivers
>>>
>>> From there (on the reverse bisect path) I get the following Oops,
>>> except for the full patch set having another stack trace as in my
>>> previous
>>> mail (dying in zfcp code).
>>>
>>
>> [...]
>>
>>>
 @@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue
 *q, struct Scsi_Host *shost,
  struct request *req;
  struct fc_bsg_job *job;
  enum fc_dispatch_result ret;
 +struct fc_bsg_reply *bsg_reply;

  if (!get_device(dev))
  return;
 @@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue
 *q, struct Scsi_Host *shost,
  /* check if we have the msgcode value at least */
  if (job->request_len < sizeof(uint32_t)) {
  BUG_ON(job->reply_len < sizeof(uint32_t));
 -job->reply->reply_payload_rcv_len = 0;
 -job->reply->result = -ENOMSG;
 +bsg_reply = job->reply;
 +bsg_reply->reply_payload_rcv_len = 0;
 +bsg_reply->result = -ENOMSG;
> 
> Compiler optimization re-ordered above two lines and the first pointer
> derefence is bsg_reply->result [field offset 0] where bsg_reply is NULL.
> The assignment tries to write to memory at address NULL causing the
> kernel page fault.
> 
I spoke to our compiler people, and they strongly believed this not to
be the case. Or, put it the other way round, if such a thing would
happen it would be a compiler issue.

Have you checked the compiler output?

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Steffen Maier]

On 10/13/2016 06:24 PM, Johannes Thumshirn wrote:

On Thu, Oct 13, 2016 at 05:15:25PM +0200, Steffen Maier wrote:

I'm puzzled.

$ git bisect start fc_bsg master



3087864ce3d7282f59021245d8a5f83ef1caef18 is the first bad commit
commit 3087864ce3d7282f59021245d8a5f83ef1caef18
Author: Johannes Thumshirn 
Date:   Wed Oct 12 15:06:28 2016 +0200

scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
helper variables bsg_request and bsg_reply. This will be helpfull  when
transitioning to bsg-lib.

Signed-off-by: Johannes Thumshirn 

:04 04 140c4b6829d5cfaec4079716e0795f63f8bc3bd2 
0d9fe225615679550be91fbd9f84c09ab1e280fc M  drivers


From there (on the reverse bisect path) I get the following Oops,
except for the full patch set having another stack trace as in my previous
mail (dying in zfcp code).



[...]




@@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue *q, struct 
Scsi_Host *shost,
struct request *req;
struct fc_bsg_job *job;
enum fc_dispatch_result ret;
+   struct fc_bsg_reply *bsg_reply;

if (!get_device(dev))
return;
@@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue *q, struct 
Scsi_Host *shost,
/* check if we have the msgcode value at least */
if (job->request_len < sizeof(uint32_t)) {
BUG_ON(job->reply_len < sizeof(uint32_t));
-   job->reply->reply_payload_rcv_len = 0;
-   job->reply->result = -ENOMSG;
+   bsg_reply = job->reply;
+   bsg_reply->reply_payload_rcv_len = 0;
+   bsg_reply->result = -ENOMSG;


Compiler optimization re-ordered above two lines and the first pointer 
derefence is bsg_reply->result [field offset 0] where bsg_reply is NULL.
The assignment tries to write to memory at address NULL causing the 
kernel page fault.


Does your suggested change for [PATCH v3 02/16], shuffling the 
job->request_len checks, address above kernel page fault?



job->reply_len = sizeof(uint32_t);
fc_bsg_jobdone(job);
spin_lock_irq(q->queue_lock);



Ahm and what exactly can break here? It's just assigning variables. Now
I'm puzzled too.



--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Steffen Maier]

I'm puzzled.

$ git bisect start fc_bsg master
Bisecting: 8 revisions left to test after this (roughly 3 steps)
[005d51510eee6102636d5dbb06310531c5d46151] scsi: fc: implement kref 
backed reference counting

$ git bisect bad
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[bef6da201de1bb81bb4d9511f9a155862efc251f] scsi: Unify interfaces of 
fc_bsg_jobdone and bsg_job_done

$ git bisect bad
Bisecting: 1 revision left to test after this (roughly 1 step)
[3087864ce3d7282f59021245d8a5f83ef1caef18] scsi: don't use 
fc_bsg_job::request and fc_bsg_job::reply directly

$ git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[81aea44720d22d2e0c4a2613ae8b1c256ef6b0cb] scsi: Get rid of struct 
fc_bsg_buffer

$ git bisect good
3087864ce3d7282f59021245d8a5f83ef1caef18 is the first bad commit
commit 3087864ce3d7282f59021245d8a5f83ef1caef18
Author: Johannes Thumshirn 
Date:   Wed Oct 12 15:06:28 2016 +0200

scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
helper variables bsg_request and bsg_reply. This will be helpfull  when
transitioning to bsg-lib.

Signed-off-by: Johannes Thumshirn 

:04 04 140c4b6829d5cfaec4079716e0795f63f8bc3bd2 
0d9fe225615679550be91fbd9f84c09ab1e280fc M  drivers


From there (on the reverse bisect path) I get the following Oops,
except for the full patch set having another stack trace as in my 
previous mail (dying in zfcp code).



[   46.942452] Oops: 0004 ilc:2 [#1] [   46.942460] PREEMPT  SMP [   46.942465]
[   46.942470] Modules linked in: nf_log_ipv6 xt_pkttype nf_log_ipv4 
nf_log_common xt_LOG xt_limit ip6t_REJECT nf_reject_ipv6 xt_tcpudp 
nf_conntrack_ipv6 nf_defrag_ipv6 ip6table_raw ipt_REJECT nf_reject_ipv4 
iptable_raw xt_CT iptable_filter ip6table_mangle nf_conntrack_netbios_ns 
nf_conntrack_broadcast nf_conntrack_ipv4 nf_defrag_ipv4 ip_tables xt_conntrack 
nf_conntrack ip6table_filter ip6_tables x_tables qeth_l2 ghash_s390 prng ecb 
aes_s390 des_s390 des_generic sha512_s390 sha256_s390 sha1_s390 sha_common 
dm_mod qeth ccwgroup zfcp qdio autofs4
[   46.942547] CPU: 1 PID: 1714 Comm: zfcp_ping Not tainted 4.8.0fcbsg+ #9
[   46.942550] Hardware name: IBM  2964 N96  702
  (z/VM)
[   46.942556] task: 5c988008 task.stack: 5d2ec000
[   46.942560] Krnl PSW : 0704e0018000 007c91ec[   46.942574]  
(fc_bsg_request_handler+0x404/0x4b0)
[   46.942579]
[   46.942583]R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:000:
[   46.942598]  RI:0 EA:3
[   46.942601]
[   46.942601] Krnl GPRS:  ffcb  
8001
[   46.942603]007c8fe8 64398c68 69f967e8 
6a3d8008
[   46.942605]6a5e02c8 698b5490  

[   46.942607]6a9ef5f8 00a36840 007c8fe8 
5d2efa00
[   46.942619] Krnl Code: 007c91de: e55dc08c0003clfhsi  
140(%r12),3[   46.942622]
[   46.942622]007c91e4: a7240004brc 2,7c91ec
 #007c91e8: a7f40001   brc 
15,7c91ea[   46.942629]
[   46.942629]   >007c91ec: 5010b000st  
%r1,0(%r11)
  007c91f0: e54cb004   mvhi
4(%r11),0[   46.942635]
[   46.942635]007c91f6: e54cc08c0004mvhi
140(%r12),4
  007c91fc: b904002c   lgr 
%r2,%r12[   46.942643]
[   46.942643]007c9200: c0e5e2c0brasl   
%r14,7c5780
[   46.942646]
[   46.942647] Call Trace:
[   46.942650] ([<007c8fe8>] fc_bsg_request_handler+0x200/0x4b0)
[   46.942656] ([<006b8e0a>] __blk_run_queue+0x52/0x68)
[   46.942661] ([<006c549a>] blk_execute_rq_nowait+0xf2/0x110)
[   46.942664] ([<006c557a>] blk_execute_rq+0xa2/0x110)
[   46.942668] ([<006de0ee>] bsg_ioctl+0x1f6/0x268)
[   46.942675] ([<0036ca20>] do_vfs_ioctl+0x680/0x6d8)
[   46.942677] ([<0036caf4>] SyS_ioctl+0x7c/0xb0)
[   46.942685] ([<009a541e>] system_call+0xd6/0x270)
[   46.942687] INFO: lockdep is turned off.
[   46.942688] Last Breaking-Event-Address:
[   46.942692]  [<007c91e4>] fc_bsg_request_handler+0x3fc/0x4b0
[   46.942696]  [   46.942698] Kernel panic - not syncing: Fatal exception: 
panic_on_oops




On 10/12/2016 03:06 PM, Johannes Thumshirn wrote:

Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
helper variables bsg_request and bsg_reply. This will be helpfull  when
transitioning to bsg-lib.

Signed-off-by: Johannes Thumshirn 
---
 drivers/s390/scsi/zfcp_fc.c  |   9 +-
 drivers/scsi/bfa/bfad_bsg.c  |  40 +++---
 

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Johannes Thumshirn]

On Thu, Oct 13, 2016 at 05:15:25PM +0200, Steffen Maier wrote:
> I'm puzzled.
> 
> $ git bisect start fc_bsg master
> Bisecting: 8 revisions left to test after this (roughly 3 steps)
> [005d51510eee6102636d5dbb06310531c5d46151] scsi: fc: implement kref backed
> reference counting
> $ git bisect bad
> Bisecting: 3 revisions left to test after this (roughly 2 steps)
> [bef6da201de1bb81bb4d9511f9a155862efc251f] scsi: Unify interfaces of
> fc_bsg_jobdone and bsg_job_done
> $ git bisect bad
> Bisecting: 1 revision left to test after this (roughly 1 step)
> [3087864ce3d7282f59021245d8a5f83ef1caef18] scsi: don't use
> fc_bsg_job::request and fc_bsg_job::reply directly
> $ git bisect bad
> Bisecting: 0 revisions left to test after this (roughly 0 steps)
> [81aea44720d22d2e0c4a2613ae8b1c256ef6b0cb] scsi: Get rid of struct
> fc_bsg_buffer
> > $ git bisect good
> > 3087864ce3d7282f59021245d8a5f83ef1caef18 is the first bad commit
> > commit 3087864ce3d7282f59021245d8a5f83ef1caef18
> > Author: Johannes Thumshirn 
> > Date:   Wed Oct 12 15:06:28 2016 +0200
> > 
> > scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly
> > 
> > Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
> > helper variables bsg_request and bsg_reply. This will be helpfull  when
> > transitioning to bsg-lib.
> > 
> > Signed-off-by: Johannes Thumshirn 
> > 
> > :04 04 140c4b6829d5cfaec4079716e0795f63f8bc3bd2 
> > 0d9fe225615679550be91fbd9f84c09ab1e280fc M  drivers
> 
> From there (on the reverse bisect path) I get the following Oops,
> except for the full patch set having another stack trace as in my previous
> mail (dying in zfcp code).
> 

[...]

> 
> > @@ -3937,6 +3944,7 @@ fc_bsg_request_handler(struct request_queue *q, 
> > struct Scsi_Host *shost,
> > struct request *req;
> > struct fc_bsg_job *job;
> > enum fc_dispatch_result ret;
> > +   struct fc_bsg_reply *bsg_reply;
> > 
> > if (!get_device(dev))
> > return;
> > @@ -3973,8 +3981,9 @@ fc_bsg_request_handler(struct request_queue *q, 
> > struct Scsi_Host *shost,
> > /* check if we have the msgcode value at least */
> > if (job->request_len < sizeof(uint32_t)) {
> > BUG_ON(job->reply_len < sizeof(uint32_t));
> > -   job->reply->reply_payload_rcv_len = 0;
> > -   job->reply->result = -ENOMSG;
> > +   bsg_reply = job->reply;
> > +   bsg_reply->reply_payload_rcv_len = 0;
> > +   bsg_reply->result = -ENOMSG;
> > job->reply_len = sizeof(uint32_t);
> > fc_bsg_jobdone(job);
> > spin_lock_irq(q->queue_lock);
> > 

Ahm and what exactly can break here? It's just assigning variables. Now
I'm puzzled too.

I'll have to look into it tomorrow.

Byte,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Hannes Reinecke]

On 10/12/2016 03:06 PM, Johannes Thumshirn wrote:
> Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
> helper variables bsg_request and bsg_reply. This will be helpfull  when
> transitioning to bsg-lib.
> 
> Signed-off-by: Johannes Thumshirn 
> ---
>  drivers/s390/scsi/zfcp_fc.c  |   9 +-
>  drivers/scsi/bfa/bfad_bsg.c  |  40 +++---
>  drivers/scsi/ibmvscsi/ibmvfc.c   |  22 ++--
>  drivers/scsi/libfc/fc_lport.c|  23 ++--
>  drivers/scsi/lpfc/lpfc_bsg.c | 194 +---
>  drivers/scsi/qla2xxx/qla_bsg.c   | 264 
> ++-
>  drivers/scsi/qla2xxx/qla_iocb.c  |   5 +-
>  drivers/scsi/qla2xxx/qla_isr.c   |  46 ---
>  drivers/scsi/qla2xxx/qla_mr.c|  10 +-
>  drivers/scsi/scsi_transport_fc.c |  37 +++---
>  10 files changed, 387 insertions(+), 263 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly

[Johannes Thumshirn]

Don't use fc_bsg_job::request and fc_bsg_job::reply directly, but use
helper variables bsg_request and bsg_reply. This will be helpfull  when
transitioning to bsg-lib.

Signed-off-by: Johannes Thumshirn 
---
 drivers/s390/scsi/zfcp_fc.c  |   9 +-
 drivers/scsi/bfa/bfad_bsg.c  |  40 +++---
 drivers/scsi/ibmvscsi/ibmvfc.c   |  22 ++--
 drivers/scsi/libfc/fc_lport.c|  23 ++--
 drivers/scsi/lpfc/lpfc_bsg.c | 194 +---
 drivers/scsi/qla2xxx/qla_bsg.c   | 264 ++-
 drivers/scsi/qla2xxx/qla_iocb.c  |   5 +-
 drivers/scsi/qla2xxx/qla_isr.c   |  46 ---
 drivers/scsi/qla2xxx/qla_mr.c|  10 +-
 drivers/scsi/scsi_transport_fc.c |  37 +++---
 10 files changed, 387 insertions(+), 263 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fc.c b/drivers/s390/scsi/zfcp_fc.c
index 237688a..4c4023f 100644
--- a/drivers/s390/scsi/zfcp_fc.c
+++ b/drivers/s390/scsi/zfcp_fc.c
@@ -900,8 +900,9 @@ static struct zfcp_fc_wka_port *zfcp_fc_job_wka_port(struct 
fc_bsg_job *job)
u32 preamble_word1;
u8 gs_type;
struct zfcp_adapter *adapter;
+   struct fc_bsg_request *bsg_request = job->request;
 
-   preamble_word1 = job->request->rqst_data.r_ct.preamble_word1;
+   preamble_word1 = bsg_request->rqst_data.r_ct.preamble_word1;
gs_type = (preamble_word1 & 0xff00) >> 24;
 
adapter = (struct zfcp_adapter *) job->shost->hostdata[0];
@@ -938,6 +939,7 @@ static int zfcp_fc_exec_els_job(struct fc_bsg_job *job,
 {
struct zfcp_fsf_ct_els *els = job->dd_data;
struct fc_rport *rport = job->rport;
+   struct fc_bsg_request *bsg_request = job->request;
struct zfcp_port *port;
u32 d_id;
 
@@ -949,7 +951,7 @@ static int zfcp_fc_exec_els_job(struct fc_bsg_job *job,
d_id = port->d_id;
put_device(>dev);
} else
-   d_id = ntoh24(job->request->rqst_data.h_els.port_id);
+   d_id = ntoh24(bsg_request->rqst_data.h_els.port_id);
 
els->handler = zfcp_fc_ct_els_job_handler;
return zfcp_fsf_send_els(adapter, d_id, els, job->req->timeout / HZ);
@@ -983,6 +985,7 @@ int zfcp_fc_exec_bsg_job(struct fc_bsg_job *job)
struct Scsi_Host *shost;
struct zfcp_adapter *adapter;
struct zfcp_fsf_ct_els *ct_els = job->dd_data;
+   struct fc_bsg_request *bsg_request = job->request;
 
shost = job->rport ? rport_to_shost(job->rport) : job->shost;
adapter = (struct zfcp_adapter *)shost->hostdata[0];
@@ -994,7 +997,7 @@ int zfcp_fc_exec_bsg_job(struct fc_bsg_job *job)
ct_els->resp = job->reply_payload.sg_list;
ct_els->handler_data = job;
 
-   switch (job->request->msgcode) {
+   switch (bsg_request->msgcode) {
case FC_BSG_RPT_ELS:
case FC_BSG_HST_ELS_NOLOGIN:
return zfcp_fc_exec_els_job(job, adapter);
diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c
index d1ad020..48366d8 100644
--- a/drivers/scsi/bfa/bfad_bsg.c
+++ b/drivers/scsi/bfa/bfad_bsg.c
@@ -3132,7 +3132,9 @@ bfad_iocmd_handler(struct bfad_s *bfad, unsigned int cmd, 
void *iocmd,
 static int
 bfad_im_bsg_vendor_request(struct fc_bsg_job *job)
 {
-   uint32_t vendor_cmd = job->request->rqst_data.h_vendor.vendor_cmd[0];
+   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 =
(struct bfad_im_port_s *) job->shost->hostdata[0];
struct bfad_s *bfad = im_port->bfad;
@@ -3175,8 +3177,8 @@ bfad_im_bsg_vendor_request(struct fc_bsg_job *job)
 
/* Fill the BSG job reply data */
job->reply_len = job->reply_payload.payload_len;
-   job->reply->reply_payload_rcv_len = job->reply_payload.payload_len;
-   job->reply->result = rc;
+   bsg_reply->reply_payload_rcv_len = job->reply_payload.payload_len;
+   bsg_reply->result = rc;
 
job->job_done(job);
return rc;
@@ -3184,9 +3186,9 @@ error:
/* free the command buffer */
kfree(payload_kbuf);
 out:
-   job->reply->result = rc;
+   bsg_reply->result = rc;
job->reply_len = sizeof(uint32_t);
-   job->reply->reply_payload_rcv_len = 0;
+   bsg_reply->reply_payload_rcv_len = 0;
return rc;
 }
 
@@ -3362,18 +3364,20 @@ bfad_im_bsg_els_ct_request(struct fc_bsg_job *job)
struct bfad_fcxp*drv_fcxp;
struct bfa_fcs_lport_s *fcs_port;
struct bfa_fcs_rport_s *fcs_rport;
-   uint32_t command_type = job->request->msgcode;
+   struct fc_bsg_request *bsg_request = bsg_request;
+   struct fc_bsg_reply *bsg_reply = job->reply;
+   uint32_t command_type = bsg_request->msgcode;
unsigned long flags;
struct bfad_buf_info *rsp_buf_info;
void *req_kbuf = NULL, *rsp_kbuf =