| Message ID | 1381418103-3852-3-git-send-email-bfields@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > For every other problem here we bail out with an error, but here for > some reason we're setting a negative cache entry (with, note, an > undefined expiry). > > It seems simplest just to bail out in the same way as we do in other > cases. > > Cc: Simo Sorce <simo@redhat.com> > Reported-by: Andi Kleen <andi@firstfloor.org> > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c > index 09fb638..008cdad 100644 > --- a/net/sunrpc/auth_gss/svcauth_gss.c > +++ b/net/sunrpc/auth_gss/svcauth_gss.c > @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd, > if (!ud->found_creds) { > /* userspace seem buggy, we should always get at least a > * mapping to nobody */ > - dprintk("RPC: No creds found, marking Negative!\n"); > - set_bit(CACHE_NEGATIVE, &rsci.h.flags); > + dprintk("RPC: No creds found!\n"); > + goto out; > } else { > > /* steal creds */ IIRC, we are doing this to avoid rapid upcall loops in the kernel, where we keep hammering upcalls out and keep getting an error back. I am not sure it is wise to not set a temp negative cache here. Simo.
On Thu, Oct 10, 2013 at 11:37:12AM -0400, Simo Sorce wrote: > On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > For every other problem here we bail out with an error, but here for > > some reason we're setting a negative cache entry (with, note, an > > undefined expiry). > > > > It seems simplest just to bail out in the same way as we do in other > > cases. > > > > Cc: Simo Sorce <simo@redhat.com> > > Reported-by: Andi Kleen <andi@firstfloor.org> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c > > index 09fb638..008cdad 100644 > > --- a/net/sunrpc/auth_gss/svcauth_gss.c > > +++ b/net/sunrpc/auth_gss/svcauth_gss.c > > @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd, > > if (!ud->found_creds) { > > /* userspace seem buggy, we should always get at least a > > * mapping to nobody */ > > - dprintk("RPC: No creds found, marking Negative!\n"); > > - set_bit(CACHE_NEGATIVE, &rsci.h.flags); > > + dprintk("RPC: No creds found!\n"); > > + goto out; > > } else { > > > > /* steal creds */ > > IIRC, we are doing this to avoid rapid upcall loops in the kernel, where > we keep hammering upcalls out and keep getting an error back. Looks like returning an error instead results in closing the connection to the client, so, depends how the client replies I guess. In any case I don't see why we'd treat this particular gss-proxy bug differently than we would any other (like, say, passing down bad xdr, or a gss context that we can't import). --b. > I am not sure it is wise to not set a temp negative cache here. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-10-10 at 15:23 -0400, J. Bruce Fields wrote: > On Thu, Oct 10, 2013 at 11:37:12AM -0400, Simo Sorce wrote: > > On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote: > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > For every other problem here we bail out with an error, but here for > > > some reason we're setting a negative cache entry (with, note, an > > > undefined expiry). > > > > > > It seems simplest just to bail out in the same way as we do in other > > > cases. > > > > > > Cc: Simo Sorce <simo@redhat.com> > > > Reported-by: Andi Kleen <andi@firstfloor.org> > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > --- > > > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c > > > index 09fb638..008cdad 100644 > > > --- a/net/sunrpc/auth_gss/svcauth_gss.c > > > +++ b/net/sunrpc/auth_gss/svcauth_gss.c > > > @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd, > > > if (!ud->found_creds) { > > > /* userspace seem buggy, we should always get at least a > > > * mapping to nobody */ > > > - dprintk("RPC: No creds found, marking Negative!\n"); > > > - set_bit(CACHE_NEGATIVE, &rsci.h.flags); > > > + dprintk("RPC: No creds found!\n"); > > > + goto out; > > > } else { > > > > > > /* steal creds */ > > > > IIRC, we are doing this to avoid rapid upcall loops in the kernel, where > > we keep hammering upcalls out and keep getting an error back. > > Looks like returning an error instead results in closing the connection > to the client, so, depends how the client replies I guess. > > In any case I don't see why we'd treat this particular gss-proxy bug > differently than we would any other (like, say, passing down bad xdr, or > a gss context that we can't import). Uhmm I did not recall that, I guess the change is ok then. Simo. > --b. > > > I am not sure it is wise to not set a temp negative cache here. > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 10, 2013 at 04:45:52PM -0400, Simo Sorce wrote: > On Thu, 2013-10-10 at 15:23 -0400, J. Bruce Fields wrote: > > On Thu, Oct 10, 2013 at 11:37:12AM -0400, Simo Sorce wrote: > > > On Thu, 2013-10-10 at 11:15 -0400, J. Bruce Fields wrote: > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > > > For every other problem here we bail out with an error, but here for > > > > some reason we're setting a negative cache entry (with, note, an > > > > undefined expiry). > > > > > > > > It seems simplest just to bail out in the same way as we do in other > > > > cases. > > > > > > > > Cc: Simo Sorce <simo@redhat.com> > > > > Reported-by: Andi Kleen <andi@firstfloor.org> > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > > --- > > > > net/sunrpc/auth_gss/svcauth_gss.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c > > > > index 09fb638..008cdad 100644 > > > > --- a/net/sunrpc/auth_gss/svcauth_gss.c > > > > +++ b/net/sunrpc/auth_gss/svcauth_gss.c > > > > @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd, > > > > if (!ud->found_creds) { > > > > /* userspace seem buggy, we should always get at least a > > > > * mapping to nobody */ > > > > - dprintk("RPC: No creds found, marking Negative!\n"); > > > > - set_bit(CACHE_NEGATIVE, &rsci.h.flags); > > > > + dprintk("RPC: No creds found!\n"); > > > > + goto out; > > > > } else { > > > > > > > > /* steal creds */ > > > > > > IIRC, we are doing this to avoid rapid upcall loops in the kernel, where > > > we keep hammering upcalls out and keep getting an error back. > > > > Looks like returning an error instead results in closing the connection > > to the client, so, depends how the client replies I guess. > > > > In any case I don't see why we'd treat this particular gss-proxy bug > > differently than we would any other (like, say, passing down bad xdr, or > > a gss context that we can't import). > > Uhmm I did not recall that, I guess the change is ok then. OK, thanks for the review. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c index 09fb638..008cdad 100644 --- a/net/sunrpc/auth_gss/svcauth_gss.c +++ b/net/sunrpc/auth_gss/svcauth_gss.c @@ -1167,8 +1167,8 @@ static int gss_proxy_save_rsc(struct cache_detail *cd, if (!ud->found_creds) { /* userspace seem buggy, we should always get at least a * mapping to nobody */ - dprintk("RPC: No creds found, marking Negative!\n"); - set_bit(CACHE_NEGATIVE, &rsci.h.flags); + dprintk("RPC: No creds found!\n"); + goto out; } else { /* steal creds */