[SSSD] [PATCHES] sss_cache tool invalidates records in memory cache

Simo Sorce simo at redhat.com
Thu Sep 13 17:59:33 UTC 2012


On Wed, 2012-09-12 at 15:12 +0200, Michal Židek wrote:
> On 09/12/2012 02:39 PM, Michal Židek wrote:
> > Patch 1 - tools_util.h now provides signal_sssd function. It
> > replaces send_sighup function from sss_debuglevel.c. It can
> > be used to send any signal to monitor (used in patch 2).
> >
> > Patch 2 - https://fedorahosted.org/sssd/ticket/1311

I have questions, nack until answered.

Comments inline:


> 
> From 17da45856fc0a723ae9d09431cb372c68a2ef07a Mon Sep 17 00:00:00 2001
> From: Michal Zidek <mzidek at redhat.com>
> Date: Tue, 11 Sep 2012 18:44:52 +0200
> Subject: [PATCH 2/2] sss_cache tool invalidates records in memory
> cache.
> 
> ---
>  src/responder/nss/nsssrv.c            |  37 +++++++++++-
>  src/responder/nss/nsssrv_mmap_cache.c | 110
> ++++++++++++++++++++++++++++++++++
>  src/responder/nss/nsssrv_mmap_cache.h |   2 +
>  src/tools/sss_cache.c                 |   9 +++
>  4 files changed, 157 insertions(+), 1 deletion(-)
> 
> diff --git a/src/responder/nss/nsssrv.c b/src/responder/nss/nsssrv.c
> index 64267e8..59f92c5 100644
> --- a/src/responder/nss/nsssrv.c
> +++ b/src/responder/nss/nsssrv.c
> @@ -51,10 +51,13 @@
>  #define SHELL_REALLOC_INCREMENT 5
>  #define SHELL_REALLOC_MAX       50
>  
> +static int nss_logrotate(DBusMessage *message,
> +                        struct sbus_connection *conn);
> +
>  struct sbus_method monitor_nss_methods[] = {
>      { MON_CLI_METHOD_PING, monitor_common_pong },
>      { MON_CLI_METHOD_RES_INIT, monitor_common_res_init },
> -    { MON_CLI_METHOD_ROTATE, responder_logrotate },
> +    { MON_CLI_METHOD_ROTATE, nss_logrotate },
>      { NULL, NULL }
>  };
>  
> @@ -66,6 +69,37 @@ struct sbus_interface monitor_nss_interface = {
>      NULL
>  };
>  
> +static int nss_logrotate(DBusMessage *message,
> +                        struct sbus_connection *conn)
> +{
> +    errno_t ret;
> +    struct resp_ctx *rctx =
> talloc_get_type(sbus_conn_get_private_data(conn),
> +                                            struct resp_ctx);
> +    struct nss_ctx *nctx = (struct nss_tx*)rctx->pvt_ctx;
> +
> +    ret = monitor_common_rotate_logs(rctx->cdb,
> rctx->confdb_service_path);
> +    if (ret != EOK) return ret;

Main questions:
the invalidation functions seem to be called unconditionally, what
prevent cache invalidation durint a normal log rotation ?
Shouldn't cache invalidation happen exclusively when sss_cache is asking
to drop caches ?

> +    /* Invalidate memory caches if used */
> +    if (nctx->pwd_mc_ctx != NULL) {

you do not need to check here ^^ you already check inside
sss_mmap_cache_validate if the input is null and return in that case.

> +        ret = sss_mmap_cache_invalidate(nctx->pwd_mc_ctx);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  ("passwd mmap cache invalidation failed\n"));
> +        }
> +    }
> +
> +    if (nctx->grp_mc_ctx != NULL) {

same as above

> +        ret = sss_mmap_cache_invalidate(nctx->grp_mc_ctx);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_CRIT_FAILURE,
> +                  ("group mmap cache invalidation failed\n"));
> +        }
> +    }
> +
> +    return monitor_common_pong(message, conn);
> +}


