netdev - Re: [patch 1/3] netlink: allocate group bitmaps dynamically

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 17 Jul 2007 17:45:53 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Johannes Berg <johannes@...solutions.net>
CC:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	Zhang Rui <rui.zhang@...el.com>, jamal <hadi@...erus.ca>
Subject: Re: [patch 1/3] netlink: allocate group bitmaps dynamically

Johannes Berg wrote:
> Allow changing the number of groups for a netlink family
> after it has been created, use RCU to protect the listeners
> bitmap keeping netlink_has_listeners() lock-free.
> 
> Signed-off-by: Johannes Berg <johannes@...solutions.net>
> 
> ---
>  include/linux/netlink.h  |    1 
>  net/netlink/af_netlink.c |   86 +++++++++++++++++++++++++++++++++++------------
>  2 files changed, 66 insertions(+), 21 deletions(-)
> 
> --- wireless-dev.orig/net/netlink/af_netlink.c	2007-07-17 14:05:30.210964463 +0200
> +++ wireless-dev/net/netlink/af_netlink.c	2007-07-17 14:05:30.720964463 +0200
> -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();


This is actually a bug in the current code I think, netlink_lock_table
is a reader lock.

>  	groups = nl_table[sk->sk_protocol].groups;
>  	if (!nl_table[sk->sk_protocol].registered)
>  		err = -ENOENT;
> -	netlink_unlock_table();
>  
>  	if (err)
> -		return err;
> +		goto out_unlock;
>  
> -	nlk->groups = kzalloc(NLGRPSZ(groups), GFP_KERNEL);
> -	if (nlk->groups == NULL)
> -		return -ENOMEM;
> +	if (nlk->ngroups >= groups)
> +		goto out_unlock;
> +
> +	new_groups = krealloc(nlk->groups, NLGRPSZ(groups), GFP_KERNEL);
> +	if (new_groups == NULL) {
> +		err = -ENOMEM;
> +		goto out_unlock;
> +	}
> +	memset((char*)new_groups + NLGRPSZ(nlk->ngroups), 0,
> +	       NLGRPSZ(groups) - NLGRPSZ(nlk->ngroups));
> +
> +	nlk->groups = new_groups;
>  	nlk->ngroups = groups;
> -	return 0;
> + out_unlock:
> +	netlink_unlock_table();
> +	return err;
>  }


> +int netlink_change_ngroups(int unit, unsigned int groups)


I think it would be more consistent to pass the kernel socket
instead of the unit.

> +{
> +	unsigned long *listeners, old = NULL;
> +	int err = 0;
> +
> +	netlink_table_grab();
> +	if (NLGRPSZ(nl_table[unit].groups) < NLGRPSZ(groups)) {
> +		listeners = kzalloc(NLGRPSZ(groups), GFP_ATOMIC);
> +		if (!listeners) {
> +			err = -ENOMEM;
> +			goto out_ungrab;
> +		}
> +		old = nl_table[unit].listeners;
> +		memcpy(listeners, old, NLGRPSZ(nl_table[unit].groups));
> +		rcu_assign_pointer(nl_table[unit].listeners, listeners);
> +	}
> +	nl_table[unit].groups = groups;


This might set the group to a value < 32. I don't expect it matters,
but when I changed to old code to support > 32 groups I enforced
a minimum of 32 so anything outside the kernel multicasting on them
would still work (even though its a really stupid idea). So for
consistency this should probably also use a minimum of 32.

-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html