On Mon, Sep 21, 2015 at 05:34:46PM +0200, Willy Tarreau wrote: > On Mon, Sep 21, 2015 at 04:26:51PM +0200, Remi Gacogne wrote: > > On 09/21/2015 02:57 PM, Willy Tarreau wrote: > > > > > > On Mon, Sep 21, 2015 at 10:28:09AM +0000, mlist wrote: > > >> We installed "HA-Proxy version 1.6-dev5-caa6a1b 2015/09/17", as you can > > >> see conn_cur in stick table seems wrong. Also with a single http access > > >> test, we get inizial correct conn_cur=1 but after expired haproxy insert > > >> a new record with same expire time with conn_cur like that below. So no > > >> more src_conn_cur filtering for DDOS prevention can be done. > > >> > > >> # table: main_fe, type: ip, size:102400, used:1 > > >> 0x1d47cfc: key=192.168.1.21 use=0 exp=26956 conn_cnt=21 > > >> conn_rate(6000)=2 conn_cur=4294967275 sess_cnt=21 http_req_cnt=21 > > > > > > Could you please share your configuration, I'm unable to reproduce the > > > same > > > problem and I'd like to understand why. There's clearly a bug here :-/ > > > > I'm sure you already noticed that, but the value of conn_cur seems to be > > 2^32 - conn_cnt. As I don't believe much in that kind of coincidence, I > > would bet my money on conn_cur being decremented twice. > > Hmmm bad news :-( > > It's due to something I had to interrupt when working on splitting sessions > and streams and that I forgot to finish :-( > > So here what happens is that the counters in the stream are the copy of those > in the session but we don't know which ones were allocated from the session > and which ones from the stream, so all stream counters are released then all > session counters are released. This happens when the tracking is performed at > the connection level. I'll check, I suspect that we *just* need to compare the > pointers when doing the store_counters operation in the stream, but I need to > validate. > > Thanks for reporting this issue, it's a really nasty one that we're lucky to > find before people start to use it in production. I may delay dev6 a little > bit depending on the difficulties I'm facing.
It's OK with the attached fix. It's not the cleanest one for the long term but at least it does the right thing regarding this shared tracking between the session and the stream. Thanks for reporting this one! Willy
>From a68f7629ddc4f12f63f6e1a6703b8f6952393cde Mon Sep 17 00:00:00 2001 From: Willy Tarreau <[email protected]> Date: Mon, 21 Sep 2015 17:48:24 +0200 Subject: BUG/MEDIUM: stick-tables: fix double-decrement of tracked entries Mailing list participant "mlist" reported negative conn_cur values in stick tables as the result of "tcp-request connection track-sc". The reason is that after the stick entry it copied from the session to the stream, both the session and the stream grab a reference to the entry and when the stream ends, it decrements one reference and one connection, then the same is done for the session. In fact this problem was already encountered slightly differently in the past and addressed by Thierry using the patch below as it was believed by then to be only a refcount issue since it was the observable symptom : 827752e "BUG/MEDIUM: stick-tables: refcount error after copying SC..." In reality the problem is that the stream must touch neither the refcount nor the connection count for entries it inherits from the session. While we have no way to tell whether a track entry was inherited from the session (since they're simply memcpy'd), it is possible to prevent the stream from touching an entry that already exists in the session because that's a guarantee that it was inherited from it. Note that it may be a temporary fix. Maybe in the future when a session gives birth to multiple streams we'll face a situation where a session may be updated to add more tracked entries after instanciating some streams. The correct long-term fix is to mark some tracked entries as shared or private (or RO/RW). That will allow the session to track more entries even after the same trackers are being used by early streams. No backport is needed, this is only caused by the session/stream split in 1.6. --- include/proto/stream.h | 13 +++++++++++-- src/stream.c | 11 +++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/include/proto/stream.h b/include/proto/stream.h index d4f8213..1fea1ab 100644 --- a/include/proto/stream.h +++ b/include/proto/stream.h @@ -88,7 +88,8 @@ static inline enum obj_type *strm_orig(const struct stream *strm) /* Remove the refcount from the stream to the tracked counters, and clear the * pointer to ensure this is only performed once. The caller is responsible for - * ensuring that the pointer is valid first. + * ensuring that the pointer is valid first. We must be extremely careful not + * to touch the entries we inherited from the session. */ static inline void stream_store_counters(struct stream *s) { @@ -98,6 +99,10 @@ static inline void stream_store_counters(struct stream *s) for (i = 0; i < MAX_SESS_STKCTR; i++) { if (!stkctr_entry(&s->stkctr[i])) continue; + + if (stkctr_entry(&s->sess->stkctr[i])) + continue; + ptr = stktable_data_ptr(s->stkctr[i].table, stkctr_entry(&s->stkctr[i]), STKTABLE_DT_CONN_CUR); if (ptr) stktable_data_cast(ptr, conn_cur)--; @@ -109,7 +114,8 @@ static inline void stream_store_counters(struct stream *s) /* Remove the refcount from the stream counters tracked at the content level if * any, and clear the pointer to ensure this is only performed once. The caller - * is responsible for ensuring that the pointer is valid first. + * is responsible for ensuring that the pointer is valid first. We must be + * extremely careful not to touch the entries we inherited from the session. */ static inline void stream_stop_content_counters(struct stream *s) { @@ -120,6 +126,9 @@ static inline void stream_stop_content_counters(struct stream *s) if (!stkctr_entry(&s->stkctr[i])) continue; + if (stkctr_entry(&s->sess->stkctr[i])) + continue; + if (!(stkctr_flags(&s->stkctr[i]) & STKCTR_TRACK_CONTENT)) continue; diff --git a/src/stream.c b/src/stream.c index dadb80f..6c1f7ab 100644 --- a/src/stream.c +++ b/src/stream.c @@ -71,7 +71,6 @@ struct stream *stream_new(struct session *sess, struct task *t, enum obj_type *o struct stream *s; struct connection *conn = objt_conn(origin); struct appctx *appctx = objt_appctx(origin); - int i; if (unlikely((s = pool_alloc2(pool2_stream)) == NULL)) return s; @@ -108,13 +107,13 @@ struct stream *stream_new(struct session *sess, struct task *t, enum obj_type *o s->current_rule_list = NULL; s->current_rule = NULL; - /* Copy SC counters for the stream. Each SC counter will be used by - * the stream, so we need to increment the refcount. + /* Copy SC counters for the stream. We don't touch refcounts because + * any reference we have is inherited from the session. Since the stream + * doesn't exist without the session, the session's existence guarantees + * we don't lose the entry. During the store operation, the stream won't + * touch these ones. */ memcpy(s->stkctr, sess->stkctr, sizeof(s->stkctr)); - for (i = 0; i < MAX_SESS_STKCTR; i++) - if (stkctr_entry(&s->stkctr[i])) - stkctr_entry(&s->stkctr[i])->ref_cnt++; s->sess = sess; s->si[0].flags = SI_FL_NONE; -- 1.7.12.1
