Overhaul the ban_lurker to avoid a race condition where it checks

an object at the same time a request-lookup does.

The functional solution to this race is to hold the objhdr mutex,
which we already hold briefly to get the refcount, also when we do
the check.

In terms of souce code this inlines the problematic HSH_FindBan()
function in the lurker.

And since that was major surgery af few other acts of improvement
was carried out also.

Most notably, we will now scan all applicable bans in the lurker
and not give up on the first ban that tests req.* variables.

bin/varnishd/cache_ban.c

@@ -421,7 +421,7 @@ ban_check_object(struct object *o, const struct sess *sp, int has_req)
 	struct objcore *oc;
 	struct ban_test *bt;
 	struct ban * volatile b0;
-	unsigned tests;
+	unsigned tests, skipped;
 
 	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
 	CHECK_OBJ_NOTNULL(o, OBJECT_MAGIC);
@@ -441,12 +441,19 @@ ban_check_object(struct object *o, const struct sess *sp, int has_req)
 	 * inspect the list past that ban.
 	 */
 	tests = 0;
+	skipped = 0;
 	for (b = b0; b != oc->ban; b = VTAILQ_NEXT(b, list)) {
 		CHECK_OBJ_NOTNULL(b, BAN_MAGIC);
 		if (b->flags & BAN_F_GONE)
 			continue;
-		if (!has_req && (b->flags & BAN_F_REQ))
-			return (0);
+		if (!has_req && (b->flags & BAN_F_REQ)) {
+			/*
+			 * We cannot test this one, but there might
+			 * be other bans that match, so we soldier on
+			 */
+			skipped++;
+			continue;
+		}
 		VTAILQ_FOREACH(bt, &b->tests, list) {
 			tests++;
 			if (bt->func(bt, o, sp))
@@ -456,6 +463,14 @@ ban_check_object(struct object *o, const struct sess *sp, int has_req)
 			break;
 	}
 
+	if (b == oc->ban && skipped > 0) {
+		/*
+		 * Not banned, but some tests were skipped, so we cannot know
+		 * for certain that it cannot be, so we just have to give up.
+		 */
+		return (0);
+	}
+
 	Lck_Lock(&ban_mtx);
 	oc->ban->refcount--;
 	VTAILQ_REMOVE(&oc->ban->objcore, oc, ban_list);
@@ -487,67 +502,105 @@ int
 BAN_CheckObject(struct object *o, const struct sess *sp)
 {
 
-	return ban_check_object(o, sp, 1);
+	return (ban_check_object(o, sp, 1));
 }
 
 /*--------------------------------------------------------------------
  * Ban tail lurker thread
  */
 
-static void * __match_proto__(bgthread_t)
-ban_lurker(struct sess *sp, void *priv)
+static void
+ban_lurker_work(const struct sess *sp)
 {
 	struct ban *b, *bf;
-	struct objcore *oc;
+	struct objhead *oh;
+	struct objcore *oc, *oc2;
 	struct object *o;
 	int i;
 
-	(void)priv;
-	while (1) {
-		WSL_Flush(sp->wrk, 0);
-		WRK_SumStat(sp->wrk);
-		if (params->ban_lurker_sleep == 0.0) {
-			AZ(sleep(1));
-			continue;
-		}
-		Lck_Lock(&ban_mtx);
+	WSL_Flush(sp->wrk, 0);
+	WRK_SumStat(sp->wrk);
 
-		/* First try to route the last ban */
-		bf = BAN_CheckLast();
-		if (bf != NULL) {
-			Lck_Unlock(&ban_mtx);
-			BAN_Free(bf);
-			TIM_sleep(params->ban_lurker_sleep);
-			continue;
-		}
-		/* Then try to poke the first object on the last ban */
-		oc = NULL;
-		while (1) {
-			b = VTAILQ_LAST(&ban_head, banhead_s);
-			if (b == ban_start)
-				break;
-			oc = VTAILQ_FIRST(&b->objcore);
-			if (oc == NULL)
-				break;
-			HSH_FindBan(sp, &oc);
+	Lck_Lock(&ban_mtx);
+
+	/* First try to route the last ban */
+	bf = BAN_CheckLast();
+	if (bf != NULL) {
+		Lck_Unlock(&ban_mtx);
+		BAN_Free(bf);
+		return;
+	}
+
+	/* Find the last ban give up, if we have only one */
+	b = VTAILQ_LAST(&ban_head, banhead_s);
+	if (b == ban_start) {
+		Lck_Unlock(&ban_mtx);
+		return;
+	}
+
+	/* Find the first object on it, if any */
+	oc = VTAILQ_FIRST(&b->objcore);
+	if (oc == NULL) {
+		Lck_Unlock(&ban_mtx);
+		return;
+	}
+
+	/* Try to lock the objhead */
+	oh = oc->objhead;
+	CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
+	if (Lck_Trylock(&oh->mtx)) {
+		Lck_Unlock(&ban_mtx); 
+		return;
+	}
+
+	/*
+	 * See if the objcore is still on the objhead since we race against
+	 * HSH_Deref() which comes in the opposite locking order.
+	 */
+	VTAILQ_FOREACH(oc2, &oh->objcs, list)
+		if (oc == oc2)
 			break;
-		}
+	if (oc2 == NULL) {
+		Lck_Unlock(&oh->mtx);
 		Lck_Unlock(&ban_mtx);
-		if (oc == NULL) {
+		return;
+	}
+	/*
+	 * Grab a reference to the OC and we can let go of the BAN mutex
+	 */
+	AN(oc->refcnt);
+	oc->refcnt++;
+	Lck_Unlock(&ban_mtx);
+
+	/*
+	 * Get the object and check it against all relevant bans
+	 */
+	o = oc_getobj(sp->wrk, oc);
+	i = ban_check_object(o, sp, 0);
+	Lck_Unlock(&oh->mtx);
+	WSP(sp, SLT_Debug, "lurker: %p %g %d", oc, o->exp.ttl, i);
+	(void)HSH_Deref(sp->wrk, NULL, &o);
+}
+
+static void * __match_proto__(bgthread_t)
+ban_lurker(struct sess *sp, void *priv)
+{
+
+	(void)priv;
+	while (1) {
+		if (params->ban_lurker_sleep == 0.0) {
+			/* Lurker is disabled.  */
 			TIM_sleep(1.0);
 			continue;
 		}
-		// AZ(oc->flags & OC_F_PERSISTENT);
-		o = oc_getobj(sp->wrk, oc);
-		i = ban_check_object(o, sp, 0);
-		WSP(sp, SLT_Debug, "lurker: %p %g %d", oc, o->exp.ttl, i);
-		(void)HSH_Deref(sp->wrk, NULL, &o);
 		TIM_sleep(params->ban_lurker_sleep);
+		ban_lurker_work(sp);
+		WSL_Flush(sp->wrk, 0);
+		WRK_SumStat(sp->wrk);
 	}
 	NEEDLESS_RETURN(NULL);
 }
 
-
 /*--------------------------------------------------------------------
  * Release a tail reference
  */
@@ -680,12 +733,14 @@ BAN_Compile(void)
 		av = ParseArgv(b->test, 0);
 		XXXAN(av);
 		XXXAZ(av[0]);
-		for (i = 1; av[i] != NULL; i += 3) {
-			if (i != 1) {
-				AZ(strcmp(av[i], "&&"));
-				i++;
-			}
-			AZ(BAN_AddTest(NULL, b, av[i], av[i + 1], av[i + 2]));
+		XXXAN(av[1]);
+		XXXAN(av[2]);
+		XXXAN(av[3]);
+		AZ(BAN_AddTest(NULL, b, av[1], av[2], av[3]));
+		for (i = 4; av[i] != NULL; i += 4) {
+			AZ(strcmp(av[i], "&&"));
+			AZ(BAN_AddTest(NULL, b,
+			    av[i + 1], av[i + 2], av[i + 3]));
 		}
 	}
 	ban_start = VTAILQ_FIRST(&ban_head);

bin/varnishd/cache_hash.c

@@ -710,43 +710,6 @@ HSH_Deref(struct worker *w, struct objcore *oc, struct object **oo)
 	return (0);
 }
 
-/*--------------------------------------------------------------------
- * This one is slightly tricky.  This is called from the BAN module
- * to try to wash the object which holds the oldest ban.
- * We compete against HSH_Deref() which comes in the opposite
- * locking order, we need to hold the BAN mutex, to stop the
- * BAN_DestroyObj() call in HSH_Deref(), so that the objhead
- * will not be removed under us.
- * NB: Do not call this function any other way or from any other
- * NB: place in the code.  It will not work for you.
- */
-
-void
-HSH_FindBan(const struct sess *sp, struct objcore **oc)
-{
-	struct objcore *oc1, *oc2;
-	struct objhead *oh;
-
-	oc1 = *oc;
-	CHECK_OBJ_NOTNULL(oc1, OBJCORE_MAGIC);
-	oh = oc1->objhead;
-	CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
-	if (Lck_Trylock(&oh->mtx)) {
-		*oc = NULL;
-		return;
-	}
-	VTAILQ_FOREACH(oc2, &oh->objcs, list)
-		if (oc1 == oc2)
-			break;
-	if (oc2 != NULL)
-		(void)oc_getobj(sp->wrk, oc2);
-	if (oc2 != NULL)
-		oc2->refcnt++;
-	Lck_Unlock(&oh->mtx);
-	*oc = oc2;
-}
-
-
 void
 HSH_Init(void)
 {

bin/varnishd/hash_slinger.h

@@ -59,7 +59,6 @@ void HSH_Ref(struct objcore *o);
 void HSH_Drop(struct sess *sp);
 void HSH_Init(void);
 void HSH_AddString(const struct sess *sp, const char *str);
-void HSH_FindBan(const struct sess *sp, struct objcore **oc);
 struct objcore *HSH_Insert(const struct sess *sp);
 void HSH_Purge(const struct sess *, struct objhead *, double ttl, double grace);
 void HSH_config(const char *h_arg);