linux-kernel - Re: [PATCH 03/11] block: add rq->resid

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: <4A08C26E.80801@kernel.org>
Date:	Tue, 12 May 2009 09:27:26 +0900
From:	Tejun Heo <tj@...nel.org>
To:	Boaz Harrosh <bharrosh@...asas.com>
CC:	axboe@...nel.dk, linux-kernel@...r.kernel.org, jeff@...zik.org,
	linux-ide@...r.kernel.org, James.Bottomley@...senPartnership.com,
	linux-scsi@...r.kernel.org, bzolnier@...il.com,
	petkovbb@...glemail.com, sshtylyov@...mvista.com,
	mike.miller@...com, Eric.Moore@....com, stern@...land.harvard.edu,
	fujita.tomonori@....ntt.co.jp, zaitcev@...hat.com,
	Geert.Uytterhoeven@...ycom.com, sfr@...b.auug.org.au,
	grant.likely@...retlab.ca, paul.clements@...eleye.com,
	tim@...erelk.net, jeremy@...source.com, adrian@...en.demon.co.uk,
	oakad@...oo.com, dwmw2@...radead.org, schwidefsky@...ibm.com,
	ballabio_dario@....com, davem@...emloft.net, rusty@...tcorp.com.au,
	Markus.Lidel@...dowconnect.com,
	Doug Gilbert <dgilbert@...erlog.com>,
	"Darrick J. Wong" <djwong@...ibm.com>
Subject: Re: [PATCH 03/11] block: add rq->resid_len

Hello, Boaz.

Boaz Harrosh wrote:
> On 05/11/2009 02:48 AM, Tejun Heo wrote:
>> Does resid_len make any sense w/ failed requests?  I think we would be
>> better off with declaring residual count to be undefined on request
>> failure.  Is there any place which depends on it?
>>
>> That said, the value is eventually exported to userland, so it might
>> be better to not change it.  Eh... I don't know.
> 
> When possible, residual should be exact because the residual amount
> is not bounced and might even be zeroed-out for security, as the
> meaning of residual is that these bytes are garbage.

When a request failed, the whole buffer is garbage.  There's no
partial transfer.  There shouldn't be.  I don't think residual count
on request failure means anything.  Also, the 'whenever possible'
doesn't mean much when the issuer can't determine whether the value is
valid or not.  On success, we should guarantee resid count is valid,
on failure, I don't see a way we can.

>>>> index 3da02e4..6605ec9 100644
>>>> --- a/drivers/scsi/libsas/sas_expander.c
>>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>>> @@ -1936,12 +1936,8 @@ int sas_smp_handler(struct Scsi_Host *shost, struct sas_rphy *rphy,
>>>>  			       bio_data(rsp->bio), rsp->data_len);
>>>>  	if (ret > 0) {
>>>>  		/* positive number is the untransferred residual */
>>>> -		rsp->data_len = ret;
>>>> -		req->data_len = 0;
>>>> +		rsp->resid_len = ret;
>>>>  		ret = 0;
>>>> -	} else if (ret == 0) {
>>>> -		rsp->data_len = 0;
>>>> -		req->data_len = 0;
>>>>  	}
>>>>  
>>>>  	return ret;
>>> This is actually a bug fix, as well as a strait conversion
>> Can you elaborate a bit about the bug fix part?
> 
> Nothing big really, just that before (according to the comment), the
> theoretical negative case would be full-residual. and now it is zero
> (untouched).

Yeah, the same thing as above, I suppose.

> I know that in iscsi a negative residual is possible which means
> over-flow. That is: the target had more data to give then the buffer
> had space for. (which is not an error at all)

Hmm... I've never seen negative residual in use.  Is it even defined?

>>> The above is just a case of:
>>>
>>> -		int leftover = (req->hard_nr_sectors << 9);
>>> -
>>> -		if (blk_pc_request(req))
>>> -			leftover = req->data_len;
>>> +		int leftover = blk_rq_bytes();
>>>
>>> Which you separated into to stages, much later right?
>> Aieee.. yeah, that's one stupid misconversion.  That function should
>> just use blk_end_request_all().  Will fix.  Thanks for spotting it.
> 
> Yes, there is a couple of other places that have that with the
> meaning of blk_end_request_all() (Have I commented on one?). Are you
> doing this conversion in these patchset? or this is for a second
> pass?

If you spot some, please go ahead and submit patches.  I don't have
any patch in my queue which hasn't been posted.

Thanks.

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