[PATCH] sunrpc: on successful gss error pipe write, don't return error (try #2)

[PATCH] sunrpc: on successful gss error pipe write, don't return error (try #2)

[Jeff Layton]

When handling the gssd downcall, the kernel should distinguish between a
successful downcall that contains an error code and a failed downcall
(i.e. where the parsing failed or some other sort of problem occurred).

In the former case, gss_pipe_downcall should be returning the number of
bytes written to the pipe instead of an error.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/auth_gss/auth_gss.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 3c3c50f..7afc8e2 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -645,6 +645,9 @@ gss_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
 	if (IS_ERR(p)) {
 		err = PTR_ERR(p);
 		gss_msg->msg.errno = (err == -EAGAIN) ? -EAGAIN : -EACCES;
+		/* special case: downcall was successful, but held an error */
+		if (err == -EACCES)
+			err = mlen;
 		goto err_release_msg;
 	}
 	gss_msg->ctx = gss_get_ctx(ctx);
-- 
1.6.5.2

_______________________________________________
NFSv4 mailing list
NFSv4@linux-nfs.org
http://linux-nfs.org/cgi-bin/mailman/listinfo/nfsv4

Re: [PATCH] sunrpc: on successful gss error pipe write, don't return error (try #2)

[Trond Myklebust]

