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: <4A093C54.2060706@kernel.org>
Date:	Tue, 12 May 2009 18:07:32 +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:
>> When a request failed, the whole buffer is garbage.
> 
> ret is the transferred size, right? I don't see any check for  
> success/failure in below code.
> 
>> There's no
>> partial transfer.  There shouldn't be.  I don't think residual count
>> on request failure means anything. 
> 
> That's not true, there are many cases when transfer failed eventually
> but some bytes are valid. Even the simple read/write case. Imagine a 
> very large transfer with last sector encounter a "bad sector". that can
> be critical, (trying to rescue a disk). And many other examples.

No, it doesn't work like that.  As I wrote before, those partial
transfers are returned to the issuer as partial _SUCCESS_ not partial
failure.  ie. it's successful request w/ positive residual count not
the other way around.  The error is communicated to the issuer when
the leftover is re-issued.  There's no such thing as partial failure.

Please consider write failing after successfully writing certain
number of bytes.  A failed write command MUST NOT cause any actual
write on the device.

Residual count is not about how many bytes have been transferred.  The
number of transferred bytes itself doesn't mean a thing because it
changes depending on which transfer protocol is used regardless of
where the actual failure is.  Residual count is about how many bytes
have been actually produced or consumed and when a command fails none
should have been.

>> 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.
>>
> 
> Code is as strong as it's weakest link, right? If lower
> driver/firmware is brain-dead, what can we do? But why give up where
> you can do better?
>
> The scsi standard is very clear about what every one should do with
> the residual and what it means at every stage, everyone should do
> his part. Here at the middle layer we need to correctly translate
> what lower level returned and pass it up the chain.
> 
> Must stacks are amateuristic in regard to error handling but some
> are not, what
>
> should we strive for, if we can? 

No it's not about being good or bad.  It's about being plain
undefined.  There is no value in setting an inherently undefined value
to certain values in certain corner cases.

>>> 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?
> 
> It is defined, as I explained before. But yes no one uses it in Kernel.
> The "good" low-level drivers fix it up by setting resid to zero, in that
> case. (other wise the upper layers might crash)

If the upper layer might crash if the value is negative, I think it's
quite understandable to call negative values to be undefined.  Or
should we call it unsupported?

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/