netdev - Re: [GENETLINK]: Question: global lock (genl

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 linux-cve-announce 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]

Message-ID: <20070816155819.GE32236@postel.suug.ch>
Date:	Thu, 16 Aug 2007 17:58:19 +0200
From:	Thomas Graf <tgraf@...g.ch>
To:	Richard MUSIL <richard.musil@...com>
Cc:	Patrick McHardy <kaber@...sh.net>, netdev@...r.kernel.org
Subject: Re: [GENETLINK]: Question: global lock (genl_mutex) possible refinement?

* Richard MUSIL <richard.musil@...com> 2007-07-24 13:09
> Thomas Graf wrote:
> > Please provide a new overall patch which is not based on your
> > initial patch so I can review your idea properly.
> 
> Here it goes (merging two previous patches). I have diffed
> against v2.6.22, which I am using currently as my base:

Sorry for taking so long.

> @@ -150,9 +176,9 @@ int genl_register_ops(struct genl_family *family, struct genl_ops *ops)
>  	if (ops->policy)
>  		ops->flags |= GENL_CMD_CAP_HASPOL;
>  
> -	genl_lock();
> +	genl_fam_lock(family);
>  	list_add_tail(&ops->ops_list, &family->ops_list);
> -	genl_unlock();
> +	genl_fam_unlock(family);

For registering operations, it is sufficient to just acquire the
family lock, the family itself can't disappear while holding it.

> @@ -216,8 +242,9 @@ int genl_register_family(struct genl_family *family)
>  		goto errout;
>  
>  	INIT_LIST_HEAD(&family->ops_list);
> +	mutex_init(&family->lock);
>  
> -	genl_lock();
> +	genl_fam_lock(family);
>  
>  	if (genl_family_find_byname(family->name)) {
>  		err = -EEXIST;
> @@ -251,14 +278,14 @@ int genl_register_family(struct genl_family *family)
>  		family->attrbuf = NULL;
>  
>  	list_add_tail(&family->family_list, genl_family_chain(family->id));
> -	genl_unlock();
> +	genl_fam_unlock(family);

This looks good.

> @@ -303,38 +332,57 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  	struct genlmsghdr *hdr = nlmsg_data(nlh);
>  	int hdrlen, err;
>  
> +	genl_fam_lock(NULL);
>  	family = genl_family_find_byid(nlh->nlmsg_type);
> -	if (family == NULL)
> +	if (family == NULL) {
> +		genl_fam_unlock(NULL);
>  		return -ENOENT;
> +	}
> +
> +	/* get particular family lock, but release global family lock
> +	 * so registering operations for other families are possible */
> +	genl_onefam_lock(family);
> +	genl_fam_unlock(NULL);

I don't like having two locks for something as trivial as this.
Basically the only reason the global lock is required here is to
protect from family removal which can be avoided otherwise by
using RCU list operations.

Therefore, I'd propose the following lock semantics:
Use own global mutex to protect writing to the family list, make
reading side lockless using rcu for use when looking up family
upon mesage processing. Use a family lock to protect writing to
operations list and serialize messae processing with unregister
operations.
-
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