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

Jakub Hrozek jhrozek at redhat.com
Thu Sep 5 09:26:34 UTC 2013


On Thu, Sep 05, 2013 at 11:20:12AM +0200, Lukas Slebodnik wrote:
> On (05/09/13 11:05), Jakub Hrozek wrote:
> >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.
> crash
> 
> Let's assume that
>    rec->hash1 == rec->hash2       // alias DUP_HASH
>    && rec->next == MC_INVALID_VAL // there is no ather rec in chain
> 
> 
> 
> sss_mc_rm_rec_from_chain(mcc, rec, rec->hash1);
> 
> //after first call  mcc->hash_table[DUP_HASH] will be equal to MC_INVALID_VAL
> //and second call will be decribed below
> sss_mc_rm_rec_from_chain(mcc, rec, rec->hash2);
> 
> 
> static void sss_mc_rm_rec_from_chain(struct sss_mc_ctx *mcc,
>                                      struct sss_mc_rec *rec,
>                                      uint32_t hash)
> .. snip ...
>     slot = mcc->hash_table[hash]; // slot will be MC_INVALID_VAL (0xffffffff)
>     cur = MC_SLOT_TO_PTR(mcc->data_table, slot, struct sss_mc_rec);
>                          // cur will refer after the mmaped data (Out of bound)
>     if (cur == rec) {    // comparision is FALSE, no crash
>         /* rec->next can refer to record without matching hashes.
>          * We need to skip this(those) records, because
>          * mcc->hash_table[hash] have to refer to valid start of the chain.
>          */
>         mcc->hash_table[hash] = sss_mc_get_next_slot_with_hash(mcc, rec, hash);
>     } else {
>         slot = cur->next;   // dereference of cur pointer "cur->next"
>                             // result: SEGFAULT
> 
> LS

OK, thank you for explanation. The patch looks good to me, although I
would like a second opinion from Simo or Michal as well.



More information about the sssd-devel mailing list