linux-kernel - Re: [PATCH] block: fix residual byte count handling

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]

Date:	Tue, 04 Mar 2008 20:26:31 +0200
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	Tejun Heo <htejun@...il.com>
CC:	FUJITA Tomonori <tomof@....org>, efault@....de,
	jens.axboe@...cle.com, fujita.tomonori@....ntt.co.jp,
	James.Bottomley@...senPartnership.com, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org, linux-ide@...r.kernel.org,
	linux-scsi@...r.kernel.org, jgarzik@...ox.com, bzolnier@...il.com
Subject: Re: [PATCH] block: fix residual byte count handling

On Tue, Mar 04 2008 at 18:42 +0200, Tejun Heo <htejun@...il.com> wrote:
> Tejun Heo wrote:
>> Tejun Heo wrote:
>>> FUJITA Tomonori wrote:
>>>>> Aiee... device going down after timing out on READ_DISC_INFO.  That's
>>>>> gruesome.  Can you please try the other patches?
>>>> Tejun, I thought that libata needs a fix for sum(sg) != rq->data_len. No?
>>> The extra_len you added to qc->nbytes should be it.  The only other
>>> place to pay attention is the ATAPI transfer chunk size and your patch
>>> seems to get it right.
>>>
>>>> Now Jens' git tree should work with all the non libata stuff, ide,
>>>> firewire, bsg, etc. But I'm not sure about libata.
>>> With the second patch, all others should be fine no matter what.  I'll
>>> go check libata part again.
>> I can reproduce the problem here and it's very weird.  I'll report back
>> when I know more.
> 
> Okay, I got it.  Heh, it turns out SCSI and/or block layer is not
> ready for rq->data_len != sum(sg).  When adjusted command completes,
> SCSI midlayer completes the command with rq->data_len for PC commands
> which eventually ends up in __end_that_request_first().  As there are
> extra sg area left after completing rq->data_len, blk layer says so to
> SCSI layer and SCSI layer retries the command only with the appended
> area.
> 
> The following patch gets the writing going.  I really think it's a
> serious mistake to break rq->data_len == sum(sg).  If we break
> rq->data_len == requested size, the worst bugs are giving wrong size
> when issuing commands to application layer of devices which is
> relatively easy to spot and not all that command anyway.  Breaking
> rq->data_len == sum(sg), bugs will be in internal mechanics, DMA
> engine programming and transport layer.  Oh well...
> 
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index fecba05..32439ac 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -757,7 +757,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
>  				"Notifying upper driver of completion "
>  				"(result %x)\n", cmd->result));
>  
> -	good_bytes = scsi_bufflen(cmd);
> +	good_bytes = scsi_bufflen(cmd) + cmd->request->data_len;

Are you sure? is it not:
+	good_bytes = scsi_bufflen(cmd) + cmd->request->extra_len

>          if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
>  		drv = scsi_cmd_to_driver(cmd);
>  		if (drv->done)
> 
> 

I hate this patch. I wish you could maybe take the extra_len into 
account inside blk_end_request. The padding should be transparent
to all concerned but the requesting LLD and the internals of the
block layer. If block layer added padding it should take that into
account on completion. My $0.2.

Boaz
 
--
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/