diff mbox series

cipso,calipso: resolve a number of problems with the DOI refcounts

Message ID 161489339182.63157.2775083878484465675.stgit@olly (mailing list archive)
State Accepted
Headers show
Series cipso,calipso: resolve a number of problems with the DOI refcounts | expand

Commit Message

Paul Moore March 4, 2021, 9:29 p.m. UTC 
The current CIPSO and CALIPSO refcounting scheme for the DOI
definitions is a bit flawed in that we:

1. Don't correctly match gets/puts in netlbl_cipsov4_list().
2. Decrement the refcount on each attempt to remove the DOI from the
   DOI list, only removing it from the list once the refcount drops
   to zero.

This patch fixes these problems by adding the missing "puts" to
netlbl_cipsov4_list() and introduces a more conventional, i.e.
not-buggy, refcounting mechanism to the DOI definitions.  Upon the
addition of a DOI to the DOI list, it is initialized with a refcount
of one, removing a DOI from the list removes it from the list and
drops the refcount by one; "gets" and "puts" behave as expected with
respect to refcounts, increasing and decreasing the DOI's refcount by
one.

Fixes: b1edeb102397 ("netlabel: Replace protocol/NetLabel linking with refrerence counts")
Fixes: d7cce01504a0 ("netlabel: Add support for removing a CALIPSO DOI.")
Reported-by: syzbot+9ec037722d2603a9f52e@syzkaller.appspotmail.com
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 net/ipv4/cipso_ipv4.c            |   11 +----------
 net/ipv6/calipso.c               |   14 +++++---------
 net/netlabel/netlabel_cipso_v4.c |    3 +++
 3 files changed, 9 insertions(+), 19 deletions(-)

Comments

Paul Moore March 4, 2021, 9:32 p.m. UTC | #1 
On Thu, Mar 4, 2021 at 4:29 PM Paul Moore <paul@paul-moore.com> wrote:
>
> The current CIPSO and CALIPSO refcounting scheme for the DOI
> definitions is a bit flawed in that we:
>
> 1. Don't correctly match gets/puts in netlbl_cipsov4_list().
> 2. Decrement the refcount on each attempt to remove the DOI from the
>    DOI list, only removing it from the list once the refcount drops
>    to zero.
>
> This patch fixes these problems by adding the missing "puts" to
> netlbl_cipsov4_list() and introduces a more conventional, i.e.
> not-buggy, refcounting mechanism to the DOI definitions.  Upon the
> addition of a DOI to the DOI list, it is initialized with a refcount
> of one, removing a DOI from the list removes it from the list and
> drops the refcount by one; "gets" and "puts" behave as expected with
> respect to refcounts, increasing and decreasing the DOI's refcount by
> one.
>
> Fixes: b1edeb102397 ("netlabel: Replace protocol/NetLabel linking with refrerence counts")
> Fixes: d7cce01504a0 ("netlabel: Add support for removing a CALIPSO DOI.")
> Reported-by: syzbot+9ec037722d2603a9f52e@syzkaller.appspotmail.com
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  net/ipv4/cipso_ipv4.c            |   11 +----------
>  net/ipv6/calipso.c               |   14 +++++---------
>  net/netlabel/netlabel_cipso_v4.c |    3 +++
>  3 files changed, 9 insertions(+), 19 deletions(-)

As a FYI, this patch has been tested by looping through a number of
NetLabel/CALIPSO/CIPSO tests overnight, a reproducer from one of the
syzbot reports (multiple times), and the selinux-testsuite tests;
everything looked good at the end of the testing.

Thanks to syzbot and Dmitry for finding and reporting the bug.
David Miller March 4, 2021, 10:33 p.m. UTC | #2 
From: Paul Moore <paul@paul-moore.com>
Date: Thu, 04 Mar 2021 16:29:51 -0500

> +static void calipso_doi_putdef(struct calipso_doi *doi_def);
> +

This is a global symbol, so why the static decl here?

Thanks.
Paul Moore March 4, 2021, 11:13 p.m. UTC | #3 
On Thu, Mar 4, 2021 at 5:33 PM David Miller <davem@davemloft.net> wrote:
> From: Paul Moore <paul@paul-moore.com>
> Date: Thu, 04 Mar 2021 16:29:51 -0500
>
> > +static void calipso_doi_putdef(struct calipso_doi *doi_def);
> > +
>
> This is a global symbol, so why the static decl here?

To resolve this:

  CC      net/ipv6/calipso.o
net/ipv6/calipso.c: In function ‘calipso_doi_remove’:
net/ipv6/calipso.c:453:2: error: implicit declaration of function ‘calipso_doi_p
utdef’

I think there are some odd things with how the CALIPSO prototypes are
handled, some of that I'm guessing is due to handling IPv6
as-a-module, but regardless of the reason it seemed like the smallest
fix was to add the forward declaration at the top of the file.
Considering that I believe this should be sent to -stable I figured a
smaller patch, with less chance for merge conflicts, would be more
desirable.

Or are you simply concerned about the static tag?  I simply kept the
static from the function definition; removing it causes a mismatch
which makes the compiler unhappy.

Either way, if you want this done another way, let me know what you
want and I'll respin it.
David Miller March 4, 2021, 11:27 p.m. UTC | #4 
From: Paul Moore <paul@paul-moore.com>
Date: Thu, 4 Mar 2021 18:13:21 -0500

