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;
