linux-kernel - Re: [PATCH] genalloc: add support of named pool

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: <20111216164243.01c41930.akpm@linux-foundation.org>
Date:	Fri, 16 Dec 2011 16:42:43 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] genalloc: add support of named pool

On Thu,  8 Dec 2011 16:50:08 +0100
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com> wrote:

> so we can get the pool in the driver by name instead of passing it via
> parameter

I'm not seeing any code whcih actually uses this interface, so there's
no reason to apply the patch?

The changelog should tell us why you believe this patch is needed: use
case, end-user value, etc.

> --- a/include/linux/genalloc.h
> +++ b/include/linux/genalloc.h
> @@ -33,9 +33,12 @@
>   *  General purpose special memory pool descriptor.
>   */
>  struct gen_pool {
> +	char *name;			/* pool name */
>  	spinlock_t lock;
>  	struct list_head chunks;	/* list of chunks in this pool */
>  	int min_alloc_order;		/* minimum allocation order */
> +
> +	struct list_head list;

The other fields have nice descriptive comments - why shouldn't this one?

> +static LIST_HEAD(pools);

Except for certain special circumstances (which I don't think apply
here), a global list needs a global lock to protect it.

> +
>  static int set_bits_ll(unsigned long *addr, unsigned long mask_to_set)
>  {
>  	unsigned long val, nval;
> @@ -136,23 +138,50 @@ static int bitmap_clear_ll(unsigned long *map, int start, int nr)
>  }
>  
>  /**
> + * gen_pool_byname - get pool by name
> + * @name: pool name
> + *

Remove that empty comment line.

> + */
> +struct gen_pool* gen_pool_byname(char *name)

The patch has several minor coding-style errors which can be detected
by scripts/checkpatch.pl.

> +{
> +	struct gen_pool *pool;
> +
> +	if (!name)
> +		return NULL;
> +
> +	list_for_each_entry(pool, &pools, list) {
> +		if(pool->name && strcmp(pool->name, name) == 0)
> +			return pool;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(gen_pool_byname);

This function is racy and cannot be fixed.  If some other process comes
along and deletes *pool immediately after this function has completed,
the caller of this function is left holding a dead pointer.


--
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/