linux-kernel - Re: [RFC][PATCH] flexible array implementation v3

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, 23 Jul 2009 10:44:50 +0900
From:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
To:	Dave Hansen <dave@...ux.vnet.ibm.com>
Cc:	akpm@...ux-foundation.org, containers@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org, menage@...gle.com,
	vda.linux@...glemail.com, mikew@...gle.com, lizf@...fujitsu.com,
	xiyou.wangcong@...il.com
Subject: Re: [RFC][PATCH] flexible array implementation v3

On Wed, 22 Jul 2009 10:53:45 -0700
Dave Hansen <dave@...ux.vnet.ibm.com> wrote:

> 
> 
> Changes from v2:
> - renamed some of the index functions
> - added preallocation function
> - added flex_array_free_parts() for use with
>   statically allocated bases
> - killed append() function
> 
> Changes from v1:
> - to vs too typo
> - added __check_part_and_nr() and gave it a warning
> - fixed off-by-one check on __nr_part_ptrs()
> - added FLEX_ARRAY_INIT() macro
> - some kerneldoc comments about the capacity
>   with various sized objects
> - comments to note lack of locking semantice
> 

Seems nice, thank you. but a nitpick..

+struct flex_array *flex_array_alloc(int element_size, int total, gfp_t flags)
+{
+	struct flex_array *ret;
+	int max_size = nr_base_part_ptrs() * __elements_per_part(element_size);
+
+	/* max_size will end up 0 if element_size > PAGE_SIZE */
+	if (total > max_size)
+		return NULL;

Can't we store "total" somewhere and do error check ?

And some magic value as following can't be defined for 'total' ?

#define FLEX_ARRAY_MAX_ELEMENTS		(-1) // Use maximum flex array.


> +/**
> + * flex_array_get - pull data back out of the array
> + * @element_nr:	index of the element to fetch from the array
> + *
> + * Returns a pointer to the data at index @element_nr.  Note
> + * that this is a copy of the data that was passed in.  If you
> + * are using this to store pointers, you'll get back &ptr.
> + *
> + * Locking must be provided by the caller.
> + */
> +void *flex_array_get(struct flex_array *fa, int element_nr)
> +{
> +	int part_nr = fa_element_to_part_nr(fa, element_nr);
> +	struct flex_array_part *part;
> +	int offset;
> +
> +	if (__check_part_nr(fa, part_nr))
> +		return NULL;
return -EINVAL, here ?
(Can't we compared with stored "total" ?)

> +	if (!fa->parts[part_nr])
> +		return NULL;
> +

The caller can't know whether
 - there are no entry or
 - NULL(0) is stored at the array.

Then, he has to check gotten value is valid or not by himself.

Can't we return -ENOENT here(especially when prealloc() is called) ?
But ah, anyway, all-zero elements in allocated array exists ;(
and the user can get value from an entry never be put.

If we can fill the first 4bytes of _unused_ index by some magic code like this
#define FLEX_ARRAY_UNUSED_MAGIC		(0xa5a5a5a5)
(if maintaining bitmap/status of usage is nonsense)
and flex_array_get() can return -ENOENT, the users will feel easy.

Overprotection ;) ?


Thanks,
-Kame

> +	part = fa->parts[part_nr];
> +	offset = index_inside_part(fa, element_nr);
> +	return &part->elements[index_inside_part(fa, element_nr)];
> +}








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