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

Jakub Hrozek jhrozek at redhat.com
Thu Sep 5 13:51:28 UTC 2013


On Thu, Sep 05, 2013 at 09:02:36AM -0400, Simo Sorce wrote:
> On Thu, 2013-09-05 at 11:26 +0200, Jakub Hrozek wrote:
> > 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.
> 
> The first patch is better, more fail safe, in the unlikely case
> something else ends up causing the slot to have a MC_INVALID_VAL
> content.
> 
> Simo.

Yep, that was my thinking as well.

ACK.



More information about the sssd-devel mailing list