lkml.org 
[lkml]   [2016]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 02/16] scsi: don't use fc_bsg_job::request and fc_bsg_job::reply directly
From
Date


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 <jthumshirn@suse.de>
>>> 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 <jthumshirn@suse.de>
>>>
>>> :040000 040000 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

\
 
 \ /
  Last update: 2016-10-28 11:55    [W:0.172 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site