Attachment 153945 Details for Bug 237374 – patch -- upstream patch from neil brown

[patch] patch -- upstream patch from neil brown

integ-oops.patch (text/plain), 4.01 KB, created by Jeff Layton on 2007-05-02 13:49:05 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2007-05-02 13:49:05 UTC

Size: 4.01 KB

patch

obsolete

>On Saturday April 21, jlayton@redhat.com wrote:
>> When the kernel calls svc_reserve to downsize the expected size of an RPC
>> reply, it fails to account for the possibility of a checksum at the end of
>> the packet. If a client mounts a NFSv2/3 with sec=krb5i/p, and does I/O then
>> you'll generally see messages similar to this in the server's ring buffer:
>> 
>> RPC request reserved 164 but used 208
>> 
>> While I was never able to verify it, I suspect that this problem is also the
>> root cause of some oopses I've seen under these conditions:
>> 
>> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=227726
>
>I don't think to two are that closely related.
>The space reservation mentioned in the "RPC request reserved ..."
>messages is a fairly soft reservation.  We try to make sure the
>network stack reserves a certain amount of space, then try never to
>start processing a request unless we will be able to reply without
>exceeding that allocation.
>If we get it wrong and do exceed the allocation, the worst that might
>happen is that we might block while writing the reply and maybe have
>to close a connection, or send out an incomplete packet, or something
>like that.  Certainly not an oops.
>
>The extra space needed at the end of the message of integrity
>information which we weren't accounting for properly does seemed to be
>accounted correctly in svcauth_gss_wrap_resp_integ (I'm less sure of
>_wrp_resp_priv, but it is probably right).
>
>Of the two Oops in that BUG, the first seems to be the
>		BUG_ON(resbuf->tail[0].iov_len);
>in svcauth_gss_wrap_resp_integ (assuming code in 2.6.9-44.ELxenU is at
>least vaguely similar to current -mm).
>
>I don't think this BUG_ON is correct.  If a readdir finds zero entries,
>then will be some trailer information in the 'tail', but page_len will
>be 0.  I think the following patch is correct and could fix that.
>Bruce:  does it look OK to you?
>
>
>The second oops is harder to interpret.
>I looks like tcp_send_page is getting a NULL page, though it could be
>getting a length > PAGE_SIZE which might have the same effect.
>
>It reminds me of
>  http://bugzilla.kernel.org/show_bug.cgi?id=7795
>but I'm not convinced it is the same.
>
>> 
>> Unfortunately, there doesn't seem to be a good way to reliably determine the
>> expected checksum length prior to actually calculating it, particularly with
>> schemes like spkm3.
>
>Yes, that asn1 encoding does seem rather awkward.
>
>Maybe we should just use RPC_MAX_AUTH_SIZE like other bits of GSS code
>does.  There is no great cost in reserving too much space - it just
>might slow things down a little when tight on memory.
>What would you think of submitting an incremental patch which replaces
>the "56" with "RPC_MAX_AUTH_SIZE" ?
>
>Thanks (and sorry for the delay).
>NeilBrown
>
>--
>Simplify (and fix) adding of integrity data to end of rpc message
>
>There is no value in a special case for adding integ data to the
>'head' part of the reply in the case where there is no body.
>Always put it in the 'tail', placing that after the current head if it
>doesn't have a place yet.
>As readdir can potentially return an empty body and a non-empty tail,
>the BUG_ON can fire.
>
>Signed-off-by: Neil Brown <neilb@suse.de>
>
>### Diffstat output
> ./net/sunrpc/auth_gss/svcauth_gss.c |    8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
>diff .prev/net/sunrpc/auth_gss/svcauth_gss.c ./net/sunrpc/auth_gss/svcauth_gss.c
>--- .prev/net/sunrpc/auth_gss/svcauth_gss.c	2007-05-01 11:42:31.000000000 +1000
>+++ ./net/sunrpc/auth_gss/svcauth_gss.c	2007-05-01 11:42:55.000000000 +1000
>@@ -1210,13 +1210,7 @@ svcauth_gss_wrap_resp_integ(struct svc_r
> 	if (xdr_buf_subsegment(resbuf, &integ_buf, integ_offset,
> 				integ_len))
> 		BUG();
>-	if (resbuf->page_len == 0
>-			&& resbuf->head[0].iov_len + RPC_MAX_AUTH_SIZE
>-			< PAGE_SIZE) {
>-		BUG_ON(resbuf->tail[0].iov_len);
>-		/* Use head for everything */
>-		resv = &resbuf->head[0];
>-	} else if (resbuf->tail[0].iov_base == NULL) {
>+	if (resbuf->tail[0].iov_base == NULL) {
> 		if (resbuf->head[0].iov_len + RPC_MAX_AUTH_SIZE > PAGE_SIZE)
> 			goto out_err;
> 		resbuf->tail[0].iov_base = resbuf->head[0].iov_base
>

Actions: View | Diff

Attachments on bug 237374: 153272 | 153945