On Mon, Apr 14, 2008 at 03:43:12PM -0400, Chuck Lever wrote: > On Apr 14, 2008, at 1:48 PM, J. Bruce Fields wrote: >> On Mon, Apr 14, 2008 at 12:27:08PM -0400, Chuck Lever wrote: >>> Paranoia: Ensure a negative error value return from kernel_sendpage >>> never >>> matches a large buffer length. >> >> That is a little paranoid. Absent an argument for exactly what sort >> of >> bug could allow us to reach this point with the head iov_len in at >> least >> the gigabytes, I'm inclined to leave this alone for simplicity's >> sake.... > > I would like to document that we have noticed the mixed sign comparison > there, and that it has been made entirely safe. In certain cases, the > code as it stands does not do what it looks like it does. > > I'm not recommending these changes to be pedantic, nor am I doing this > for my own health. When we leave checks like this out, what we have is > "clever" nondeterministic code rather than safe code. OK, I can buy the argument that it's a little clever and non-idiomatic to skip the explicit check for the error case. > This is simply > good software engineering, but every one of these I've proposed has been > argued over and often rejected. > > We know that humans write this code, and that humans make mistakes. We > should be doing everything we can to make this code less vulnerable to > coding mistakes. This is not about _fixing_ bugs, it's about > _preventing_ them. I understand that, but for me there's a problem of documentation. It helps me, as I read the code, if I can distinguish between failures that our code is designed to handle (a buggy or malicious nfs client, a failed disk, whatever) from failures that it's not designed to handle (memory corruption). So for the latter, my knee-jerk reaction is "why isn't that a BUG_ON (or at least a WARN_ON)?" But admittedly in this particular case, that's not really a problem--I can't really see thinking "hey, is this code suggesting that it's possible for iov_len to be greater than 2^31 at this point?". --b. > >>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> >>> --- >>> >>> net/sunrpc/svcsock.c | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c >>> index 6d4162b..a8ae279 100644 >>> --- a/net/sunrpc/svcsock.c >>> +++ b/net/sunrpc/svcsock.c >>> @@ -200,7 +200,7 @@ static int svc_sendto(struct svc_rqst *rqstp, >>> struct xdr_buf *xdr) >>> flags = 0; >>> len = kernel_sendpage(sock, rqstp->rq_respages[0], 0, >>> xdr->head[0].iov_len, flags); >>> - if (len != xdr->head[0].iov_len) >>> + if (len < 0 || len != xdr->head[0].iov_len) >>> goto out; >>> slen -= xdr->head[0].iov_len; >>> if (slen == 0) >>> > > -- > Chuck Lever > chuck[dot]lever[at]oracle[dot]com > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