> On Thu, Mar 4, 2021 at 5:33 PM David Miller <davem@davemloft.net> wrote:
>> From: Paul Moore <paul@paul-moore.com>
>> Date: Thu, 04 Mar 2021 16:29:51 -0500
>>
>> > +static void calipso_doi_putdef(struct calipso_doi *doi_def);
>> > +
>>
>> This is a global symbol, so why the static decl here?
> 
> To resolve this:
> 
>   CC      net/ipv6/calipso.o
> net/ipv6/calipso.c: In function ‘calipso_doi_remove’:
> net/ipv6/calipso.c:453:2: error: implicit declaration of function ‘calipso_doi_p
> utdef’
> 
> I think there are some odd things with how the CALIPSO prototypes are
> handled, some of that I'm guessing is due to handling IPv6
> as-a-module, but regardless of the reason it seemed like the smallest
> fix was to add the forward declaration at the top of the file.
> Considering that I believe this should be sent to -stable I figured a
> smaller patch, with less chance for merge conflicts, would be more
> desirable.

Thanks for explaining...
patchwork-bot+netdevbpf@kernel.org March 4, 2021, 11:30 p.m. UTC | #5 
Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Thu, 04 Mar 2021 16:29:51 -0500 you wrote:
> The current CIPSO and CALIPSO refcounting scheme for the DOI
> definitions is a bit flawed in that we:
> 
> 1. Don't correctly match gets/puts in netlbl_cipsov4_list().
> 2. Decrement the refcount on each attempt to remove the DOI from the
>    DOI list, only removing it from the list once the refcount drops
>    to zero.
> 
> [...]

Here is the summary with links:
  - cipso,calipso: resolve a number of problems with the DOI refcounts
    https://git.kernel.org/netdev/net/c/ad5d07f4a9cd

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
diff mbox series

Patch

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 471d33a0d095..be09c7669a79 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -519,16 +519,10 @@  int cipso_v4_doi_remove(u32 doi, struct netlbl_audit *audit_info)
 		ret_val = -ENOENT;
 		goto doi_remove_return;
 	}
-	if (!refcount_dec_and_test(&doi_def->refcount)) {
-		spin_unlock(&cipso_v4_doi_list_lock);
-		ret_val = -EBUSY;
-		goto doi_remove_return;
-	}
 	list_del_rcu(&doi_def->list);
 	spin_unlock(&cipso_v4_doi_list_lock);
 
-	cipso_v4_cache_invalidate();
-	call_rcu(&doi_def->rcu, cipso_v4_doi_free_rcu);
+	cipso_v4_doi_putdef(doi_def);
 	ret_val = 0;
 
 doi_remove_return:
@@ -585,9 +579,6 @@  void cipso_v4_doi_putdef(struct cipso_v4_doi *doi_def)
 
 	if (!refcount_dec_and_test(&doi_def->refcount))
 		return;
-	spin_lock(&cipso_v4_doi_list_lock);
-	list_del_rcu(&doi_def->list);
-	spin_unlock(&cipso_v4_doi_list_lock);
 
 	cipso_v4_cache_invalidate();
 	call_rcu(&doi_def->rcu, cipso_v4_doi_free_rcu);
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 51184a70ac7e..1578ed9e97d8 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -83,6 +83,9 @@  struct calipso_map_cache_entry {
 
 static struct calipso_map_cache_bkt *calipso_cache;
 
+static void calipso_cache_invalidate(void);
+static void calipso_doi_putdef(struct calipso_doi *doi_def);
+
 /* Label Mapping Cache Functions
  */
 
@@ -444,15 +447,10 @@  static int calipso_doi_remove(u32 doi, struct netlbl_audit *audit_info)
 		ret_val = -ENOENT;
 		goto doi_remove_return;
 	}
-	if (!refcount_dec_and_test(&doi_def->refcount)) {
-		spin_unlock(&calipso_doi_list_lock);
-		ret_val = -EBUSY;
-		goto doi_remove_return;
-	}
 	list_del_rcu(&doi_def->list);
 	spin_unlock(&calipso_doi_list_lock);
 
-	call_rcu(&doi_def->rcu, calipso_doi_free_rcu);
+	calipso_doi_putdef(doi_def);
 	ret_val = 0;
 
 doi_remove_return:
@@ -508,10 +506,8 @@  static void calipso_doi_putdef(struct calipso_doi *doi_def)
 
 	if (!refcount_dec_and_test(&doi_def->refcount))
 		return;
-	spin_lock(&calipso_doi_list_lock);
-	list_del_rcu(&doi_def->list);
-	spin_unlock(&calipso_doi_list_lock);
 
+	calipso_cache_invalidate();
 	call_rcu(&doi_def->rcu, calipso_doi_free_rcu);
 }
 
diff --git a/net/netlabel/netlabel_cipso_v4.c b/net/netlabel/netlabel_cipso_v4.c
index 726dda95934c..4f50a64315cf 100644
--- a/net/netlabel/netlabel_cipso_v4.c
+++ b/net/netlabel/netlabel_cipso_v4.c
@@ -575,6 +575,7 @@  static int netlbl_cipsov4_list(struct sk_buff *skb, struct genl_info *info)
 
 		break;
 	}
+	cipso_v4_doi_putdef(doi_def);
 	rcu_read_unlock();
 
 	genlmsg_end(ans_skb, data);
@@ -583,12 +584,14 @@  static int netlbl_cipsov4_list(struct sk_buff *skb, struct genl_info *info)
 list_retry:
 	/* XXX - this limit is a guesstimate */
 	if (nlsze_mult < 4) {
+		cipso_v4_doi_putdef(doi_def);
 		rcu_read_unlock();
 		kfree_skb(ans_skb);
 		nlsze_mult *= 2;
 		goto list_start;
 	}
 list_failure_lock:
+	cipso_v4_doi_putdef(doi_def);
 	rcu_read_unlock();
 list_failure:
 	kfree_skb(ans_skb);