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.
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;