[PATCH] RPC: add wrapper for svc_reserve to account for checksum

[PATCH] RPC: add wrapper for svc_reserve to account for checksum

[Jeff Layton]

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

This is probably also a problem for other sec= types and for NFSv4. The large
reserved size for NFSv4 compound packets seems to generally paper over the
problem, however.

This patch adds a wrapper for svc_reserve that accounts for the possibility of
a checksum. It also fixes up the appropriate callers of svc_reserve to call
the wrapper. For now, it just uses a hardcoded value that I determined via
testing. That value may need to be revised upward as things change, or we
may want to eventually add a new auth_op that attempts to calculate this
somehow.

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.

Signed-off-by: Jeff Layton <jlayton@redhat.com>

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index f61142a..7d47c16 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -175,7 +175,7 @@ nfsd3_proc_read(struct svc_rqst *rqstp, struct nfsd3_readargs *argp,
 	if (NFSSVC_MAXBLKSIZE < resp->count)
 		resp->count = NFSSVC_MAXBLKSIZE;
 
-	svc_reserve(rqstp, ((1 + NFS3_POST_OP_ATTR_WORDS + 3)<<2) + resp->count +4);
+	svc_reserve_auth(rqstp, ((1 + NFS3_POST_OP_ATTR_WORDS + 3)<<2) + resp->count +4);
 
 	fh_copy(&resp->fh, &argp->fh);
 	nfserr = nfsd_read(rqstp, &resp->fh, NULL,
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 4e06810..21f2ff7 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -154,7 +154,7 @@ nfsd_proc_read(struct svc_rqst *rqstp, struct nfsd_readargs *argp,
 				argp->count);
 		argp->count = NFSSVC_MAXBLKSIZE;
 	}
-	svc_reserve(rqstp, (19<<2) + argp->count + 4);
+	svc_reserve_auth(rqstp, (19<<2) + argp->count + 4);
 
 	resp->count = argp->count;
 	nfserr = nfsd_read(rqstp, fh_copy(&resp->fh, &argp->fh), NULL,
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 94350f5..60f24d6 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -326,4 +326,24 @@ int		   svc_register(struct svc_serv *, int, unsigned short);
 void		   svc_wake_up(struct svc_serv *);
 void		   svc_reserve(struct svc_rqst *rqstp, int space);
 
+/*
+ * When we want to reduce the size of the reserved space in the response
+ * buffer, we need to take into account the size of any checksum data that
+ * may be at the end of the packet. For now, just use a hardcoded value
+ * for each possible authflavor. This will need to be updated when new
+ * encryption types or algorithms are added, or we'll have to come up with
+ * a way to reasonably calculate this on the fly (maybe via a new auth_op).
+ */
+static inline void 
+svc_reserve_auth(struct svc_rqst *rqstp, int space)
+{
+	int			added_space = 0;
+
+	switch(rqstp->rq_authop->flavour) {
+		case RPC_AUTH_GSS:
+			added_space = 56; /* determined empirically */
+	}
+	return svc_reserve(rqstp, space + added_space);
+}
+
 #endif /* SUNRPC_SVC_H */
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 0aab5b5..2d390d1 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -379,7 +379,7 @@ svc_process(struct svc_serv *serv, struct svc_rqst *rqstp)
 	 * better idea of reply size
 	 */
 	if (procp->pc_xdrressize)
-		svc_reserve(rqstp, procp->pc_xdrressize<<2);
+		svc_reserve_auth(rqstp, procp->pc_xdrressize<<2);
 
 	/* Call the function that processes the request. */
 	if (!versp->vs_dispatch) {
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c

Re: [NFS] [PATCH] RPC: add wrapper for svc_reserve to account for checksum

[Neil Brown]

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

Re: [NFS] [PATCH] RPC: add wrapper for svc_reserve to account for checksum

[J. Bruce Fields]

On Tue, May 01, 2007 at 12:14:11PM +1000, Neil Brown wrote:
> 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.

Yep.

> Bruce:  does it look OK to you?

It looks sensible, but it's a little late for me--I'll take another look
at it and run some tests tommorow.

--b.

Re: [NFS] [PATCH] RPC: add wrapper for svc_reserve to account for checksum

[Jeff Layton]

On Tue, May 01, 2007 at 12:14:11PM +1000, Neil Brown wrote:
> 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
> 

Thanks for looking into this, Neil. I wasn't sure how hard a reservation that
was (still working on wrapping my brain around the RPC code). I'll have a look
at the patch you posted. Hopefully that is the problem...

As far as your suggestion to use RPC_MAX_AUTH_SIZE, that sounds like a good
idea to me. I'm all for avoiding "magic" numbers. How does this look? I also
cleaned up the comments as well.

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 9114f11..ed7cbf1 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -399,10 +399,9 @@ char *		   svc_print_addr(struct svc_rqst *, char *, size_t);
 /*
  * When we want to reduce the size of the reserved space in the response
  * buffer, we need to take into account the size of any checksum data that
- * may be at the end of the packet. For now, just use a hardcoded value
- * for each possible authflavor. This will need to be updated when new
- * encryption types or algorithms are added, or we'll have to come up with
- * a way to reasonably calculate this on the fly (maybe via a new auth_op).
+ * may be at the end of the packet. This is difficult to determine exactly
+ * for all cases without actually generating the checksum, so we just use a
+ * static value.
  */
 static inline void 
 svc_reserve_auth(struct svc_rqst *rqstp, int space)
@@ -411,7 +410,7 @@ svc_reserve_auth(struct svc_rqst *rqstp, int space)
 
 	switch(rqstp->rq_authop->flavour) {
 		case RPC_AUTH_GSS:
-			added_space = 56; /* determined empirically */
+			added_space = RPC_MAX_AUTH_SIZE;
 	}
 	return svc_reserve(rqstp, space + added_space);
 }

Re: [NFS] [PATCH] RPC: add wrapper for svc_reserve to account for checksum

[Jeff Layton]

On Tue, May 01, 2007 at 01:01:27PM -0400, Jeff Layton wrote:
> On Tue, May 01, 2007 at 12:14:11PM +1000, Neil Brown wrote:
> > 
> > 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
> > 
> 
> Thanks for looking into this, Neil. I wasn't sure how hard a reservation that
> was (still working on wrapping my brain around the RPC code). I'll have a look
> at the patch you posted. Hopefully that is the problem...
> 
> As far as your suggestion to use RPC_MAX_AUTH_SIZE, that sounds like a good
> idea to me. I'm all for avoiding "magic" numbers. How does this look? I also
> cleaned up the comments as well.
> 

Resending patch with a proper signed-off-by line. Also cc'ing Andrew:

Signed-off-by: Jeff Layton <jlayton@redhat.com>

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 9114f11..ed7cbf1 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -399,10 +399,9 @@ char *		   svc_print_addr(struct svc_rqst *, char *, size_t);
 /*
  * When we want to reduce the size of the reserved space in the response
  * buffer, we need to take into account the size of any checksum data that
- * may be at the end of the packet. For now, just use a hardcoded value
- * for each possible authflavor. This will need to be updated when new
- * encryption types or algorithms are added, or we'll have to come up with
- * a way to reasonably calculate this on the fly (maybe via a new auth_op).
+ * may be at the end of the packet. This is difficult to determine exactly
+ * for all cases without actually generating the checksum, so we just use a
+ * static value.
  */
 static inline void 
 svc_reserve_auth(struct svc_rqst *rqstp, int space)
@@ -411,7 +410,7 @@ svc_reserve_auth(struct svc_rqst *rqstp, int space)
 
 	switch(rqstp->rq_authop->flavour) {
 		case RPC_AUTH_GSS:
-			added_space = 56; /* determined empirically */
+			added_space = RPC_MAX_AUTH_SIZE;
 	}
 	return svc_reserve(rqstp, space + added_space);
 }

Re: [NFS] [PATCH] RPC: add wrapper for svc_reserve to account for checksum

[Neil Brown]

On Tuesday May 1, jlayton@redhat.com wrote:
> 
> Resending patch with a proper signed-off-by line. Also cc'ing Andrew:

Thanks, but it isn't much good to Andrew without a changelog entry,
though probably it should just go in as
  rpc--add-wrapper-for-svc_reserve-to-account-for-checksum-fix

Acked-by: NeilBrown <neilb@suse.de>

(If it doesn't appear in the next -mm, I'll send it all nicely
formated and changeloged).

NeilBrown

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> 
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 9114f11..ed7cbf1 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -399,10 +399,9 @@ char *		   svc_print_addr(struct svc_rqst *, char *, size_t);
>  /*
>   * When we want to reduce the size of the reserved space in the response
>   * buffer, we need to take into account the size of any checksum data that
> - * may be at the end of the packet. For now, just use a hardcoded value
> - * for each possible authflavor. This will need to be updated when new
> - * encryption types or algorithms are added, or we'll have to come up with
> - * a way to reasonably calculate this on the fly (maybe via a new auth_op).
> + * may be at the end of the packet. This is difficult to determine exactly
> + * for all cases without actually generating the checksum, so we just use a
> + * static value.
>   */
>  static inline void 
>  svc_reserve_auth(struct svc_rqst *rqstp, int space)
> @@ -411,7 +410,7 @@ svc_reserve_auth(struct svc_rqst *rqstp, int space)
>  
>  	switch(rqstp->rq_authop->flavour) {
>  		case RPC_AUTH_GSS:
> -			added_space = 56; /* determined empirically */
> +			added_space = RPC_MAX_AUTH_SIZE;
>  	}
>  	return svc_reserve(rqstp, space + added_space);
>  }

