On Fri, 18 Dec 2009 14:33:37 -0500
Trond Myklebust <Trond.Myklebust@xxxxxxxxxx> wrote:
> On Fri, 2009-12-18 at 10:25 -0500, Jeff Layton wrote:
> > On Fri, 18 Dec 2009 09:47:52 -0500
> > Trond Myklebust <Trond.Myklebust@xxxxxxxxxx> wrote:
> >
> > > On Fri, 2009-12-18 at 09:39 -0500, Jeff Layton wrote:
> > > > Yeah, we could do that with the existing code. I sort of don't like
> > > > that because it's hard to know if other functions could eventually
> > > > return an EACCES for another reason and then that error would bubble up
> > > > to this function. If you think it's the right thing to do though, I'm
> > > > OK with it.
> > >
> > > The error EACCES means 'you are not authorised'. I don't see how it can
> > > be ambiguous here.
> > >
> >
> > I guess I was thinking that the pipe writer might not be authorized to
> > import the sec context for some reason, which would be a different
> > situation. If that'll never be the case, then a single error code is
> > probably fine. I'll respin it to just special case EACCES for now and
> > will plan to have it special case the other error when that work is
> > more fleshed out.
> >
> > > > FWIW: The reason I'm poking around in here is because I'm taking a stab
> > > > at fixing the problem where syscalls start returning errors when a krb5
> > > > ticket expires.
> > > >
> > > > As part of that, I want to have gssd send a more granular error code
> > > > and have the kernel adjust what it does accordingly. I'd like to have
> > > > it retry the upcall indefinitely when there's an expired credcache, and
> > > > return an error when there's no credcache at all).
> > >
> > > That makes sense.
> > >
> > > > Without a separate downcall error field, we'll need to special case at
> > > > least 2 different errors -- one for a "real" EACCES and one that
> > > > indicates that the ticket expired and the upcall should be retried
> > > > instead.
> > >
> > > We can find another error for the 'ticket expired' case. EKEYEXPIRED
> > > springs to mind...
> > >
> >
> > That's exactly the error I was going to use.
> >
> > > > > BTW: while looking at this, I spotted a nasty bug in
> > > > > gss_import_sec_context_kerberos(). If the kzalloc() call fails, we will
> > > > > return a random error code since 'p' still points to a valid memory
> > > > > location...
> > > >
> > > > Good catch. Do you want fix that one, or should I?
> > > >
> > >
> > > I can do it, if you like.
> > >
> >
> > Sure. I see another problem in this area too. gss_import_sec_context
> > can return GSS_S_FAILURE which is unsigned and positive when cast to a
> > signed value. gss_import_sec_context checks for a negative return from
> > that function though. Should it be checking for non-zero instead?
> >
>
> No. All the other functions there return ENOMEM on memory allocation
> failure. The GSS_S_FAILURE is just wrong.
>
>
> --------------------------------------------------------------------------------------------
> SUNRPC: Fix the return value in gss_import_sec_context()
>
> From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
>
> If the context allocation fails, it will return GSS_S_FAILURE, which is
> neither a valid error code, nor is it even negative.
>
> Return ENOMEM instead...
>
> Reported-by: Jeff Layton <jlayton@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxx
> Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> ---
>
> net/sunrpc/auth_gss/gss_mech_switch.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>
> diff --git a/net/sunrpc/auth_gss/gss_mech_switch.c b/net/sunrpc/auth_gss/gss_mech_switch.c
> index 6efbb0c..76e4c6f 100644
> --- a/net/sunrpc/auth_gss/gss_mech_switch.c
> +++ b/net/sunrpc/auth_gss/gss_mech_switch.c
> @@ -252,7 +252,7 @@ gss_import_sec_context(const void *input_token, size_t bufsize,
> struct gss_ctx **ctx_id)
> {
> if (!(*ctx_id = kzalloc(sizeof(**ctx_id), GFP_KERNEL)))
> - return GSS_S_FAILURE;
> + return -ENOMEM;
> (*ctx_id)->mech_type = gss_mech_get(mech);
>
> return mech->gm_ops
>
Looks good.
Acked-by: 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
