[redhat-lspp] RE: [IPSEC] flow: Cache negative results

[Joy Latten]

On Wed, 2007-01-10 at 15:08 -0600, Venkat Yekkirala wrote:
> > > Only, on a security policy denial (-ESRCH from the LSM hook), a 0
> > > is returned by the resolver to signify no applicable policy since
> > > a negative result is akin to no policy. And I see the "no policy"
> > > case is already cached.
> >
> > I'm not talking about an xfrm policy lookup failure, that exists
> > with or without SELinux. I'm talking about an error returned from
> > security_xfrm_policy_lookup(), i.e., whether a policy can be used
> > or not.
> 
> I was talking about this (the latter) as well. Currently, on a proper
> "negative", -ESRCH is returned by security_xfrm_policy_lookup(), and
> this comes back up as a 0 from resolver(), correctly indicating NO
> applicable
> xfrm policy (after taking security into account). But if
> security_xfrm_policy_lookup()
> were to return anything other than a zero or -ESRCH, such as -ENOMEM,
> you will see it come back up as such (as -ENOMEM) from resolver(),
> and in this case, it's neither a positive nor a negative, just an error.
> Hence a full lookup would be in order, the next time round.
> 
> >  For that case, we only cache positive results currently.
> 
> Negatives are currently properly cached as NULL. Any errors
> returned from resolver() are true errors, not negatives. Hence,
> they needn't be cached.
> 
> Also, I would fix the other bug you had noted, by something like:
> 
> @@ -232,11 +232,7 @@ nocache:
>                 err = resolver(key, family, dir, &obj, &obj_ref);
> 
>                 if (fle) {
> -                       if (err) {
> -                               /* Force security policy check on next
> lookup */
> -                               *head = fle->next;
> -                               flow_entry_kill(cpu, fle);
> -                       } else {
> +                       if (!err) {
>                                 fle->genid = atomic_read(&flow_cache_genid);
> 
>                                 if (fle->object)
> 
> 
> I am planning to test and submit a patch to SELinux
> to invoke flow_cache_flush() on policy reloads tomorrow.
> 
> I believe changes to labels on SPD rules are already
> taken care of by checks involving flow_cache_genid.

Wow! Venkat, I wonder if this explains why our lspp kernels fail to
accept unlabeled packets when you have just a single ipsec policy
entered!  And why I never see any denials!

In our lspp kernel, xfrm_policy_lookup() returns -ESRCH to 
flow_cache_lookup() when he is not able to find a policy. 
(as is the case with unlabeled_packets)
flow_cache_lookup() then processes this as an error.

In the upstream kernel (I have been looking at 2.6.20-rc3-git4),
xfrm_policy_lookup() returns 0 in this case... 

flow_cache_lookup() then treats it as an error in lspp kernel
and as a success in the 2.6.0-rc3-git4 kernel.org kernel.

I tried the following patch and I was finally able to receive 
unlabeled packets in lspp kernel when a single ipsec policy
is entered! 

Please let me know if this looks ok and I am not
jumping the gun. This only applies to our lspp kernel 
and was built against lspp62 kernel.

Regards,
Joy
 
diff -urpN linux-2.6.18.ppc64.orig/net/xfrm/xfrm_policy.c
linux-2.6.18.ppc64/net/xfrm/xfrm_policy.c
--- linux-2.6.18.ppc64.orig/net/xfrm/xfrm_policy.c      2007-01-11
15:56:23.000000000 -0600
+++ linux-2.6.18.ppc64/net/xfrm/xfrm_policy.c   2007-01-11
16:28:10.000000000 -0600
@@ -609,7 +609,7 @@ static int xfrm_policy_lookup(struct flo
                               void **objp, atomic_t **obj_refp)
 {
        struct xfrm_policy *pol;
-       int ret = -ESRCH;
+       int ret = 0;

        read_lock_bh(&xfrm_policy_lock);
        for (pol = xfrm_policy_list[dir]; pol; pol = pol->next) {
@@ -627,6 +627,8 @@ static int xfrm_policy_lookup(struct flo
                                xfrm_pol_hold(pol);
                                break;
                        }
+                       if (ret == -ESRCH)
+                               ret = 0;                 
		}
        }
        read_unlock_bh(&xfrm_policy_lock);