[PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

[PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

[Jesper Juhl]

In fs/nfsd/nfs2acl.c::nfsaclsvc_encode_getaclres() I see a few issues.

1) At the top of the function we assign to the 'inode' variable by 
dereferencing 'dentry', but further down we test 'dentry' for NULL. So, if 
'dentry' (which is really 'resp->fh.fh_dentry') can be NULL, then either 
we have a potential NULL pointer deref bug or we have a superflous test.

2) In the case of  resp->fh.fh_dentry  != NULL  we have a duplicate 
assignment to 'inode' - just one would do.

3) There are two locations in the function where we may return before we 
use the value of the variable 'w', but we compute it at the very top of the 
function. So in the case where we return early we have wasted a few cycles 
computing a value that was never used.


This patch solves these issues by :

1) Moving the computation of 'w' to just before it is used, after the two 
potential returns.

2) Remove the initial assignment of 'dentry->d_inode' 
(aka resp->fh.fh_dentry->d_inode) to 'inode' so that the assignment only 
happens once and happens after the NULL test.


So we get 3 benefits from this patch : 

1) We avoid a potential NULL pointer deref.

2) We save a few CPU cycles from not computing 'w' in the case of an early 
return as well as saving the assignment to 'inode' in the  dentry == NULL 
case, and there's only a single assignment to 'inode' ever.

3) We save a few bytes of .text in the object file.
    before:
        text    data     bss     dec     hex filename
        2502     204       0    2706     a92 fs/nfsd/nfs2acl.o
    after:
        text    data     bss     dec     hex filename
        2489     204       0    2693     a85 fs/nfsd/nfs2acl.o


Patch is compile tested only since I don't currently have a setup where I 
can meaningfully test it further.

Please comment and/or apply.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 fs/nfsd/nfs2acl.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index e3eca08..d89d63f 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -221,12 +221,10 @@ static int nfsaclsvc_encode_getaclres(st
 		struct nfsd3_getaclres *resp)
 {
 	struct dentry *dentry = resp->fh.fh_dentry;
-	struct inode *inode = dentry->d_inode;
-	int w = nfsacl_size(
-		(resp->mask & NFS_ACL)   ? resp->acl_access  : NULL,
-		(resp->mask & NFS_DFACL) ? resp->acl_default : NULL);
 	struct kvec *head = rqstp->rq_res.head;
+	struct inode *inode;
 	unsigned int base;
+	int w;
 	int n;
 
 	if (dentry == NULL || dentry->d_inode == NULL)
@@ -239,6 +237,9 @@ static int nfsaclsvc_encode_getaclres(st
 		return 0;
 	base = (char *)p - (char *)head->iov_base;
 
+	w = nfsacl_size(
+		(resp->mask & NFS_ACL)   ? resp->acl_access  : NULL,
+		(resp->mask & NFS_DFACL) ? resp->acl_default : NULL);
 	rqstp->rq_res.page_len = w;
 	while (w > 0) {
 		if (!rqstp->rq_respages[rqstp->rq_resused++])


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

[David Rientjes]

On Fri, 27 Oct 2006, Jesper Juhl wrote:

> In fs/nfsd/nfs2acl.c::nfsaclsvc_encode_getaclres() I see a few issues.
> 
> 1) At the top of the function we assign to the 'inode' variable by 
> dereferencing 'dentry', but further down we test 'dentry' for NULL. So, if 
> 'dentry' (which is really 'resp->fh.fh_dentry') can be NULL, then either 
> we have a potential NULL pointer deref bug or we have a superflous test.
> 

resp->fh.fh_dentry cannot be NULL on nfsaclsvc_encode_getaclres so the 
early assignment is appropriate for both *dentry and *inode.  *inode will 
need to be checked for NULL in the conditional, however, and return 0 on 
true.

> 3) There are two locations in the function where we may return before we 
> use the value of the variable 'w', but we compute it at the very top of the 
> function. So in the case where we return early we have wasted a few cycles 
> computing a value that was never used.
> 

w should be an unsigned int.

		David

Re: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

[Jesper Juhl]

On Friday 27 October 2006 23:46, David Rientjes wrote:
> On Fri, 27 Oct 2006, Jesper Juhl wrote:
> 
> > In fs/nfsd/nfs2acl.c::nfsaclsvc_encode_getaclres() I see a few issues.
> > 
> > 1) At the top of the function we assign to the 'inode' variable by 
> > dereferencing 'dentry', but further down we test 'dentry' for NULL. So, if 
> > 'dentry' (which is really 'resp->fh.fh_dentry') can be NULL, then either 
> > we have a potential NULL pointer deref bug or we have a superflous test.
> > 
> 
> resp->fh.fh_dentry cannot be NULL on nfsaclsvc_encode_getaclres so the 
> early assignment is appropriate for both *dentry and *inode.

