f300bab "nfsd41: sunrpc: add new xprt class for nfsv4.1 backchannel"
introduced an error case branch that lacks an actual `return' keyword
before the return value. Add it.
Signed-off-by: Johannes Weiner <[email protected]>
Cc: Alexandros Batsakis <[email protected]>
---
net/sunrpc/xprtsock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2444,7 +2444,7 @@ static struct rpc_xprt *xs_setup_bc_tcp(
struct svc_sock *bc_sock;
if (!args->bc_xprt)
- ERR_PTR(-EINVAL);
+ return ERR_PTR(-EINVAL);
xprt = xs_setup_xprt(args, xprt_tcp_slot_table_entries);
if (IS_ERR(xprt))
On Tue, 2010-05-04 at 13:59 +0200, Johannes Weiner wrote:
> f300bab "nfsd41: sunrpc: add new xprt class for nfsv4.1 backchannel"
> introduced an error case branch that lacks an actual `return' keyword
> before the return value. Add it.
>
> Signed-off-by: Johannes Weiner <[email protected]>
> Cc: Alexandros Batsakis <[email protected]>
> ---
> net/sunrpc/xprtsock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -2444,7 +2444,7 @@ static struct rpc_xprt *xs_setup_bc_tcp(
> struct svc_sock *bc_sock;
>
> if (!args->bc_xprt)
> - ERR_PTR(-EINVAL);
> + return ERR_PTR(-EINVAL);
>
> xprt = xs_setup_xprt(args, xprt_tcp_slot_table_entries);
> if (IS_ERR(xprt))
No. It should either be a BUG_ON(), or else be removed entirely.
Returning an error value for something that is clearly a programming bug
is not a particularly useful exercise...
Cheers
Trond
Trond Myklebust wrote:
> On Tue, 2010-05-04 at 13:59 +0200, Johannes Weiner wrote:
> > f300bab "nfsd41: sunrpc: add new xprt class for nfsv4.1 backchannel"
> > introduced an error case branch that lacks an actual `return' keyword
> > before the return value. Add it.
> >
> > Signed-off-by: Johannes Weiner <[email protected]>
> > Cc: Alexandros Batsakis <[email protected]>
> > ---
> > net/sunrpc/xprtsock.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- a/net/sunrpc/xprtsock.c
> > +++ b/net/sunrpc/xprtsock.c
> > @@ -2444,7 +2444,7 @@ static struct rpc_xprt *xs_setup_bc_tcp(
> > struct svc_sock *bc_sock;
> >
> > if (!args->bc_xprt)
> > - ERR_PTR(-EINVAL);
> > + return ERR_PTR(-EINVAL);
> >
> > xprt = xs_setup_xprt(args, xprt_tcp_slot_table_entries);
> > if (IS_ERR(xprt))
>
> No. It should either be a BUG_ON(), or else be removed entirely.
> Returning an error value for something that is clearly a programming bug
> is not a particularly useful exercise...
>
Removing NULL check is wrong because it will NULL pointer dereference later.
Tetsuo Handa wrote:
> Jani Nikula wrote:
> > Signed-off-by: Jani Nikula <[email protected]>
> >
> > ---
> >
> > NOTE: I'm afraid I'm unable to test this; please consider this more a
> > bug report than a complete patch.
> > ---
> Indeed, it has to be "return ERR_PTR(-EINVAL);".
> Otherwise, it will trigger NULL pointer dereference some lines later.
>
> bc_sock = container_of(args->bc_xprt, struct svc_sock, sk_xprt);
> bc_sock->sk_bc_xprt = xprt;
>
> This bug was introduced by f300baba5a1536070d6d77bf0c8c4ca999bb4f0f
> "nfsd41: sunrpc: add new xprt class for nfsv4.1 backchannel" and
> exists in 2.6.32 and later.
On Tue, 2010-05-04 at 22:03 +0900, Tetsuo Handa wrote:
> Trond Myklebust wrote:
> > On Tue, 2010-05-04 at 13:59 +0200, Johannes Weiner wrote:
> > > f300bab "nfsd41: sunrpc: add new xprt class for nfsv4.1 backchannel"
> > > introduced an error case branch that lacks an actual `return' keyword
> > > before the return value. Add it.
> > >
> > > Signed-off-by: Johannes Weiner <[email protected]>
> > > Cc: Alexandros Batsakis <[email protected]>
> > > ---
> > > net/sunrpc/xprtsock.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > --- a/net/sunrpc/xprtsock.c
> > > +++ b/net/sunrpc/xprtsock.c
> > > @@ -2444,7 +2444,7 @@ static struct rpc_xprt *xs_setup_bc_tcp(
> > > struct svc_sock *bc_sock;
> > >
> > > if (!args->bc_xprt)
> > > - ERR_PTR(-EINVAL);
> > > + return ERR_PTR(-EINVAL);
> > >
> > > xprt = xs_setup_xprt(args, xprt_tcp_slot_table_entries);
> > > if (IS_ERR(xprt))
> >
> > No. It should either be a BUG_ON(), or else be removed entirely.
> > Returning an error value for something that is clearly a programming bug
> > is not a particularly useful exercise...
> >
> Removing NULL check is wrong because it will NULL pointer dereference later.
Wrong. Removing NULL check is _right_ because calling this function
without setting up a back channel first is a major BUG. Returning an
error value to the user is pointless, since the user has no control over
this. It is entirely under control of the sunrpc developers...
Trond
Trond Myklebust wrote:
> > > No. It should either be a BUG_ON(), or else be removed entirely.
> > > Returning an error value for something that is clearly a programming bug
> > > is not a particularly useful exercise...
> > >
> > Removing NULL check is wrong because it will NULL pointer dereference later.
>
> Wrong. Removing NULL check is _right_ because calling this function
> without setting up a back channel first is a major BUG. Returning an
> error value to the user is pointless, since the user has no control over
> this. It is entirely under control of the sunrpc developers...
>
For security people, removing
if (!args->bc_xprt)
ERR_PTR(-EINVAL);
is worse and changing to
BUG_ON(!args->bc_xprt);
is better.