Re: [NFS] [PATCH] RPC: add wrapper for svc_reserve to account for checksum

[J. Bruce Fields]

On Mon, Apr 30, 2007 at 11:33:10PM -0400, bfields wrote:
> On Tue, May 01, 2007 at 12:14:11PM +1000, Neil Brown wrote:
> > Bruce:  does it look OK to you?
> 
> It looks sensible, but it's a little late for me--I'll take another look
> at it and run some tests tommorow.

Just to confirm--yep, loks fine.  And I did a quick connectathon run
over krb5i just to make sure.  If someone wants it:

Acked-by: J. Bruce Fields <bfields@citi.umich.edu>

--b.

Re: [NFS] [PATCH] RPC: add wrapper for svc_reserve to account for checksum

[Jeff Layton]

On Tue, May 01, 2007 at 12:14:11PM +1000, Neil Brown wrote:
> 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>
> 

Just got through testing this patch. Unfortunately, this panic seems to much
more difficult for me to reproduce all of a sudden. I saw it quite a few
times at connectathon, but I've tried to get it to occur on fairly recent
kernels today and haven't been able to. Perhaps some other change is papering
over it somehow.

The patch doesn't seem to break anything, but I can't really confirm whether
it fixes these oopses. Still, it seems like a sensible change AFAICT...

-- Jeff