Re: [PATCH] netlink: allocate group bitmaps dynamically

[Patrick McHardy <kaber@trash.net>]

Johannes Berg wrote:
> On Tue, 2007-07-03 at 14:05 +0200, Patrick McHardy wrote:
> 
> 
>>>--- wireless-dev.orig/net/netlink/af_netlink.c	2007-07-03 00:10:31.617889695 +0200
>>>+++ wireless-dev/net/netlink/af_netlink.c	2007-07-03 00:31:30.267889695 +0200
>>>@@ -316,8 +316,11 @@ netlink_update_listeners(struct sock *sk
>>> 
>>> 	for (i = 0; i < NLGRPSZ(tbl->groups)/sizeof(unsigned long); i++) {
>>> 		mask = 0;
>>>-		sk_for_each_bound(sk, node, &tbl->mc_list)
>>>-			mask |= nlk_sk(sk)->groups[i];
>>>+		sk_for_each_bound(sk, node, &tbl->mc_list) {
>>>+			if (nlk_sk(sk)->ngroups >=
>>>+			    (i + 1) * sizeof(unsigned long))
>>
>>
>>This condition implies that a socket can bind to a non-existant
>>group, which shouldn't be possible.
> 
> 
> Actually, it's the other way around, the socket can bind to group 10
> only 32 groups are present (one unsigned long) and then some other code
> goes to add groups increasing the limit to 64, and then the socket still
> only has a bitmap with 32 bits (one unsigned long) and we shouldn't
> access beyond that just because the number of groups was increased.


You're right, I misread this code.

> However, you can in fact bind non-existing groups as long as the group
> number is lower than the maximum, i.e. if you start out with just one
> group as genetlink does, the netlink code increases that to 32 and you
> can bind group 25 even if generic netlink doesn't know about it yet. I
> plan to fix that when it actually matters, i.e. when I introduce
> per-group permission checks.


Yes, I was thinking of groups > 32.

> 
> 
>>>+				mask |= nlk_sk(sk)->groups[i];
>>>+		}
>>> 		tbl->listeners[i] = mask;
>>> 	}
>>> 	/* this function is only called with the netlink table "grabbed", which
>>>@@ -555,10 +558,11 @@ netlink_update_subscriptions(struct sock
>>> 	nlk->subscriptions = subscriptions;
>>> }
>>> 
>>>-static int netlink_alloc_groups(struct sock *sk)
>>>+static int netlink_realloc_groups(struct sock *sk)
>>> {
>>> 	struct netlink_sock *nlk = nlk_sk(sk);
>>> 	unsigned int groups;
>>>+	unsigned long *new_groups;
>>> 	int err = 0;
>>> 
>>> 	netlink_lock_table();
>>>@@ -570,9 +574,15 @@ static int netlink_alloc_groups(struct s
>>> 	if (err)
>>> 		return err;
>>> 
>>>-	nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
>>>-	if (nlk->groups == NULL)
>>>+	if (nlk->ngroups >= groups)
>>>+		return 0;
>>>+
>>>+	new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
>>>+	if (new_groups == NULL)
>>> 		return -ENOMEM;
>>>+	memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
>>>+	       NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
>>>+	nlk->groups = new_groups;
>>
>>
>>This should probably happen with the table grabbed to avoid races
>>with concurrent broadcasts.
> 
> 
> Hmm, possibly, I'll have to look again.


do_one_broadcast locks the table and checks nlk->groups. The
reallocation races with this without taking the lock or maybe
using rcu.

> 
> 
>>>+int netlink_change_ngroups(int unit, unsigned int groups)
>>>+{
>>>+	unsigned long *listeners;
>>>+	int err = 0;
>>>+
>>>+	netlink_table_grab();
>>
>>
>>Unfortunately that doesn't prevent races with netlink_has_listeners
>>since its lockless (and should really stay that way).
> 
> 
> Uh huh. Good point, I guess I'll have to use RCU or such here when
> changing the listeners bitmap size.


That should work.