linux-kernel - Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

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: <9ea470500903050553m6ab9dfa5hab340213388a4c67@mail.gmail.com>
Date:	Thu, 5 Mar 2009 14:53:28 +0100
From:	Borislav Petkov <petkovbb@...glemail.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] ide-{cd,floppy}: do not map all cmds to an sg

Hi,

On Thu, Mar 5, 2009 at 1:12 PM, Bartlomiej Zolnierkiewicz
<bzolnier@...il.com> wrote:
>
> Hi,
>
> On Wednesday 04 March 2009, Borislav Petkov wrote:
>> ... since some do not transfer any data from the drive and
>> rq->buffer is unset leading to an OOPS when checking VM translations
>> (CONFIG_DEBUG_VIRTUAL).
>>
>> Tested-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
>> Signed-off-by: Borislav Petkov <petkovbb@...il.com>
>
> Thanks for fixing it but I worry that the patch is not entirely correct
> (why o why did you have to throw in unrelated changes...?)

What unrelated changes? This is the ide-cd fix for when mapping a NULL
rq->buffer pointer to an sg. If you mean the [2/3] patch, I had to
remove the partial completions for ide-floppy because it was OOPSing on NULL
rq-pointer in the interrupt handler. I sent you a pretty detailed OOPS along
with backtracking to the offending code line, remember? Also, for reasons of
bisectability.

> also ideally there should have been two patches:
> - minimal bugfix for the buggy patch
> - unification/cleanup of PIO sg setup
>
>> ---
>>  drivers/ide/ide-atapi.c  |    9 ++++++++-
>>  drivers/ide/ide-cd.c     |    4 ----
>>  drivers/ide/ide-floppy.c |    4 ----
>>  3 files changed, 8 insertions(+), 9 deletions(-)
>>

..

>> diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
>> index 091d414..106cacb 100644
>> --- a/drivers/ide/ide-cd.c
>> +++ b/drivers/ide/ide-cd.c
>> @@ -916,10 +916,6 @@ static ide_startstop_t ide_cd_do_request(ide_drive_t *drive, struct request *rq,
>>
>>       cmd.rq = rq;
>>
>> -     ide_init_sg_cmd(&cmd,
>> -             blk_fs_request(rq) ? (rq->nr_sectors << 9) : rq->data_len);
>> -     ide_map_sg(drive, &cmd);
>> -
>>       return ide_issue_pc(drive, &cmd);
>>  out_end:
>>       nsectors = rq->hard_nr_sectors;
>> diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
>> index 2f8f453..b91deef 100644
>> --- a/drivers/ide/ide-floppy.c
>> +++ b/drivers/ide/ide-floppy.c
>> @@ -281,10 +281,6 @@ static ide_startstop_t ide_floppy_do_request(ide_drive_t *drive,
>>               cmd.tf_flags |= IDE_TFLAG_WRITE;
>>
>>       cmd.rq = rq;
>> -
>> -     ide_init_sg_cmd(&cmd, pc->req_xfer);
>
> There was a reason for using ->req_xfer.

Yeah, this is also one of those painful places where I cringe everytime I have
to look at. We finally have to sit down and decide which is which; sometimes
pc->req_xfer _is_ rq->data_len (idefloppy_blockpc_cmd()) and sometimes obviously
not. I'll take a look at that whole mess next and try to come up with something
more clean.

> I don't see where rq->data_len is set for REQ_TYPE_SPECIAL requests
> (ide_queue_pc_tail() and friends)?

me neither :(.

>> -     ide_map_sg(drive, &cmd);
>> -
>>       pc->rq = rq;
>>
>>       return ide_floppy_issue_pc(drive, &cmd, pc);

Man, this sucks!

-- 
Regards/Gruss,
Boris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/