* [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]
[not found] <451962BD.6050909@oracle.com>
@ 2006-09-26 17:36 ` Chuck Lever
2006-09-26 18:23 ` J. Bruce Fields
2006-09-26 18:55 ` Trond Myklebust
0 siblings, 2 replies; 6+ messages in thread
From: Chuck Lever @ 2006-09-26 17:36 UTC (permalink / raw)
To: Eric Sesterhenn / Snakebyte; +Cc: Linux NFS Mailing List
Hi Eric-
I've checked through these reports.
733 and 1110 are likely the same problem. You should report these
two, 847, and 1034 as open bugs on the NFSv4 bugzilla site, which
appears to be down at the moment (http://linux-nfs.org, where are
you?)
I have a patch for 1372+1373, and a patch for the error handling bug
you reported earlier today, both of which I will send to Trond.
---------- Forwarded message ----------
From: Eric Sesterhenn / Snakebyte <snakebyte@gmx.de>
To: Chuck Lever <chuck.lever@oracle.com>
Date: Tue, 26 Sep 2006 15:30:37 +0200
Subject: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c
* Chuck Lever (chuck.lever@oracle.com) wrote:
> While some complain that coverity finds only nits, I think coverity is
> useful in providing a nudge to re-examine your code in a fresh light.
>
> I don't have a coverity id at the moment. Can you forward the other
> relevant issues?
Here we go, just noticed that the two issues in fs/nfsd also got
listed with the fs/nfs stuff, so there are just 6 warnings.
In case you need further output, just let me know,
where [model] is included, the checker can output where
it guesses that the called function dereferences something.
thanks for checking this,
Eric
#cid 1373 base/src/linux-2.6/fs/nfs/inode.c
568 __nfs_revalidate_inode(struct nfs_server *server, struct inode
*inode)
569 {
570 int status = -ESTALE;
571 struct nfs_fattr fattr;
572 struct nfs_inode *nfsi = NFS_I(inode);
573
Event deref_ptr: Directly dereferenced pointer "inode"
Also see events: [check_after_deref]
At conditional (1): "0" taking false path
574 dfprintk(PAGECACHE, "NFS: revalidating (%s/%Ld)\n",
575 inode->i_sb->s_id, (long
long)NFS_FILEID(inode));
576
577 nfs_inc_stats(inode, NFSIOS_INODEREVALIDATE);
578 lock_kernel();
Event check_after_deref: Pointer "inode" dereferenced before NULL check
Also see events: [deref_ptr]
579 if (!inode || is_bad_inode(inode))
580 goto out_nowait
----------------------snip------------------------------------
#cid 1372 base/src/linux-2.6/fs/nfs/inode.c
568 __nfs_revalidate_inode(struct nfs_server *server, struct inode
*inode)
569 {
570 int status = -ESTALE;
571 struct nfs_fattr fattr;
572 struct nfs_inode *nfsi = NFS_I(inode);
573
574 dfprintk(PAGECACHE, "NFS: revalidating (%s/%Ld)\n",
575 inode->i_sb->s_id, (long
long)NFS_FILEID(inode));
576
Event deref_ptr_in_call: Dereferences pointer "inode" [model]
Also see events: [check_after_deref]
577 nfs_inc_stats(inode, NFSIOS_INODEREVALIDATE);
578 lock_kernel();
Event check_after_deref: Pointer "inode" dereferenced before NULL check
Also see events: [deref_ptr_in_call]
579 if (!inode || is_bad_inode(inode))
580 goto out_nowait;
----------------------snip------------------------------------
#cid 1110 base/src/linux-2.6/fs/nfsd/nfs4acl.c
374 nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl, struct posix_acl
**pacl,
375 struct posix_acl **dpacl, unsigned int flags)
376 {
377 struct nfs4_acl *dacl;
378 int error = -ENOMEM;
379
Event deref_ptr: Directly dereferenced pointer "pacl"
Also see events: [check_after_deref][check_after_deref]
380 *pacl = NULL;
381 *dpacl = NULL;
382
383 dacl = nfs4_acl_new();
At conditional (1): "dacl == 0" taking false path
384 if (dacl == NULL)
385 goto out;
386
387 error = nfs4_acl_split(acl, dacl);
At conditional (2): "error < 0" taking false path
388 if (error < 0)
389 goto out_acl;
390
Event check_after_deref: Pointer "pacl" dereferenced before NULL check
Also see events: [deref_ptr][check_after_deref]
391 if (pacl != NULL) {
392 if (acl->naces == 0) {
393 error = -ENODATA;
394 goto try_dpacl;
395 }
----------------------snip------------------------------------
#cid 1034 base/src/linux-2.6/fs/nfs/callback_xdr.c
356 static unsigned process_op(struct svc_rqst *rqstp,
357 struct xdr_stream *xdr_in, void *argp,
358 struct xdr_stream *xdr_out, void *resp)
359 {
360 struct callback_op *op = &callback_ops[0];
361 unsigned int op_nr = OP_CB_ILLEGAL;
362 unsigned int status = 0;
363 long maxlen;
364 unsigned res;
365
366 dprintk("%s: start\n", __FUNCTION__);
367 status = decode_op_hdr(xdr_in, &op_nr);
368 if (likely(status == 0)) {
369 switch (op_nr) {
370 case OP_CB_GETATTR:
371 case OP_CB_RECALL:
Event ptr_assign: Pointer "op" assigned address of static buffer pointer
"(&callback_ops + (op_nr * 16))" of size 80 and offset 160704
Also see events: [overrun-local][overrun-local]
372 op = &callback_ops[op_nr];
373 break;
374 default:
375 op_nr = OP_CB_ILLEGAL;
376 op = &callback_ops[0];
377 status =
htonl(NFS4ERR_OP_ILLEGAL);
378 }
379 }
380
381 maxlen = xdr_out->end - xdr_out->p;
At conditional (1): "maxlen > 0" taking true path
At conditional (2): "maxlen < 4096" taking true path
382 if (maxlen > 0 && maxlen < PAGE_SIZE) {
Event overrun-local: Overrun of static array of size 80 at position
160704 by accessing with pointer alias "op"
Event overrun-local: NOTE: These bugs are often difficult to see at
first glance. Coverity Prevent recommends a close inspection of the
events leading to this overrun.
Also see events: [ptr_assign][overrun-local]
At conditional (3): "status == 0" taking true path
383 if (likely(status == 0 && op->decode_args !=
NULL))
384 status = op->decode_args(rqstp, xdr_in,
argp);
385 if (likely(status == 0 && op->process_op !=
NULL))
386 status = op->process_op(argp, resp);
387 } else
388 status = htonl(NFS4ERR_RESOURCE);
389
390 res = encode_op_hdr(xdr_out, op_nr, status);
391 if (status == 0)
392 status = res;
393 if (op->encode_res != NULL && status == 0)
394 status = op->encode_res(rqstp, xdr_out, resp);
395 dprintk("%s: done, status = %d\n", __FUNCTION__,
status);
396 return status;
397 }
----------------------snip------------------------------------
#cid 847 base/src/linux-2.6/fs/nfs/nfs4proc.c
1296 nfs4_open_revalidate(struct inode *dir, struct dentry *dentry,
int openflags, struct nameidata *nd)
1297 {
1298 struct rpc_cred *cred;
1299 struct nfs4_state *state;
1300
1301 cred = rpcauth_lookupcred(NFS_CLIENT(dir)->cl_auth, 0);
1302 if (IS_ERR(cred))
1303 return PTR_ERR(cred);
Event deref_ptr_in_call: Dereferences pointer "(dentry)->d_inode"
[model]
Also see events: [check_after_deref]
1304 state = nfs4_open_delegated(dentry->d_inode, openflags,
cred);
At conditional (1): "IS_ERR != 0" taking true path
1305 if (IS_ERR(state))
1306 state = nfs4_do_open(dir, dentry, openflags,
NULL, cred);
1307 put_rpccred(cred);
At conditional (2): "IS_ERR != 0" taking true path
1308 if (IS_ERR(state)) {
1309 switch (PTR_ERR(state)) {
1310 case -EPERM:
1311 case -EACCES:
1312 case -EDQUOT:
1313 case -ENOSPC:
1314 case -EROFS:
1315 lookup_instantiate_filp(nd,
(struct dentry *)state, NULL);
1316 return 1;
At conditional (3): "PTR_ERR == -2" taking true path
1317 case -ENOENT:
Event check_after_deref: Pointer "(dentry)->d_inode" dereferenced before
NULL check
Also see events: [deref_ptr_in_call]
1318 if (dentry->d_inode == NULL)
1319 return 1;
1320 }
1321 goto out_drop
----------------------snip------------------------------------
#cid 733 base/src/linux-2.6/fs/nfsd/nfs4acl.c
374 nfs4_acl_nfsv4_to_posix(struct nfs4_acl *acl, struct posix_acl
**pacl,
375 struct posix_acl **dpacl, unsigned int flags)
376 {
377 struct nfs4_acl *dacl;
378 int error = -ENOMEM;
379
380 *pacl = NULL;
Event deref_ptr: Directly dereferenced pointer "dpacl"
Also see events: [check_after_deref]
381 *dpacl = NULL;
382
383 dacl = nfs4_acl_new();
At conditional (1): "dacl == 0" taking false path
384 if (dacl == NULL)
385 goto out;
386
387 error = nfs4_acl_split(acl, dacl);
At conditional (2): "error < 0" taking false path
388 if (error < 0)
389 goto out_acl;
390
At conditional (3): "pacl != 0" taking true path
391 if (pacl != NULL) {
At conditional (4): "(acl)->naces == 0" taking true path
392 if (acl->naces == 0) {
393 error = -ENODATA;
394 goto try_dpacl;
395 }
396
397 *pacl = _nfsv4_to_posix_one(acl, flags);
398 if (IS_ERR(*pacl)) {
399 error = PTR_ERR(*pacl);
400 *pacl = NULL;
401 goto out_acl;
402 }
403 }
404
405 try_dpacl:
Event check_after_deref: Pointer "dpacl" dereferenced before NULL check
Also see events: [deref_ptr]
406 if (dpacl != NULL) {
407 if (dacl->naces == 0) {
408 if (pacl == NULL || *pacl == NULL)
409 error = -ENODATA;
410 goto out_acl;
411 }
--
"We who cut mere stones must always be envisioning cathedrals"
-- Quarry worker's creed
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]
2006-09-26 17:36 ` [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c] Chuck Lever
@ 2006-09-26 18:23 ` J. Bruce Fields
2006-09-26 18:55 ` Trond Myklebust
1 sibling, 0 replies; 6+ messages in thread
From: J. Bruce Fields @ 2006-09-26 18:23 UTC (permalink / raw)
To: Chuck Lever; +Cc: Eric Sesterhenn / Snakebyte, Linux NFS Mailing List
On Tue, Sep 26, 2006 at 01:36:02PM -0400, Chuck Lever wrote:
> (http://linux-nfs.org, where are you?)
There was a power outage, which Trond and I dealt with by heroically
going out to lunch.
It should be better now.
--b.
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]
2006-09-26 17:36 ` [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c] Chuck Lever
2006-09-26 18:23 ` J. Bruce Fields
@ 2006-09-26 18:55 ` Trond Myklebust
2006-09-26 19:37 ` Chuck Lever
1 sibling, 1 reply; 6+ messages in thread
From: Trond Myklebust @ 2006-09-26 18:55 UTC (permalink / raw)
To: Chuck Lever; +Cc: Eric Sesterhenn / Snakebyte, Linux NFS Mailing List
On Tue, 2006-09-26 at 13:36 -0400, Chuck Lever wrote:
> Hi Eric-
>
> I've checked through these reports.
>
> 733 and 1110 are likely the same problem. You should report these
> two, 847, and 1034 as open bugs on the NFSv4 bugzilla site, which
> appears to be down at the moment (http://linux-nfs.org, where are
> you?)
>
> I have a patch for 1372+1373, and a patch for the error handling bug
> you reported earlier today, both of which I will send to Trond.
1372 and 1373 don't need fixing. Fix coverity instead.
> ---------- Forwarded message ----------
> From: Eric Sesterhenn / Snakebyte <snakebyte@gmx.de>
> To: Chuck Lever <chuck.lever@oracle.com>
> Date: Tue, 26 Sep 2006 15:30:37 +0200
> Subject: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c
> * Chuck Lever (chuck.lever@oracle.com) wrote:
> > While some complain that coverity finds only nits, I think coverity is
> > useful in providing a nudge to re-examine your code in a fresh light.
> >
> > I don't have a coverity id at the moment. Can you forward the other
> > relevant issues?
>
> Here we go, just noticed that the two issues in fs/nfsd also got
> listed with the fs/nfs stuff, so there are just 6 warnings.
> In case you need further output, just let me know,
> where [model] is included, the checker can output where
> it guesses that the called function dereferences something.
>
> thanks for checking this,
> Eric
>
> #cid 1373 base/src/linux-2.6/fs/nfs/inode.c
>
> 568 __nfs_revalidate_inode(struct nfs_server *server, struct inode
> *inode)
> 569 {
> 570 int status = -ESTALE;
> 571 struct nfs_fattr fattr;
> 572 struct nfs_inode *nfsi = NFS_I(inode);
> 573
>
> Event deref_ptr: Directly dereferenced pointer "inode"
> Also see events: [check_after_deref]
> At conditional (1): "0" taking false path
>
> 574 dfprintk(PAGECACHE, "NFS: revalidating (%s/%Ld)\n",
> 575 inode->i_sb->s_id, (long
> long)NFS_FILEID(inode));
> 576
> 577 nfs_inc_stats(inode, NFSIOS_INODEREVALIDATE);
> 578 lock_kernel();
>
> Event check_after_deref: Pointer "inode" dereferenced before NULL check
> Also see events: [deref_ptr]
>
> 579 if (!inode || is_bad_inode(inode))
> 580 goto out_nowait
>
If _ever_ __nfs_revalidate_inode() is called with a null inode pointer
value, then we definitely want to Oops. It should not be possible.
> ----------------------snip------------------------------------
> #cid 1372 base/src/linux-2.6/fs/nfs/inode.c
>
> 568 __nfs_revalidate_inode(struct nfs_server *server, struct inode
> *inode)
> 569 {
> 570 int status = -ESTALE;
> 571 struct nfs_fattr fattr;
> 572 struct nfs_inode *nfsi = NFS_I(inode);
> 573
> 574 dfprintk(PAGECACHE, "NFS: revalidating (%s/%Ld)\n",
> 575 inode->i_sb->s_id, (long
> long)NFS_FILEID(inode));
> 576
>
> Event deref_ptr_in_call: Dereferences pointer "inode" [model]
> Also see events: [check_after_deref]
Ditto.
> 577 nfs_inc_stats(inode, NFSIOS_INODEREVALIDATE);
> 578 lock_kernel();
>
> Event check_after_deref: Pointer "inode" dereferenced before NULL check
> Also see events: [deref_ptr_in_call]
>
> 579 if (!inode || is_bad_inode(inode))
> 580 goto out_nowait;
Ditto.
> ----------------------snip------------------------------------
> #cid 1034 base/src/linux-2.6/fs/nfs/callback_xdr.c
>
> 356 static unsigned process_op(struct svc_rqst *rqstp,
> 357 struct xdr_stream *xdr_in, void *argp,
> 358 struct xdr_stream *xdr_out, void *resp)
> 359 {
> 360 struct callback_op *op = &callback_ops[0];
> 361 unsigned int op_nr = OP_CB_ILLEGAL;
> 362 unsigned int status = 0;
> 363 long maxlen;
> 364 unsigned res;
> 365
> 366 dprintk("%s: start\n", __FUNCTION__);
> 367 status = decode_op_hdr(xdr_in, &op_nr);
> 368 if (likely(status == 0)) {
> 369 switch (op_nr) {
> 370 case OP_CB_GETATTR:
> 371 case OP_CB_RECALL:
>
> Event ptr_assign: Pointer "op" assigned address of static buffer pointer
> "(&callback_ops + (op_nr * 16))" of size 80 and offset 160704
> Also see events: [overrun-local][overrun-local]
Broken Coverity. The code explicitly limits the values of op to
callback_ops[OP_CB_GETATTR] and callback_ops[OP_CB_RECALL]. Anything
else is mapped to callback_ops[0].
> 372 op = &callback_ops[op_nr];
> 373 break;
> 374 default:
> 375 op_nr = OP_CB_ILLEGAL;
> 376 op = &callback_ops[0];
> 377 status =
> htonl(NFS4ERR_OP_ILLEGAL);
> 378 }
> 379 }
> 380
> 381 maxlen = xdr_out->end - xdr_out->p;
>
> At conditional (1): "maxlen > 0" taking true path
> At conditional (2): "maxlen < 4096" taking true path
>
> 382 if (maxlen > 0 && maxlen < PAGE_SIZE) {
>
> Event overrun-local: Overrun of static array of size 80 at position
> 160704 by accessing with pointer alias "op"
> Event overrun-local: NOTE: These bugs are often difficult to see at
> first glance. Coverity Prevent recommends a close inspection of the
> events leading to this overrun.
> Also see events: [ptr_assign][overrun-local]
> At conditional (3): "status == 0" taking true path
Junk. See comment above.
> 383 if (likely(status == 0 && op->decode_args !=
> NULL))
> 384 status = op->decode_args(rqstp, xdr_in,
> argp);
> 385 if (likely(status == 0 && op->process_op !=
> NULL))
> 386 status = op->process_op(argp, resp);
> 387 } else
> 388 status = htonl(NFS4ERR_RESOURCE);
> 389
> 390 res = encode_op_hdr(xdr_out, op_nr, status);
> 391 if (status == 0)
> 392 status = res;
> 393 if (op->encode_res != NULL && status == 0)
> 394 status = op->encode_res(rqstp, xdr_out, resp);
> 395 dprintk("%s: done, status = %d\n", __FUNCTION__,
> status);
> 396 return status;
> 397 }
>
> ----------------------snip------------------------------------
> #cid 847 base/src/linux-2.6/fs/nfs/nfs4proc.c
>
> 1296 nfs4_open_revalidate(struct inode *dir, struct dentry *dentry,
> int openflags, struct nameidata *nd)
> 1297 {
> 1298 struct rpc_cred *cred;
> 1299 struct nfs4_state *state;
> 1300
> 1301 cred = rpcauth_lookupcred(NFS_CLIENT(dir)->cl_auth, 0);
> 1302 if (IS_ERR(cred))
> 1303 return PTR_ERR(cred);
>
> Event deref_ptr_in_call: Dereferences pointer "(dentry)->d_inode"
> [model]
> Also see events: [check_after_deref]
Broken coverity. The caller _always_ tests for negative dentries and
routes around this call.
> 1304 state = nfs4_open_delegated(dentry->d_inode, openflags,
> cred);
>
> At conditional (1): "IS_ERR != 0" taking true path
What on earth does this "error" mean?
> 1305 if (IS_ERR(state))
> 1306 state = nfs4_do_open(dir, dentry, openflags,
> NULL, cred);
> 1307 put_rpccred(cred);
>
> At conditional (2): "IS_ERR != 0" taking true path
Ditto?
> 1308 if (IS_ERR(state)) {
> 1309 switch (PTR_ERR(state)) {
> 1310 case -EPERM:
> 1311 case -EACCES:
> 1312 case -EDQUOT:
> 1313 case -ENOSPC:
> 1314 case -EROFS:
> 1315 lookup_instantiate_filp(nd,
> (struct dentry *)state, NULL);
> 1316 return 1;
>
> At conditional (3): "PTR_ERR == -2" taking true path
Ditto?
> 1317 case -ENOENT:
>
> Event check_after_deref: Pointer "(dentry)->d_inode" dereferenced before
> NULL check
> Also see events: [deref_ptr_in_call]
Junk. See above.
> 1318 if (dentry->d_inode == NULL)
> 1319 return 1;
> 1320 }
> 1321 goto out_drop
>
Cheers,
Trond
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]
2006-09-26 18:55 ` Trond Myklebust
@ 2006-09-26 19:37 ` Chuck Lever
2006-09-26 20:13 ` Trond Myklebust
0 siblings, 1 reply; 6+ messages in thread
From: Chuck Lever @ 2006-09-26 19:37 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Eric Sesterhenn / Snakebyte, Linux NFS Mailing List
Some comments below.
On 9/26/06, Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> > #cid 1373 base/src/linux-2.6/fs/nfs/inode.c
> >
> > 568 __nfs_revalidate_inode(struct nfs_server *server, struct inode
> > *inode)
> > 569 {
> > 570 int status = -ESTALE;
> > 571 struct nfs_fattr fattr;
> > 572 struct nfs_inode *nfsi = NFS_I(inode);
> > 573
> >
> > Event deref_ptr: Directly dereferenced pointer "inode"
> > Also see events: [check_after_deref]
> > At conditional (1): "0" taking false path
> >
> > 574 dfprintk(PAGECACHE, "NFS: revalidating (%s/%Ld)\n",
> > 575 inode->i_sb->s_id, (long
> > long)NFS_FILEID(inode));
> > 576
> > 577 nfs_inc_stats(inode, NFSIOS_INODEREVALIDATE);
> > 578 lock_kernel();
> >
> > Event check_after_deref: Pointer "inode" dereferenced before NULL check
> > Also see events: [deref_ptr]
> >
> > 579 if (!inode || is_bad_inode(inode))
> > 580 goto out_nowait
> >
>
> If _ever_ __nfs_revalidate_inode() is called with a null inode pointer
> value, then we definitely want to Oops. It should not be possible.
If you review this area of the code, you see that the "if (!inode ||"
is completely superfluous, in that case. That's what coverity is
pointing out here. (Actually I thought this check had already been
removed long ago).
Getting rid of the check makes the code slightly clearer, does not
change the characteristic that the code will oops if inode == NULL,
and has the side advantage of shutting up coverity checking. That's
all my patch does.
> > #cid 847 base/src/linux-2.6/fs/nfs/nfs4proc.c
> >
> > 1296 nfs4_open_revalidate(struct inode *dir, struct dentry *dentry,
> > int openflags, struct nameidata *nd)
> > 1297 {
> > 1298 struct rpc_cred *cred;
> > 1299 struct nfs4_state *state;
> > 1300
> > 1301 cred = rpcauth_lookupcred(NFS_CLIENT(dir)->cl_auth, 0);
> > 1302 if (IS_ERR(cred))
> > 1303 return PTR_ERR(cred);
> >
> > Event deref_ptr_in_call: Dereferences pointer "(dentry)->d_inode"
> > [model]
> > Also see events: [check_after_deref]
>
> Broken coverity. The caller _always_ tests for negative dentries and
> routes around this call.
>
> > 1304 state = nfs4_open_delegated(dentry->d_inode, openflags,
> > cred);
> >
> > At conditional (1): "IS_ERR != 0" taking true path
>
> What on earth does this "error" mean?
It's not an error. This is describing the execution path coverity
used to find the problem. The conditional is assumed to be true for
the sake of this error report.
> > 1305 if (IS_ERR(state))
> > 1306 state = nfs4_do_open(dir, dentry, openflags,
> > NULL, cred);
> > 1307 put_rpccred(cred);
> >
> > At conditional (2): "IS_ERR != 0" taking true path
> Ditto?
> > 1308 if (IS_ERR(state)) {
> > 1309 switch (PTR_ERR(state)) {
> > 1310 case -EPERM:
> > 1311 case -EACCES:
> > 1312 case -EDQUOT:
> > 1313 case -ENOSPC:
> > 1314 case -EROFS:
> > 1315 lookup_instantiate_filp(nd,
> > (struct dentry *)state, NULL);
> > 1316 return 1;
> >
> > At conditional (3): "PTR_ERR == -2" taking true path
> Ditto?
> > 1317 case -ENOENT:
> >
> > Event check_after_deref: Pointer "(dentry)->d_inode" dereferenced before
> > NULL check
> > Also see events: [deref_ptr_in_call]
>
> Junk. See above.
>
> > 1318 if (dentry->d_inode == NULL)
> > 1319 return 1;
> > 1320 }
> > 1321 goto out_drop
What coverity is pointing out here is that if it is true that
dentry->d_inode is always valid, then how is this check ever useful?
Either a comment or removal of the check is called for here. I can't
see that the contents of dentry are changed by any routine called by
this function. So why are we checking if d_inode is NULL here?
--
"We who cut mere stones must always be envisioning cathedrals"
-- Quarry worker's creed
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]
2006-09-26 19:37 ` Chuck Lever
@ 2006-09-26 20:13 ` Trond Myklebust
2006-09-27 14:00 ` Eric Sesterhenn / Snakebyte
0 siblings, 1 reply; 6+ messages in thread
From: Trond Myklebust @ 2006-09-26 20:13 UTC (permalink / raw)
To: Chuck Lever; +Cc: Eric Sesterhenn / Snakebyte, Linux NFS Mailing List
On Tue, 2006-09-26 at 15:37 -0400, Chuck Lever wrote:
> Getting rid of the check makes the code slightly clearer, does not
> change the characteristic that the code will oops if inode == NULL,
> and has the side advantage of shutting up coverity checking. That's
> all my patch does.
That would be fine.
> > > #cid 847 base/src/linux-2.6/fs/nfs/nfs4proc.c
> > >
> > > 1296 nfs4_open_revalidate(struct inode *dir, struct dentry *dentry,
> > > int openflags, struct nameidata *nd)
> > > 1297 {
> > > 1298 struct rpc_cred *cred;
> > > 1299 struct nfs4_state *state;
> > > 1300
> > > 1301 cred = rpcauth_lookupcred(NFS_CLIENT(dir)->cl_auth, 0);
> > > 1302 if (IS_ERR(cred))
> > > 1303 return PTR_ERR(cred);
> > >
> > > Event deref_ptr_in_call: Dereferences pointer "(dentry)->d_inode"
> > > [model]
> > > Also see events: [check_after_deref]
> >
> > Broken coverity. The caller _always_ tests for negative dentries and
> > routes around this call.
> >
> > > 1304 state = nfs4_open_delegated(dentry->d_inode, openflags,
> > > cred);
> > >
> > > At conditional (1): "IS_ERR != 0" taking true path
> >
> > What on earth does this "error" mean?
>
> It's not an error. This is describing the execution path coverity
> used to find the problem. The conditional is assumed to be true for
> the sake of this error report.
>
> > > 1305 if (IS_ERR(state))
> > > 1306 state = nfs4_do_open(dir, dentry, openflags,
> > > NULL, cred);
> > > 1307 put_rpccred(cred);
> > >
> > > At conditional (2): "IS_ERR != 0" taking true path
> > Ditto?
> > > 1308 if (IS_ERR(state)) {
> > > 1309 switch (PTR_ERR(state)) {
> > > 1310 case -EPERM:
> > > 1311 case -EACCES:
> > > 1312 case -EDQUOT:
> > > 1313 case -ENOSPC:
> > > 1314 case -EROFS:
> > > 1315 lookup_instantiate_filp(nd,
> > > (struct dentry *)state, NULL);
> > > 1316 return 1;
> > >
> > > At conditional (3): "PTR_ERR == -2" taking true path
> > Ditto?
> > > 1317 case -ENOENT:
> > >
> > > Event check_after_deref: Pointer "(dentry)->d_inode" dereferenced before
> > > NULL check
> > > Also see events: [deref_ptr_in_call]
> >
> > Junk. See above.
> >
> > > 1318 if (dentry->d_inode == NULL)
> > > 1319 return 1;
> > > 1320 }
> > > 1321 goto out_drop
>
> What coverity is pointing out here is that if it is true that
> dentry->d_inode is always valid, then how is this check ever useful?
> Either a comment or removal of the check is called for here. I can't
> see that the contents of dentry are changed by any routine called by
> this function. So why are we checking if d_inode is NULL here?
I agree that we don't need to. Just drop the -ENOENT case altogether.
Cheers,
Trond
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]
2006-09-26 20:13 ` Trond Myklebust
@ 2006-09-27 14:00 ` Eric Sesterhenn / Snakebyte
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sesterhenn / Snakebyte @ 2006-09-27 14:00 UTC (permalink / raw)
To: Trond Myklebust
Cc: Eric Sesterhenn / Snakebyte, Linux NFS Mailing List, Chuck Lever
* Trond Myklebust (trond.myklebust@fys.uio.no) wrote:
> On Tue, 2006-09-26 at 15:37 -0400, Chuck Lever wrote:
> > Getting rid of the check makes the code slightly clearer, does not
> > change the characteristic that the code will oops if inode == NULL,
> > and has the side advantage of shutting up coverity checking. That's
> > all my patch does.
>
> That would be fine.
marked 1372, 1373 and 847 as BUG, they will dissappear from
coverity once the patches hit linus tree. Chuck, will you
do the patch for 847 which drops the ENOENT or should i submit one?
1034 is marked as FALSE in coverity, i will submit 1110 and 733
to the bugzilla as soon as i get my password
should I submit further coverity warnings to this list,
if some appear in the future?
Thanks, Eric
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-09-27 14:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <451962BD.6050909@oracle.com>
2006-09-26 17:36 ` [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c] Chuck Lever
2006-09-26 18:23 ` J. Bruce Fields
2006-09-26 18:55 ` Trond Myklebust
2006-09-26 19:37 ` Chuck Lever
2006-09-26 20:13 ` Trond Myklebust
2006-09-27 14:00 ` Eric Sesterhenn / Snakebyte
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.