On Fri, 2009-12-18 at 11:27 -0500, Jeff Layton wrote: 
> When handling the gssd downcall, the kernel should distinguish between a
> successful downcall that contains an error code and a failed downcall
> (i.e. where the parsing failed or some other sort of problem occurred).
> 
> In the former case, gss_pipe_downcall should be returning the number of
> bytes written to the pipe instead of an error.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  net/sunrpc/auth_gss/auth_gss.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index 3c3c50f..7afc8e2 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -645,6 +645,9 @@ gss_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
>  	if (IS_ERR(p)) {
>  		err = PTR_ERR(p);
>  		gss_msg->msg.errno = (err == -EAGAIN) ? -EAGAIN : -EACCES;
> +		/* special case: downcall was successful, but held an error */
> +		if (err == -EACCES)
> +			err = mlen;

That line immediately above your fix still looks wrong. The point is
that AFAICS, err is never going to be set to EAGAIN. It can be EFAULT,
ENOSYS, or ENOMEM, but it will never be EAGAIN...

I think we should rather reverse that test. Really, what we want to do,
is to set msg.errno to -EAGAIN for -EFAULT and -ENOMEM (and probably for
ENOSYS too), and then set it to -EACCES _only_ in the case where the
user was not authorised.

Trond

Re: [PATCH] sunrpc: on successful gss error pipe write, don't return error (try #2)

[Jeff Layton]

On Fri, 18 Dec 2009 14:05:39 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Fri, 2009-12-18 at 11:27 -0500, Jeff Layton wrote: 
> > When handling the gssd downcall, the kernel should distinguish between a
> > successful downcall that contains an error code and a failed downcall
> > (i.e. where the parsing failed or some other sort of problem occurred).
> > 
> > In the former case, gss_pipe_downcall should be returning the number of
> > bytes written to the pipe instead of an error.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  net/sunrpc/auth_gss/auth_gss.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > index 3c3c50f..7afc8e2 100644
> > --- a/net/sunrpc/auth_gss/auth_gss.c
> > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > @@ -645,6 +645,9 @@ gss_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
> >  	if (IS_ERR(p)) {
> >  		err = PTR_ERR(p);
> >  		gss_msg->msg.errno = (err == -EAGAIN) ? -EAGAIN : -EACCES;
> > +		/* special case: downcall was successful, but held an error */
> > +		if (err == -EACCES)
> > +			err = mlen;
> 
> That line immediately above your fix still looks wrong. The point is
> that AFAICS, err is never going to be set to EAGAIN. It can be EFAULT,
> ENOSYS, or ENOMEM, but it will never be EAGAIN...
> 
> I think we should rather reverse that test. Really, what we want to do,
> is to set msg.errno to -EAGAIN for -EFAULT and -ENOMEM (and probably for
> ENOSYS too), and then set it to -EACCES _only_ in the case where the
> user was not authorised.

What should we do if err is "none of the above"? Set msg.errno to
-EACCES and return the error to the pipe writer?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] sunrpc: on successful gss error pipe write, don't return error (try #2)

[Trond Myklebust]

On Fri, 2009-12-18 at 15:13 -0500, Jeff Layton wrote: 
> On Fri, 18 Dec 2009 14:05:39 -0500
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > On Fri, 2009-12-18 at 11:27 -0500, Jeff Layton wrote: 
> > > When handling the gssd downcall, the kernel should distinguish between a
> > > successful downcall that contains an error code and a failed downcall
> > > (i.e. where the parsing failed or some other sort of problem occurred).
> > > 
> > > In the former case, gss_pipe_downcall should be returning the number of
> > > bytes written to the pipe instead of an error.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  net/sunrpc/auth_gss/auth_gss.c |    3 +++
> > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > > index 3c3c50f..7afc8e2 100644
> > > --- a/net/sunrpc/auth_gss/auth_gss.c
> > > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > > @@ -645,6 +645,9 @@ gss_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
> > >  	if (IS_ERR(p)) {
> > >  		err = PTR_ERR(p);
> > >  		gss_msg->msg.errno = (err == -EAGAIN) ? -EAGAIN : -EACCES;
> > > +		/* special case: downcall was successful, but held an error */
> > > +		if (err == -EACCES)
> > > +			err = mlen;
> > 
> > That line immediately above your fix still looks wrong. The point is
> > that AFAICS, err is never going to be set to EAGAIN. It can be EFAULT,
> > ENOSYS, or ENOMEM, but it will never be EAGAIN...
> > 
> > I think we should rather reverse that test. Really, what we want to do,
> > is to set msg.errno to -EAGAIN for -EFAULT and -ENOMEM (and probably for
> > ENOSYS too), and then set it to -EACCES _only_ in the case where the
> > user was not authorised.
> 
> What should we do if err is "none of the above"? Set msg.errno to
> -EACCES and return the error to the pipe writer?
> 

The question is will it ever be 'none of the above'? We clearly cannot
be returning arbitrary errors to gssd, so we need to define a set that
makes sense. The only other error I can see that we might to add to the
above list, would be EINVAL (to mean 'you just sent me some garbage
argument that I cannot decode').

Retrying the upcall would seem to be the correct thing to do in case of
EINVAL too...

Trond

Re: [PATCH] sunrpc: on successful gss error pipe write, don't return error (try #2)

[Jeff Layton]

On Fri, 18 Dec 2009 15:24:26 -0500
Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> On Fri, 2009-12-18 at 15:13 -0500, Jeff Layton wrote: 
> > On Fri, 18 Dec 2009 14:05:39 -0500
> > Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> > 
> > > On Fri, 2009-12-18 at 11:27 -0500, Jeff Layton wrote: 
> > > > When handling the gssd downcall, the kernel should distinguish between a
> > > > successful downcall that contains an error code and a failed downcall
> > > > (i.e. where the parsing failed or some other sort of problem occurred).
> > > > 
> > > > In the former case, gss_pipe_downcall should be returning the number of
> > > > bytes written to the pipe instead of an error.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > ---
> > > >  net/sunrpc/auth_gss/auth_gss.c |    3 +++
> > > >  1 files changed, 3 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > > > index 3c3c50f..7afc8e2 100644
> > > > --- a/net/sunrpc/auth_gss/auth_gss.c
> > > > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > > > @@ -645,6 +645,9 @@ gss_pipe_downcall(struct file *filp, const char __user *src, size_t mlen)
> > > >  	if (IS_ERR(p)) {
> > > >  		err = PTR_ERR(p);
> > > >  		gss_msg->msg.errno = (err == -EAGAIN) ? -EAGAIN : -EACCES;
> > > > +		/* special case: downcall was successful, but held an error */
> > > > +		if (err == -EACCES)
> > > > +			err = mlen;
> > > 
> > > That line immediately above your fix still looks wrong. The point is
> > > that AFAICS, err is never going to be set to EAGAIN. It can be EFAULT,
> > > ENOSYS, or ENOMEM, but it will never be EAGAIN...
> > > 
> > > I think we should rather reverse that test. Really, what we want to do,
> > > is to set msg.errno to -EAGAIN for -EFAULT and -ENOMEM (and probably for
> > > ENOSYS too), and then set it to -EACCES _only_ in the case where the
> > > user was not authorised.
> > 
> > What should we do if err is "none of the above"? Set msg.errno to
> > -EACCES and return the error to the pipe writer?
> > 
> 
> The question is will it ever be 'none of the above'? We clearly cannot
> be returning arbitrary errors to gssd, so we need to define a set that
> makes sense. The only other error I can see that we might to add to the
> above list, would be EINVAL (to mean 'you just sent me some garbage
> argument that I cannot decode').
> 
> Retrying the upcall would seem to be the correct thing to do in case of
> EINVAL too...
> 

Well, I'd like to think that we'd always get the lower layers right, but
I know better :). We probably ought to code defensively here and account
for the possibility that this function could return a different error
due to an inadvertent change in a lower function.

If the "bad" error is transient then -EAGAIN would probably just paper
over it. If it's not transient then the mount would hang. -EACCES is a
nice hard error, but could be confused with a "real" -EACCES. We could
BUG() in that case if we're sure it'd be a real bug...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] sunrpc: on successful gss error pipe write, don't return error (try #2)

[Trond Myklebust]

On Fri, 2009-12-18 at 15:38 -0500, Jeff Layton wrote:

> Well, I'd like to think that we'd always get the lower layers right, but
> I know better :). We probably ought to code defensively here and account
> for the possibility that this function could return a different error
> due to an inadvertent change in a lower function.
> 
> If the "bad" error is transient then -EAGAIN would probably just paper
> over it. If it's not transient then the mount would hang. -EACCES is a
> nice hard error, but could be confused with a "real" -EACCES. We could
> BUG() in that case if we're sure it'd be a real bug...


switch (err) {
case -EACCES:
gss_msg->msg.errno = -EACCES;
break;
case -EFAULT:
case -ENOMEM:
case -EINVAL:
case -ENOSYS:
gss_msg->msg.errno = -EAGAIN;
break;
default:
BUG();
}

Yup. That looks good to me... ..and with the fixes to
gss_import_sec_context_kerberos and gss_import_sec_context, it will
actually be correct too.

BTW: get_key() returns EINVAL on error, so that does indeed need to be
supported in the above list.

Trond

Re: [PATCH] sunrpc: on successful gss error pipe write, don't return error (try #2)

[Trond Myklebust]

On Fri, 2009-12-18 at 15:56 -0500, Trond Myklebust wrote: 
> switch (err) {
> case -EACCES:
> gss_msg->msg.errno = -EACCES;
> break;
> case -EFAULT:
> case -ENOMEM:
> case -EINVAL:
> case -ENOSYS:
> gss_msg->msg.errno = -EAGAIN;
> break;
> default:
> BUG();
> }
> 
> Yup. That looks good to me...


Well... Minus the helpful reformatting that evolution did for me after I
pressed the 'send' button.

Trond