linux-kernel - Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

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]

Date:	Thu, 1 May 2014 10:53:37 -0400
From:	Tejun Heo <tj@...nel.org>
To:	Shuah Khan <shuah.kh@...sung.com>
Cc:	gregkh@...uxfoundation.org, m.chehab@...sung.com, olebowle@....com,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH 1/4] drivers/base: add managed token devres interfaces

Hello,

On Tue, Apr 29, 2014 at 01:49:23PM -0600, Shuah Khan wrote:
> +/* creates a token devres and marks it available */
> +int devm_token_create(struct device *dev, const char *id)
> +{
> +	struct token_devres *tkn;
> +	size_t tkn_size;
> +
> +	tkn_size = sizeof(struct token_devres) + strlen(id) + 1;
> +	tkn = devres_alloc(devm_token_release, tkn_size, GFP_KERNEL);
> +	if (!tkn)
> +		return -ENOMEM;

Is nesting devres inside devres really necessary?  I think it should
work but why do it this way?  Just kzalloc here and free from release.

> +
> +	strcpy(tkn->id, id);
> +	tkn->in_use = false;
> +	mutex_init(&tkn->lock);
> +
> +	devres_add(dev, tkn);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_token_create);
> +
> +/* If token is available, lock it for the caller, If not return -EBUSY */
> +int devm_token_lock(struct device *dev, const char *id)

trylock probably is a more apt name.

> +{
> +	struct token_devres *tkn_ptr;
> +	int rc = 0;
> +
> +	tkn_ptr = devres_find(dev, devm_token_release, devm_token_match,
> +				(void *)id);
> +	if (tkn_ptr == NULL)
> +		return -ENODEV;
> +
> +	if (!mutex_trylock(&tkn_ptr->lock))
> +		return -EBUSY;

How is this lock supposed to be used?  Have you tested it with lockdep
enabled?  Does it ever get released by a task which is different from
the one which locked it?  If the lock ownership is really about driver
association rather than tasks, it might be necessary to nullify
lockdep protection and add your own annotation to at least track that
unlocking driver (identified how? maybe impossible?) actually owns the
lock.

> +	if (tkn_ptr->in_use)
> +		rc = -EBUSY;
> +	else
> +		tkn_ptr->in_use = true;

Wat?  Why would you have in_use protected by trylock?  What's the
reasonsing behind that?  What would you need "try"lock there?  Okay,
strick everything I wrote above.

 Nacked-by: Tejun Heo <tj@...nel.org>

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/