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(a)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.
--
Simo Sorce * Red Hat, Inc * New York