[SSSD] [PATCH] NSS: Set UID and GID to negative cache after searching all domains

Jakub Hrozek jhrozek at redhat.com
Wed Sep 18 15:58:38 UTC 2013


Hi,

the first patch fixes the bug that Jean-Baptiste Denis found earlier
today. In case the lookup was not done using a FQDN, the code would skip
setting the entries to the ncache.

The second patch is an incremental improvement. I don't think we should
abort the whole lookup if setting an entry in negcache would fail. The
negative cache is a performance optimization after all.
-------------- next part --------------
>From 69f6ed72838e37d7b8bc6201ed0b5463ffd3c95e Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 18 Sep 2013 15:42:41 +0200
Subject: [PATCH 1/2] NSS: Set UID and GID to negative cache after searching
 all domains

https://fedorahosted.org/sssd/ticket/2090

Previously, when searching by UID or GID, the negative cache will only
work in case the UID was searched for using fully qualified names.
---
 src/responder/nss/nsssrv_cmd.c | 69 ++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 229548976744220b6f10b184a28a8ac8c11559a7..5b9e71eeb4b04aef5664123d817fb248476d6da8 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -1236,7 +1236,7 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx)
                 dom = get_next_domain(dom, true);
                 continue;
             }
-            return ENOENT;
+            goto done;
         }
 
         if (dom != dctx->domain) {
@@ -1274,15 +1274,8 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx)
                 continue;
             }
 
-            DEBUG(2, ("No results for getpwuid call\n"));
-
-            /* set negative cache only if not result of cache check */
-            ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);
-            if (ret != EOK) {
-                return ret;
-            }
-
-            return ENOENT;
+            DEBUG(SSSDBG_MINOR_FAILURE, ("No results for getpwuid call\n"));
+            goto done;
         }
 
         /* if this is a caching provider (or if we haven't checked the cache
@@ -1306,7 +1299,14 @@ static int nss_cmd_getpwuid_search(struct nss_dom_ctx *dctx)
         return EOK;
     }
 
-    DEBUG(2, ("No matching domain found for [%d], fail!\n", cmdctx->id));
+done:
+    /* set negative cache only if not result of cache check */
+    ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);
+    if (ret != EOK) {
+        return ret;
+    }
+
+    DEBUG(SSSDBG_MINOR_FAILURE, ("No matching domain found for [%d]\n", cmdctx->id));
     return ENOENT;
 }
 
@@ -2668,7 +2668,7 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx)
                 dom = get_next_domain(dom, true);
                 continue;
             }
-            return ENOENT;
+            goto done;
         }
 
         if (dom != dctx->domain) {
@@ -2706,15 +2706,8 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx)
                 continue;
             }
 
-            DEBUG(2, ("No results for getgrgid call\n"));
-
-            /* set negative cache only if not result of cache check */
-            ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);
-            if (ret != EOK) {
-                return ret;
-            }
-
-            return ENOENT;
+            DEBUG(SSSDBG_MINOR_FAILURE, ("No results for getgrgid call\n"));
+            goto done;
         }
 
         /* if this is a caching provider (or if we haven't checked the cache
@@ -2738,7 +2731,14 @@ static int nss_cmd_getgrgid_search(struct nss_dom_ctx *dctx)
         return EOK;
     }
 
-    DEBUG(2, ("No matching domain found for [%d], fail!\n", cmdctx->id));
+done:
+    /* set negative cache only if not result of cache check */
+    ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);
+    if (ret != EOK) {
+        return ret;
+    }
+
+    DEBUG(SSSDBG_MINOR_FAILURE, ("No matching domain found for [%d]\n", cmdctx->id));
     return ENOENT;
 }
 
@@ -3806,20 +3806,7 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
             }
 
             DEBUG(SSSDBG_OP_FAILURE, ("No matching user or group found.\n"));
