On Fri, 18 Dec 2009 15:24:26 -0500
Trond Myklebust <Trond.Myklebust@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
