[Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]

[Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]

[Chuck Lever]

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

Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]

[J. Bruce Fields]

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

Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]

[Trond Myklebust]

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

Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]

[Chuck Lever]

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

Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]

[Trond Myklebust]

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

Re: [Fwd: Re: [Patch] Possible dereference in fs/nfsd/nfs4callback.c]

[Eric Sesterhenn / Snakebyte]

* 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