I didn't convince myself of that on my first scan of the code, that's why 
I opted to keep the NULL check.

> *inode will  
> need to be checked for NULL in the conditional, however, and return 0 on 
> true.
> 
Right, that agrees with my reading as well.

> > 3) There are two locations in the function where we may return before we 
> > use the value of the variable 'w', but we compute it at the very top of the 
> > function. So in the case where we return early we have wasted a few cycles 
> > computing a value that was never used.
> > 
> 
> w should be an unsigned int.
> 
Makes sense.

Thank you for commenting.

Here's a patch, on top of the previous one, to address you comments.
Feedback welcome.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 fs/nfsd/nfs2acl.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index d89d63f..069d7d0 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -222,14 +222,13 @@ static int nfsaclsvc_encode_getaclres(st
 {
 	struct dentry *dentry = resp->fh.fh_dentry;
 	struct kvec *head = rqstp->rq_res.head;
-	struct inode *inode;
+	struct inode *inode = dentry->d_inode;
 	unsigned int base;
-	int w;
+	unsigned int w;
 	int n;
 
-	if (dentry == NULL || dentry->d_inode == NULL)
+	if (!inode)
 		return 0;
-	inode = dentry->d_inode;
 
 	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
 	*p++ = htonl(resp->mask);

Re: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

[Andreas Gruenbacher]

On Saturday 28 October 2006 00:01, Jesper Juhl wrote:
> > > 3) There are two locations in the function where we may return before
> > > we use the value of the variable 'w', but we compute it at the very top
> > > of the function. So in the case where we return early we have wasted a
> > > few cycles computing a value that was never used.

Computing w later in the function is fine.

> > w should be an unsigned int.
>
> Makes sense.

No, this breaks the while loop further below: with an unsigned int, the loop 
counter underflows and wraps.

Please fix this identically in fs/nfsd/nfs2acl.c and fs/nfsd/nfs3acl.c.

Thanks,
Andreas

Re: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

[Jesper Juhl]

On 31/10/06, Andreas Gruenbacher <agruen@suse.de> wrote:
> On Saturday 28 October 2006 00:01, Jesper Juhl wrote:
> > > > 3) There are two locations in the function where we may return before
> > > > we use the value of the variable 'w', but we compute it at the very top
> > > > of the function. So in the case where we return early we have wasted a
> > > > few cycles computing a value that was never used.
>
> Computing w later in the function is fine.
>
> > > w should be an unsigned int.
> >
> > Makes sense.
>
> No, this breaks the while loop further below: with an unsigned int, the loop
> counter underflows and wraps.
>
Whoops. OK.

> Please fix this identically in fs/nfsd/nfs2acl.c and fs/nfsd/nfs3acl.c.
>
Sure thing, expect patches later this evening.

BTW: I posted an add-on patch on top of my first one - apart from the
"make w unsigned" bit, is the rest of that OK?

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

[David Rientjes]

On Tue, 31 Oct 2006, Andreas Gruenbacher wrote:

> > > w should be an unsigned int.
> >
> > Makes sense.
> 
> No, this breaks the while loop further below: with an unsigned int, the loop 
> counter underflows and wraps.
> 

