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
--
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
