[IPSEC] flow: Cache negative results

[IPSEC] flow: Cache negative results

[Herbert Xu]

Hi:

[IPSEC] flow: Cache negative security checks

This patch causes security policy denials to be cached instead of
causing a relookup every time.  This is OK because we already cache
positive security policy results which is strictly worse as far as
security is concerned.  In particular, if the security system (not
IPsec policies but the rules under security/) changes such that a
positive result turns negative (denial), we will ignore it and
continue to allow traffic through based on the cached policy.

So if the security folks actually care about this, they'd need to
flush the flow cache whenever a relevant change is made to the
security database.  Whether this is done or not does not affect
this patch.

Given that we do want to cache positive results even in the presence
of SELinux (otherwise we might as well disable flow.c entirely), it
is natural to cache negative results too.

This patch also happens to fix a nasty bug where if an expiring
flow entry that's not at the head happens to trigger a security
denial, all entries before it are removed from the cache and
leaked.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/core/flow.c b/net/core/flow.c
index d137f97..b0d1777 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -197,12 +197,16 @@ void *flow_cache_lookup(struct flowi *key, u16 family, u8 dir,
 			if (fle->genid == atomic_read(&flow_cache_genid)) {
 				void *ret = fle->object;
 
-				if (ret)
+				if (fle->object_ref)
 					atomic_inc(fle->object_ref);
 				local_bh_enable();
 
 				return ret;
 			}
+
+			if (fle->object_ref)
+				atomic_dec(fle->object_ref);
+			fle->object_ref = NULL;
 			break;
 		}
 	}
@@ -218,7 +222,7 @@ void *flow_cache_lookup(struct flowi *key, u16 family, u8 dir,
 			fle->family = family;
 			fle->dir = dir;
 			memcpy(&fle->key, key, sizeof(*key));
-			fle->object = NULL;
+			fle->object_ref = NULL;
 			flow_count(cpu)++;
 		}
 	}
@@ -230,28 +234,20 @@ nocache:
 		atomic_t *obj_ref;
 
 		err = resolver(key, family, dir, &obj, &obj_ref);
+		if (err)
+			obj = ERR_PTR(err);
 
 		if (fle) {
-			if (err) {
-				/* Force security policy check on next lookup */
-				*head = fle->next;
-				flow_entry_kill(cpu, fle);
-			} else {
-				fle->genid = atomic_read(&flow_cache_genid);
+			fle->object = obj;
+			fle->genid = atomic_read(&flow_cache_genid);
 
-				if (fle->object)
-					atomic_dec(fle->object_ref);
-
-				fle->object = obj;
+			if (!err && obj) {
 				fle->object_ref = obj_ref;
-				if (obj)
-					atomic_inc(fle->object_ref);
+				atomic_inc(obj_ref);
 			}
 		}
 		local_bh_enable();
 
-		if (err)
-			obj = ERR_PTR(err);
 		return obj;
 	}
 }

Re: [IPSEC] flow: Cache negative results

[David Miller]

From: Herbert Xu <herbert.xu@redhat.com>
Date: Wed, 10 Jan 2007 18:22:51 +1100

> This patch also happens to fix a nasty bug where if an expiring
> flow entry that's not at the head happens to trigger a security
> denial, all entries before it are removed from the cache and
> leaked.

Nasty, this is because "fle != NULL" can occur in the
'nocache' path for two reasons.  New entry, or the case
that's buggy here, existing entry with out-of-date generation
ID.

Thanks Herbert, I'll look this over some more and apply it some
time tomorrow.

Re: [IPSEC] flow: Cache negative results

[James Morris]

On Wed, 10 Jan 2007, Herbert Xu wrote:

> Hi:
> 
> [IPSEC] flow: Cache negative security checks

We did some work in this part of the code a few months back -- IIRC it was 
resolved correctly from a security point of view.

