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(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.
Yep, that was my thinking as well.
ACK.