On Thu, Sep 05, 2013 at 03:51:28PM +0200, Jakub Hrozek wrote:
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.
Pushed to master, sssd-1-10 and sssd-1-9