[SSSD] [PATCH] mmap_cache: Do not remove record from chain twice

Jakub Hrozek jhrozek at redhat.com
Thu Sep 5 09:05:43 UTC 2013


On Thu, Sep 05, 2013 at 10:12:28AM +0200, Lukas Slebodnik wrote:
> On (05/09/13 09:59), Lukas Slebodnik wrote:
> >ehlo,
> >
> >It is not very likely, that record will have the same hash1 and hash2, but it
> >is possible. In this situation, it does not make sense to remove record twice.
> >
> >Function sss_mc_rm_rec_from_chain was not robust and sssd_nss could crash
> >in this situation. It was only possible if record was alone in chain.
> >
> >The reproducer is very simple name and uid must be the same. (and we should
> >wait until this record will be removed)
> >
> >We solved problem with in mmap_cache with commit
> >4662725ffef62b3b2502481438effa7c8fef9f80
> >"mmap_cache: Skip records which doesn't have same hash"
> >
> >Although this problem was solved, user reported another crash
> >https://bugzilla.redhat.com/show_bug.cgi?id=997406#c12
> >He wrote:
> >>On my first runs it also did not core dump for days ... also in this
> >>testing cycle I only had those two core dumps yesterday but no more -
> >>eems difficult to reproduce even in my environment.
> >
> >I was unable to reproduce user crash, so I decided to decrease size of
> >hash_table (16 times) and I hit this issue. Backtrace from BZ997406#c12 looks
> >differently; I am still trying to reproduce his issue.
> >
> >Patch is attached.
> >
> >LS
> >
> >BTW: I was very lucky, because I ran into situation, record had the same
> >hashes for name and uid
> >
> >(gdb) call sss_mc_hash(mcc, "testbigldap121129", 18)
> >$18 = 1532
> >(gdb) p sss_mc_hash(mcc, "121129", 7)
> >$19 = 1532
> >(gdb) p mcc->ht_size
> >$20 = 25000
> 
> >From 877c1f85c6ca16fc94b29d8b0680fa3c88cd6f86 Mon Sep 17 00:00:00 2001
> >From: Lukas Slebodnik <lslebodn at redhat.com>
> >Date: Thu, 5 Sep 2013 09:26:43 +0200
> >Subject: [PATCH] mmap_cache: Do not remove record from chain twice
> >
> >It is not very likely, that record will have the same hash1 and hash2, but it
> >is possible. In this situation, it does not make sense to remove record twice.
> >
> >Function sss_mc_rm_rec_from_chain was not robust and sssd_nss could crash
> >in this situation. It was only possible if record was alone in chain.
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/2049
> >---
> > src/responder/nss/nsssrv_mmap_cache.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> >diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
> >index 84570ac204b12a561a337ba9f3fd1ce9d104feaf..a22bbd5947cf67ffa1b8275e8608607e9a3ba39f 100644
> >--- a/src/responder/nss/nsssrv_mmap_cache.c
> >+++ b/src/responder/nss/nsssrv_mmap_cache.c
> >@@ -243,6 +243,12 @@ static void sss_mc_rm_rec_from_chain(struct sss_mc_ctx *mcc,
> >     }
> > 
> >     slot = mcc->hash_table[hash];
> >+    if (slot == MC_INVALID_VAL) {
> >+        /* record has already been removed. It may happen if rec->hash1 and
> >+         * rec->has2 are the same. (It is not very likely).
> >+         */
> >+        return;
> >+    }
> >     cur = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec);
> >     if (cur == rec) {
> >         /* rec->next can refer to record without matching hashes.
> >-- 
> 
> 
> I forgot to write alternative solution, but I prefer the first one.
> ---------------------------------------
> @@ -299,7 +299,9 @@ static void sss_mc_invalidate_rec(struct sss_mc_ctx *mcc,
>      /* hash chain 1 */
>          sss_mc_rm_rec_from_chain(mcc, rec, rec->hash1);
>      /* hash chain 2 */
> -    sss_mc_rm_rec_from_chain(mcc, rec, rec->hash2);
> +    if (rec->hash1 != rec->hash2) {
> +        sss_mc_rm_rec_from_chain(mcc, rec, rec->hash2);
> +    }
> 
>      /* Clear from free_table */
>      sss_mc_free_slots(mcc, rec);

I'm still not very familiar with mmap cache code, so I'm not sure what
the outcome of the bug would be? Crash or "just" removal of another
entry from memcache?

The first approach looks more compact to me.



More information about the sssd-devel mailing list