Re: [PATCH][XFRM] Optimize policy dumping

[Patrick McHardy <kaber@trash.net>]

[-- Attachment #1: Type: text/plain, Size: 1464 bytes --]

jamal wrote:
> On Mon, 2006-04-12 at 09:05 -0500, jamal wrote:
> 
>>Patrick,
>>
>>Your approach is much cleaner. Let me give these a few tests then
>>I will repost later today; forget about the callback approach for now.
>>
> 
> 
> I have just applied the policy patch; havent compiled or tested (the
> setup takes me a while to put together). But by staring, I am seeing
> that you will end up with the same thing of sending a NULL or the same
> entry twice.
> 
> Consider a simple hypothetical test. You have one one entry in the 
> xfrm_policy_inexact table that matches. It happens to be the fifth out
> of 10 elements. You find it at the 5th iteration. At the sixth iteration
> you send it and last becomes null. 
> 
> All the way down, you call func with a NULL entry. You could add a check
> to make sure it only gets invoked when last is not null, but the result
> is in such a case, you will never send a 0 count element. I am sure
> there could be other tricky scenarios like this that could be
> constructed.
> 
> Thoughts.

Double sending can't happen, but you're right about potentially
sending a NULL ptr when after setting it to NULL we don't find
any other matching elements.

This patch should fix it (and is even simpler), by moving the
check for pol->type != type before sending, we make sure that
last always contains a valid element unless count == 0.

Also fixed an incorrect gcc warning about last_dir potentially
being used uninitialized.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2162 bytes --]

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 64d3938..e19ec1e 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -860,33 +860,12 @@ EXPORT_SYMBOL(xfrm_policy_flush);
 int xfrm_policy_walk(u8 type, int (*func)(struct xfrm_policy *, int, int, void*),
 		     void *data)
 {
-	struct xfrm_policy *pol;
+	struct xfrm_policy *pol, *last = NULL;
 	struct hlist_node *entry;
-	int dir, count, error;
+	int dir, last_dir = 0, count, error;
 
 	read_lock_bh(&xfrm_policy_lock);
 	count = 0;
-	for (dir = 0; dir < 2*XFRM_POLICY_MAX; dir++) {
-		struct hlist_head *table = xfrm_policy_bydst[dir].table;
-		int i;
-
-		hlist_for_each_entry(pol, entry,
-				     &xfrm_policy_inexact[dir], bydst) {
-			if (pol->type == type)
-				count++;
-		}
-		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
-			hlist_for_each_entry(pol, entry, table + i, bydst) {
-				if (pol->type == type)
-					count++;
-			}
-		}
-	}
-
-	if (count == 0) {
-		error = -ENOENT;
-		goto out;
-	}
 
 	for (dir = 0; dir < 2*XFRM_POLICY_MAX; dir++) {
 		struct hlist_head *table = xfrm_policy_bydst[dir].table;
@@ -896,21 +875,35 @@ int xfrm_policy_walk(u8 type, int (*func
 				     &xfrm_policy_inexact[dir], bydst) {
 			if (pol->type != type)
 				continue;
-			error = func(pol, dir % XFRM_POLICY_MAX, --count, data);
-			if (error)
-				goto out;
+			if (last) {
+				error = func(last, last_dir % XFRM_POLICY_MAX,
+					     ++count, data);
+				if (error)
+					goto out;
+			}
+			last = pol;
+			last_dir = dir;
 		}
 		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
 			hlist_for_each_entry(pol, entry, table + i, bydst) {
 				if (pol->type != type)
 					continue;
-				error = func(pol, dir % XFRM_POLICY_MAX, --count, data);
-				if (error)
-					goto out;
+				if (last) {
+					error = func(last, last_dir % XFRM_POLICY_MAX,
+						     ++count, data);
+					if (error)
+						goto out;
+				}
+				last = pol;
+				last_dir = dir;
 			}
 		}
 	}
-	error = 0;
+	if (count == 0) {
+		error = -ENOENT;
+		goto out;
+	}
+	error = func(last, last_dir % XFRM_POLICY_MAX, 0, data);
 out:
 	read_unlock_bh(&xfrm_policy_lock);
 	return error;