>  static errno_t nss_get_etc_shells(TALLOC_CTX *mem_ctx, char
> ***_shells)
>  {
>      int i = 0;
> @@ -266,6 +300,7 @@ int nss_process_init(TALLOC_CTX *mem_ctx,
>      struct sss_cmd_table *nss_cmds;
>      struct be_conn *iter;
>      struct nss_ctx *nctx;
> +    struct tevent_signal *tes;
>      int memcache_timeout;
>      int ret, max_retries;
>      int hret;
> diff --git a/src/responder/nss/nsssrv_mmap_cache.c
> b/src/responder/nss/nsssrv_mmap_cache.c
> index 07498a9..0a87754 100644
> --- a/src/responder/nss/nsssrv_mmap_cache.c
> +++ b/src/responder/nss/nsssrv_mmap_cache.c
> @@ -58,6 +58,8 @@ struct sss_mc_ctx {
>      uint32_t seed;          /* pseudo-random seed to avoid collision
> attacks */
>      time_t valid_time_slot; /* maximum time the entry is valid in
> seconds */
>  
> +    int n_elem;             /* max number of entries stored in mmap
> cache */
> +
>      void *mmap_base;        /* base address of mmap */
>      size_t mmap_size;       /* total size of mmap */
>  
> @@ -648,6 +650,9 @@ errno_t sss_mmap_cache_init(TALLOC_CTX *mem_ctx,
> const char *name,
>      /* We can use MC_ALIGN64 for this */
>      n_elem = MC_ALIGN64(n_elem);
>  
> +    /* store n_elem into context, so it can be used later for
> invalidation */
> +    mc_ctx->n_elem = n_elem;
> +
>      /* hash table is double the size because it will store both
> forward and
>       * reverse keys (name/uid, name/gid, ..) */
>      mc_ctx->ht_size = MC_HT_SIZE(n_elem * 2);
> @@ -725,3 +730,108 @@ done:
>      return ret;
>  }

Nack:

I do not like the following function as it is largely a copy/paste of
sss_mmap_cache_init.
Why donp;t you simply pass a pointer to struct sss_mc_ctx *mc_ctx
release it, and then simply call sss_mmap_cache_init() from scratch ?

Also n_elem is currently hardcoded, so saving it is not really useful,
however I would also *really* not save it as on cache invalidation you
will want to go and see if the configuration also changed and get the
new number of elements.
This is a perfect time to do that because you are killing the old file
and creating a new one so changing size during this operation is
actually a good idea (if needed).

Same for memcache_timeout parameters (which are already parametrized and
can be fetched from CONFDB_NSS_CONF_ENTRY / CONFDB_MEMCACHE_TIMEOUT

> +errno_t sss_mmap_cache_invalidate(struct sss_mc_ctx *mc_ctx)
> +{
> +    unsigned int rseed;
> +    int payload;
> +    errno_t ret;
> +
> +    if (mc_ctx == NULL) {
> +        DEBUG(SSSDBG_TRACE_FUNC,
> +              ("Memory cache not used. Nothing to do here.\n"));
> +        return EOK;
> +    }
> +    switch (mc_ctx->type) {
> +    case SSS_MC_PASSWD:
> +        payload = SSS_AVG_PASSWD_PAYLOAD;
> +        break;
> +    case SSS_MC_GROUP:
> +        payload = SSS_AVG_GROUP_PAYLOAD;
> +        break;
> +    default:
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Memory cache corrupted.\n"));
> +        return EINVAL;
> +    }
> +
> +    /* close the old file descriptor */
> +    ret = close(mc_ctx->fd);
> +    if (ret != 0) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Unable to close file descriptor:
> %d"
> +                                    ": %s", ret, strerror(ret)));
> +        return ret;
> +    }
> +    mc_ctx->fd = -1;
> +
> +    /* hash table is double the size because it will store both
> forward and
> +     * reverse keys (name/uid, name/gid, ..) */
> +    mc_ctx->ht_size = MC_HT_SIZE(mc_ctx->n_elem * 2);
> +    mc_ctx->dt_size = MC_DT_SIZE(mc_ctx->n_elem, payload);
> +    mc_ctx->ft_size = MC_FT_SIZE(mc_ctx->n_elem);
> +    mc_ctx->mmap_size = MC_HEADER_SIZE +
> +                        MC_ALIGN64(mc_ctx->dt_size) +
> +                        MC_ALIGN64(mc_ctx->ft_size) +
> +                        MC_ALIGN64(mc_ctx->ht_size);
> +
> +
> +    /* create a new file */
> +    ret = sss_mc_create_file(mc_ctx);
> +    if (ret) {
> +        goto done;
> +    }
> +
> +    ret = ftruncate(mc_ctx->fd, mc_ctx->mmap_size);
> +    if (ret == -1) {
> +        ret = errno;
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to resize file %s: %d(%
> s)\n",
> +                                    mc_ctx->file, ret,
> strerror(ret)));
> +        goto done;
> +    }
> +
> +    /* map the file to memory */
> +    mc_ctx->mmap_base = mmap(NULL, mc_ctx->mmap_size,
> +                             PROT_READ | PROT_WRITE,
> +                             MAP_SHARED, mc_ctx->fd, 0);
> +    if (mc_ctx->mmap_base == MAP_FAILED) {
> +        ret = errno;
> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to mmap file %s(%ld): %
> d(%s)\n",
> +                                    mc_ctx->file, mc_ctx->mmap_size,
> +                                    ret, strerror(ret)));
> +        goto done;
> +    }
> +
> +    mc_ctx->data_table = MC_PTR_ADD(mc_ctx->mmap_base,
> MC_HEADER_SIZE);
> +    mc_ctx->free_table = MC_PTR_ADD(mc_ctx->data_table,
> +                                    MC_ALIGN64(mc_ctx->dt_size));
> +    mc_ctx->hash_table = MC_PTR_ADD(mc_ctx->free_table,
> +                                    MC_ALIGN64(mc_ctx->ft_size));
> +
> +    memset(mc_ctx->data_table, 0x00, mc_ctx->dt_size);
> +    memset(mc_ctx->free_table, 0x00, mc_ctx->ft_size);
> +    memset(mc_ctx->hash_table, 0xff, mc_ctx->ht_size);
> +
> +    /* generate a pseudo-random seed.
> +     * Needed to fend off dictionary based collision attacks */
> +    rseed = time(NULL) * getpid();
> +    mc_ctx->seed = rand_r(&rseed);
> +
> +    sss_mc_header_update(mc_ctx, SSS_MC_HEADER_ALIVE);
> +
> +    ret = EOK;
> +
> +done:
> +    if (ret) {
> +        if (mc_ctx->mmap_base && mc_ctx->mmap_base != MAP_FAILED) {
> +            munmap(mc_ctx->mmap_base, mc_ctx->mmap_size);
> +        }
> +        if (mc_ctx->fd != -1) {
> +            close(mc_ctx->fd);
> +            ret = unlink(mc_ctx->file);
> +            if (ret == -1) {
> +                DEBUG(SSSDBG_CRIT_FAILURE,
> +                      ("Failed to rm mmap file %s: %d(%s)\n",
> +                       mc_ctx->file, ret, strerror(ret)));
> +            }
> +        }
> +    }
> +    return ret;
> +}
> diff --git a/src/responder/nss/nsssrv_mmap_cache.h
> b/src/responder/nss/nsssrv_mmap_cache.h
> index 81241b2..3a808e6 100644
> --- a/src/responder/nss/nsssrv_mmap_cache.h
> +++ b/src/responder/nss/nsssrv_mmap_cache.h
> @@ -48,4 +48,6 @@ errno_t sss_mmap_cache_gr_store(struct sss_mc_ctx
> *mcc,
>                                  gid_t gid, size_t memnum,
>                                  char *membuf, size_t memsize);
>  
> +errno_t sss_mmap_cache_invalidate(struct sss_mc_ctx*);
> +
>  #endif /* _NSSSRV_MMAP_CACHE_H_ */
> diff --git a/src/tools/sss_cache.c b/src/tools/sss_cache.c
> index 950ff1c..63f31ad 100644
> --- a/src/tools/sss_cache.c
> +++ b/src/tools/sss_cache.c
> @@ -141,6 +141,15 @@ int main(int argc, const char *argv[])
>              ERROR("No cache object matched the specified search\n");
>              ret = ENOENT;
>              goto done;
> +        } else {
> +            /*Local cache changed -> signal monitor to invalidate
> fastcache */
> +            DEBUG(SSSDBG_TRACE_FUNC, ("Sending SIGHUP to
> monitor.\n"));
> +            ret = signal_sssd(SIGHUP);
> +            if (ret != EOK) {
> +                DEBUG(SSSDBG_CRIT_FAILURE,
> +                      ("Failed to send SIGHUP to monitor.\n"));
> +                goto done;
> +            }
>          }
>      }
>  
> -- 
> 1.7.11.2
> 
> 
-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list