This is not a problem with w being an unsigned int, it's a problem with 
the while loop.  nfsacl_size() returns unsigned int as it should and the 
while loop can be written to respect that since integer division in C 
truncates:

	for (n = w / PAGE_SIZE; n > 0; n--)
		if (!rqstp->rq_respages[rqstp->rq_resused++];

Small optimization for nfs3acl. (was: Re: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.)

[Jesper Juhl]

On Tuesday 31 October 2006 17:26, Andreas Gruenbacher wrote:
> On Saturday 28 October 2006 00:01, Jesper Juhl wrote:
> > > > 3) There are two locations in the function where we may return before
> > > > we use the value of the variable 'w', but we compute it at the very top
> > > > of the function. So in the case where we return early we have wasted a
> > > > few cycles computing a value that was never used.
> 
> Computing w later in the function is fine.
> 
...
> 
> Please fix this identically in fs/nfsd/nfs2acl.c and fs/nfsd/nfs3acl.c.
> 

Here's a patch for nfs3. Hope it's OK.


Saves a few bytes of .text and avoids calculating 'w' in 
nfs3svc_encode_getaclres() in case we return before it's needed.

   text    data     bss     dec     hex filename
   1688     132       0    1820     71c fs/nfsd/nfs3acl.o


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 fs/nfsd/nfs3acl.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
index fcad289..eb1cf22 100644
--- a/fs/nfsd/nfs3acl.c
+++ b/fs/nfsd/nfs3acl.c
@@ -171,11 +171,9 @@ static int nfs3svc_encode_getaclres(stru
 	p = nfs3svc_encode_post_op_attr(rqstp, p, &resp->fh);
 	if (resp->status == 0 && dentry && dentry->d_inode) {
 		struct inode *inode = dentry->d_inode;
-		int w = nfsacl_size(
-			(resp->mask & NFS_ACL)   ? resp->acl_access  : NULL,
-			(resp->mask & NFS_DFACL) ? resp->acl_default : NULL);
 		struct kvec *head = rqstp->rq_res.head;
 		unsigned int base;
+		int w;
 		int n;
 
 		*p++ = htonl(resp->mask);
@@ -183,6 +181,9 @@ static int nfs3svc_encode_getaclres(stru
 			return 0;
 		base = (char *)p - (char *)head->iov_base;
 
+		w = nfsacl_size(
+			(resp->mask & NFS_ACL)   ? resp->acl_access  : NULL,
+			(resp->mask & NFS_DFACL) ? resp->acl_default : NULL);
 		rqstp->rq_res.page_len = w;
 		while (w > 0) {
 			if (!rqstp->rq_respages[rqstp->rq_resused++])

Re: Small optimization for nfs3acl. (was: Re: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.)

[Andreas Gruenbacher]

On Tuesday 31 October 2006 21:39, Jesper Juhl wrote:
> Here's a patch for nfs3. Hope it's OK.

It's obviously correct.

Thanks,
Andreas

Re: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

[Andreas Gruenbacher]

On Tuesday 31 October 2006 21:39, David Rientjes wrote:
> On Tue, 31 Oct 2006, Andreas Gruenbacher wrote:
> > > > w should be an unsigned int.
> > >
> > > Makes sense.
> >
> > No, this breaks the while loop further below: with an unsigned int, the
> > loop counter underflows and wraps.
>
> This is not a problem with w being an unsigned int, it's a problem with
> the while loop.  nfsacl_size() returns unsigned int as it should and the
> while loop can be written to respect that since integer division in C
> truncates:
>
> 	for (n = w / PAGE_SIZE; n > 0; n--)
> 		if (!rqstp->rq_respages[rqstp->rq_resused++];

Assuming that PAGE_SIZE = 4096 and w = 100, the original loop iterates once, 
while your proposed version iterates zero times -- the current code does the 
right thing. So the proposed change is still bad, sorry.

Andreas