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 <[email protected]>
---
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 <[email protected]>
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
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 <[email protected]>
---
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);
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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
On 31/10/06, Andreas Gruenbacher <[email protected]> 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 <[email protected]>
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
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 <[email protected]>
---
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++])
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++];
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
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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