-
-            if (cmdctx->cmd == SSS_NSS_GETSIDBYID) {
-                ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);
-                if (ret != EOK) {
-                    return ret;
-                }
-
-                ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);
-                if (ret != EOK) {
-                    return ret;
-                }
-            }
-
-            return ENOENT;
+            goto done;
         }
 
         /* if this is a caching provider (or if we haven't checked the cache
@@ -3864,9 +3851,19 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
         return EOK;
     }
 
+done:
     if (cmdctx->cmd == SSS_NSS_GETSIDBYID) {
         DEBUG(SSSDBG_MINOR_FAILURE,
               ("No matching domain found for [%d], fail!\n", cmdctx->id));
+        ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);
+        if (ret != EOK) {
+            return ret;
+        }
+
+        ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);
+        if (ret != EOK) {
+            return ret;
+        }
     } else {
         DEBUG(SSSDBG_MINOR_FAILURE,
               ("No matching domain found for [%s], fail!\n", cmdctx->name));
-- 
1.8.3.1

-------------- next part --------------
>From 92318b5cf7c8192bef628a2bbe202b7bfb7fde45 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Wed, 18 Sep 2013 15:49:46 +0200
Subject: [PATCH 2/2] NSS: Failure to store entry negative cache should not be
 fatal

The only effect the failure to store a result to negative cache might
have would be a slower lookup next time.
---
 src/responder/nss/nsssrv_cmd.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 5b9e71eeb4b04aef5664123d817fb248476d6da8..21e48bd324c9dccb69c464d34263e0d98ad6b2e4 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -773,7 +773,8 @@ static int nss_cmd_getpwnam_search(struct nss_dom_ctx *dctx)
             /* set negative cache only if not result of cache check */
             ret = sss_ncache_set_user(nctx->ncache, false, dom, name);
             if (ret != EOK) {
-                return ret;
+                DEBUG(SSSDBG_MINOR_FAILURE,
+                      ("Cannot set negative cache for %s@%s\n", dom, name));
             }
 
             /* if a multidomain search, try with next */
@@ -1303,7 +1304,9 @@ done:
     /* set negative cache only if not result of cache check */
     ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);
     if (ret != EOK) {
-        return ret;
+        /* This should not be fatal, we'll just hit the DP next time */
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              ("Cannot set negative cache for UID %d\n", cmdctx->id));
     }
 
     DEBUG(SSSDBG_MINOR_FAILURE, ("No matching domain found for [%d]\n", cmdctx->id));
@@ -2583,7 +2586,8 @@ static int nss_cmd_getgrnam_search(struct nss_dom_ctx *dctx)
             /* set negative cache only if not result of cache check */
             ret = sss_ncache_set_group(nctx->ncache, false, dom, name);
             if (ret != EOK) {
-                return ret;
+                DEBUG(SSSDBG_MINOR_FAILURE,
+                      ("Cannot set negative cache for %s@%s\n", dom, name));
             }
 
             /* if a multidomain search, try with next */
@@ -2735,7 +2739,8 @@ done:
     /* set negative cache only if not result of cache check */
     ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);
     if (ret != EOK) {
-        return ret;
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              ("Cannot set negative cache for GID %d\n", cmdctx->id));
     }
 
     DEBUG(SSSDBG_MINOR_FAILURE, ("No matching domain found for [%d]\n", cmdctx->id));
@@ -3561,7 +3566,8 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx *dctx)
             /* set negative cache only if not result of cache check */
             ret = sss_ncache_set_user(nctx->ncache, false, dom, name);
             if (ret != EOK) {
-                return ret;
+                DEBUG(SSSDBG_MINOR_FAILURE,
+                      ("Cannot set negative cache for %s@%s\n", dom, name));
             }
 
             /* if a multidomain search, try with next */
@@ -3791,12 +3797,14 @@ static errno_t nss_cmd_getsidby_search(struct nss_dom_ctx *dctx)
             if (cmdctx->cmd == SSS_NSS_GETSIDBYNAME) {
                 ret = sss_ncache_set_user(nctx->ncache, false, dom, name);
                 if (ret != EOK) {
-                    return ret;
+                    DEBUG(SSSDBG_MINOR_FAILURE,
+                          ("Cannot set negative cache for %s@%s\n", dom, name));
                 }
 
                 ret = sss_ncache_set_group(nctx->ncache, false, dom, name);
                 if (ret != EOK) {
-                    return ret;
+                    DEBUG(SSSDBG_MINOR_FAILURE,
+                          ("Cannot set negative cache for %s@%s\n", dom, name));
                 }
             }
             /* if a multidomain search, try with next */
@@ -3857,12 +3865,14 @@ done:
               ("No matching domain found for [%d], fail!\n", cmdctx->id));
         ret = sss_ncache_set_uid(nctx->ncache, false, cmdctx->id);
         if (ret != EOK) {
-            return ret;
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  ("Cannot set negative cache for UID %d\n", cmdctx->id));
         }
 
         ret = sss_ncache_set_gid(nctx->ncache, false, cmdctx->id);
         if (ret != EOK) {
-            return ret;
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  ("Cannot set negative cache for GID %d\n", cmdctx->id));
         }
     } else {
         DEBUG(SSSDBG_MINOR_FAILURE,
@@ -3911,7 +3921,8 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
         /* set negative cache only if not result of cache check */
         ret = sss_ncache_set_sid(nctx->ncache, false, cmdctx->secid);
         if (ret != EOK) {
-            return ret;
+            DEBUG(SSSDBG_MINOR_FAILURE,
+                  ("Cannot set negative cache for %s\n", cmdctx->secid));
         }
 
         return ENOENT;
-- 
1.8.3.1



More information about the sssd-devel mailing list