(cc'ing Venkat & Paul for review).

> 
> This patch causes security policy denials to be cached instead of
> causing a relookup every time.  This is OK because we already cache
> positive security policy results which is strictly worse as far as
> security is concerned.  In particular, if the security system (not
> IPsec policies but the rules under security/) changes such that a
> positive result turns negative (denial), we will ignore it and
> continue to allow traffic through based on the cached policy.
> 
> So if the security folks actually care about this, they'd need to
> flush the flow cache whenever a relevant change is made to the
> security database.  Whether this is done or not does not affect
> this patch.
> 
> Given that we do want to cache positive results even in the presence
> of SELinux (otherwise we might as well disable flow.c entirely), it
> is natural to cache negative results too.
> 
> This patch also happens to fix a nasty bug where if an expiring
> flow entry that's not at the head happens to trigger a security
> denial, all entries before it are removed from the cache and
> leaked.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Cheers,
> 

-- 
James Morris
<jmorris@namei.org>

RE: [IPSEC] flow: Cache negative results

[Venkat Yekkirala]

> > So if the security folks actually care about this, they'd need to
> > flush the flow cache whenever a relevant change is made to the
> > security database.

I do not believe we are doing this. I will look into this ASAP.

RE: [IPSEC] flow: Cache negative results

[Venkat Yekkirala]

> > This patch causes security policy denials to be cached instead of
> > causing a relookup every time.

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 think what may have gotten us here is the comment:

                       if (err) {
                                /* Force security policy check on next
lookup */
                                *head = fle->next;
                                flow_entry_kill(cpu, fle);
                        } else {

But the error we would be looking at here would be a non-denial
related error (neither positive nor negative) that the security
server may have run into, in which case we would in fact want to
attempt a full-lookup again the next time.

<snip>
> > So if the security folks actually care about this, they'd need to
> > flush the flow cache whenever a relevant change is made to the
> > security database.

Sure. Will look into this.

> >
> > This patch also happens to fix a nasty bug where if an expiring
> > flow entry that's not at the head happens to trigger a security
> > denial, all entries before it are removed from the cache and
> > leaked.

I think just leaving the flow_entry as it is will take care of it.

IOW, no entry_killing nor messing with it's object/reference. This
should naturally cause us to invoke the resolver again the next time.

Re: [IPSEC] flow: Cache negative results

[Herbert Xu]

On Wed, Jan 10, 2007 at 11:41:07AM -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.  For that case, we only cache positive results currently.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [IPSEC] flow: Cache negative results

[Venkat Yekkirala]

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

Re: [IPSEC] flow: Cache negative results

[Herbert Xu]

On Wed, Jan 10, 2007 at 03:08:55PM -0600, Venkat Yekkirala wrote:
> 
> 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.

OK, I see that now.  The EACCES error is translated to ESRCH in
the security layer.

> Also, I would fix the other bug you had noted, by something like:

I agree.

[IPSEC] flow: Fix potential memory leak

When old flow cache entries that are not at the head of their chain
trigger a transient security error they get unlinked along with all
the entries preceding them in the chain.  The preceding entries are
not freed correctly.

This patch fixes this by simply leaving the entry around.  It's based
on a suggestion by Venkat Yekkirala.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
 
> I am planning to test and submit a patch to SELinux
> to invoke flow_cache_flush() on policy reloads tomorrow.

flow_cache_flush() shouldn't be used by SELinux.  You probably
want to increase the genid instead.

BTW, we should probably add some code to deal with overflows in
genid.  With the recent improvements in policy update speeds, it
could be conceivable for an overflow to occur.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/net/core/flow.c b/net/core/flow.c
index d137f97..5d25697 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -231,22 +231,16 @@ 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 {
-				fle->genid = atomic_read(&flow_cache_genid);
-
-				if (fle->object)
-					atomic_dec(fle->object_ref);
-
-				fle->object = obj;
-				fle->object_ref = obj_ref;
-				if (obj)
-					atomic_inc(fle->object_ref);
-			}
+		if (fle && !err) {
+			fle->genid = atomic_read(&flow_cache_genid);
+
+			if (fle->object)
+				atomic_dec(fle->object_ref);
+
+			fle->object = obj;
+			fle->object_ref = obj_ref;
+			if (obj)
+				atomic_inc(fle->object_ref);
 		}
 		local_bh_enable();
 

Re: [IPSEC] flow: Cache negative results

[David Miller]

From: Herbert Xu <herbert.xu@redhat.com>
Date: Thu, 11 Jan 2007 10:27:13 +1100

> On Wed, Jan 10, 2007 at 03:08:55PM -0600, Venkat Yekkirala wrote:
> > 
> > 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.
> 
> OK, I see that now.  The EACCES error is translated to ESRCH in
> the security layer.
> 
> > Also, I would fix the other bug you had noted, by something like:
> 
> I agree.
> 
> [IPSEC] flow: Fix potential memory leak
> 
> When old flow cache entries that are not at the head of their chain
> trigger a transient security error they get unlinked along with all
> the entries preceding them in the chain.  The preceding entries are
> not freed correctly.
> 
> This patch fixes this by simply leaving the entry around.  It's based
> on a suggestion by Venkat Yekkirala.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

This version of the fix looks good, I'll apply it, thanks Herbert.

>  
> > I am planning to test and submit a patch to SELinux
> > to invoke flow_cache_flush() on policy reloads tomorrow.
> 
> flow_cache_flush() shouldn't be used by SELinux.  You probably
> want to increase the genid instead.

That's what I recommend too.  Active flushes are very bad for
performance, passive garbage collection of stale entries should
be used for coherency whenever possible.

> BTW, we should probably add some code to deal with overflows in
> genid.  With the recent improvements in policy update speeds, it
> could be conceivable for an overflow to occur.

Even if you had a very powerful computer, doing nothing but
inserting and deleting policies, it'd take around 90 days for
the generation count to overflow.

But of course it could happen.

One thing we could do is force a full flow cache flush when some
percentage before the generation count would overflow.  Say every
%80 of the generation ID space?  That gives enough space to ensure
the flush tasklet runs before the ID really does wrap.

Re: [IPSEC] flow: Cache negative results

[Herbert Xu]

On Wed, Jan 10, 2007 at 10:06:05PM -0800, David Miller wrote:
> 
> One thing we could do is force a full flow cache flush when some
> percentage before the generation count would overflow.  Say every
> %80 of the generation ID space?  That gives enough space to ensure
> the flush tasklet runs before the ID really does wrap.

Yes that would be a good solution.  It'd prevent overflows from causing
any damage and is also pretty light weight (just a single comparison
every time the genid changes).

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [IPSEC] flow: Cache negative results

[Paul Moore]

On Wednesday, January 10 2007 2:22 am, Herbert Xu wrote:
> Hi:
>
> [IPSEC] flow: Cache negative security checks

Hi Herbert,

I'm far from a flow cache expert (David, James, and Venkat will probably be 
able to give you much better feedback) I did notice a few things which may or 
may not be issues ... comments below.  FWIW, I believe Venkat is right in 
that it shouldn't have any effect on the LSM code.

However, there is one other thing that came up when I was looking at your 
patch, unrelated to your changes, that I wanted to ask about: in 
net/core/flow.c:flow_entry_kill() ...

> static void flow_entry_kill(int cpu, struct flow_cache_entry *fle)
> {
>        if (fle->object)

Should this be "if (fle->object_ref)"?

>                atomic_dec(fle->object_ref);
>        kmem_cache_free(flow_cachep, fle);
>        flow_count(cpu)--; 
> }

Your changes:

> Index: net-2.6_review/net/core/flow.c
> ===================================================================
> --- net-2.6_review.orig/net/core/flow.c
> +++ net-2.6_review/net/core/flow.c
> @@ -197,12 +197,16 @@ void *flow_cache_lookup(struct flowi *ke
>                         if (fle->genid == atomic_read(&flow_cache_genid)) {
>                                 void *ret = fle->object;
> 
> -                               if (ret)
> +                               if (fle->object_ref)
>                                         atomic_inc(fle->object_ref);
>                                 local_bh_enable();
> 
>                                 return ret;
>                         }

This is a real nit, but would it be better to get rid of the "ret" variable 
entirely and simply "return (void *)fle->object" while you're at it?

> +
> +                       if (fle->object_ref)
> +                               atomic_dec(fle->object_ref);
> +                       fle->object_ref = NULL;
>                         break;

It seems that we could potentially save an operation in some cases by moving 
the "fle->object_ref = NULL" statement inside the if block, for example:

 if (fle->object_ref) {
         atomic_dec(fle->object_ref);
         fle->object_ref = NULL;
 }

> @@ -230,28 +234,20 @@ nocache:
>                 atomic_t *obj_ref;
> 
>                 err = resolver(key, family, dir, &obj, &obj_ref);
> +               if (err)
> +                       obj = ERR_PTR(err);
> 
>                 if (fle) {
> -                       if (err) {
> -                               /* Force security policy check on next ...
> -                               *head = fle->next;
> -                               flow_entry_kill(cpu, fle);
> -                       } else {
> -                               fle->genid = atomic_read(&flow_cache_genid);
> +                       fle->object = obj;
> +                       fle->genid = atomic_read(&flow_cache_genid);
> 
> -                               if (fle->object)
> -                                       atomic_dec(fle->object_ref);
> -
> -                               fle->object = obj;
> +                       if (!err && obj) {

Should this if statement be changed to "if (!err && obj_ref) {"?

>                                 fle->object_ref = obj_ref;
> -                               if (obj)
> -                                       atomic_inc(fle->object_ref);
> +                               atomic_inc(obj_ref);
>                         }
>                 }
>                 local_bh_enable();
> 
> -               if (err)
> -                       obj = ERR_PTR(err);
>                 return obj;
>         }
>  }

-- 
paul moore
linux security @ hp

Re: [IPSEC] flow: Cache negative results

[Herbert Xu]

Hi Paul:

On Wed, Jan 10, 2007 at 11:11:43AM -0500, Paul Moore wrote:
> 
> I'm far from a flow cache expert (David, James, and Venkat will probably be 
> able to give you much better feedback) I did notice a few things which may or 
> may not be issues ... comments below.  FWIW, I believe Venkat is right in 
> that it shouldn't have any effect on the LSM code.
> 
> However, there is one other thing that came up when I was looking at your 
> patch, unrelated to your changes, that I wanted to ask about: in 
> net/core/flow.c:flow_entry_kill() ...
> 
> > static void flow_entry_kill(int cpu, struct flow_cache_entry *fle)
> > {
> >        if (fle->object)
> 
> Should this be "if (fle->object_ref)"?
> 
> >                atomic_dec(fle->object_ref);
> >        kmem_cache_free(flow_cachep, fle);
> >        flow_count(cpu)--; 
> > }

Yep this is a real bug.  Thanks for spotting it!

Although with Venkat's suggestion this change is no longer needed.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt