linux-kernel - Re: [PATCH] blk: missing add of padded bytes to io completion byte count

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: <47CEA40F.6050903@gmail.com>
Date:	Wed, 05 Mar 2008 22:45:51 +0900
From:	Tejun Heo <htejun@...il.com>
To:	Jens Axboe <jens.axboe@...cle.com>
CC:	Boaz Harrosh <bharrosh@...asas.com>,
	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>,
	Mike Galbraith <efault@....de>,
	James.Bottomley@...senPartnership.com, tomof@....org,
	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] blk: missing add of padded bytes to io completion byte
  count

Hello, Jens, Boaz.

Jens Axboe wrote:
>>>> From: Boaz Harrosh <bharrosh@...asas.com>
>>>> Date: Wed, 5 Mar 2008 12:07:12 +0200
>>>> Subject: [PATCH] blk: missing add of padded bytes to io completion byte count
>>>>
>>>> the commit e97a294ef6938512b655b1abf17656cf2b26f709 was very wrong. This is
>>>> because scsi-ml supports the ability to split a request into smaller chunks,
>>>> in which case scsi_bufflen() is smaller then request length. Then at completion
>>>> time the remainder can be issued as a new scsi command. In that case the above
>>>> commit is a data corruption.

Thanks for catching the stupidity.  Did it actually happen?  PC commands
are not completed in pieces and padding / draining should only happen
for those.  qc->extra_len should be zero where commands can be splitted
for all current cases.

>>> We needed something for -rc4, so it had to be rushed a bit...
>>>
>>>> Also in this fix all users of block layer are taken care of, and not only
>>>> scsi devices.
>>>>
>>>> Signed-off-by: Boaz Harrosh <bharrosh@...asas.com>
>>>> Signed-off-by: Benny Halevy <bhalevy@...asas.com>
>>>> ---
>>>>  block/blk-core.c    |    4 ++++
>>>>  drivers/scsi/scsi.c |    2 +-
>>>>  2 files changed, 5 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 2a438a9..37fcccc 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -1549,6 +1549,9 @@ static int __end_that_request_first(struct request *req, int error,
>>>>  			     nr_bytes >> 9, req->sector);
>>>>  	}
>>>>  
>>>> +	if (nr_bytes >= blk_rq_bytes(req))
>>>> +		nr_bytes += req->extra_len;
>>>> +

This is getting insanely subtle.  Let's say there's PIO driver which
transfer certain sized chunks at a time and completes request partially
after completing each chunk and the driver uses draining to eat up
whatever excess data, which seems like a legit use case to me.  But it
won't work because __end_that_request_first() will terminate when it
reaches reaches the 'true' transfer size.  That's just broken API.  FWIW,

Nacked-by: Tejun Heo <htejun@...il.com>

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