linux-kernel - Re: [PATCH 09/30] v4l: vxd-dec: Add idgen api modules

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:   Tue, 24 Aug 2021 17:00:06 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     sidraya.bj@...hpartnertech.com
Cc:     gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, prashanth.ka@...hpartnertech.com,
        praneeth@...com, mchehab@...nel.org, linux-media@...r.kernel.org,
        praveen.ap@...hpartnertech.com
Subject: Re: [PATCH 09/30] v4l: vxd-dec: Add idgen api modules

Of course this is all staging code and we normally don't review staging
code too hard when it's first sent because we understand that staging
code needs a lot of work before it's acceptable.

One of the rules of staging is that you can't touch code outside of
staging.

But since I happened to glance at a some of this code then here are
some tiny comments:

On Wed, Aug 18, 2021 at 07:40:16PM +0530, sidraya.bj@...hpartnertech.com wrote:
> +/*
> + * Structure contains the ID context.
> + */
> +struct idgen_hdblk {
> +	void **link; /* to be part of single linked list */
> +	/* Array of handles in this block. */
> +	void *ahhandles[1];

Don't use 1 element flex arrays.  Do it like this:

	void *ahhandles[];

You will have to adjust all you math.  But you should be using the
"struct_size(hdblk, ahhandles, nr)" macro anyway to prevent integer
overflows.

> +int idgen_createcontext(unsigned int maxid, unsigned int blksize,
> +			int incid, void **idgenhandle)
> +{
> +	struct idgen_context *idcontext;
> +
> +	/* Create context structure */
> +	idcontext = kzalloc(sizeof(*idcontext), GFP_KERNEL);
> +	if (!idcontext)
> +		return IMG_ERROR_OUT_OF_MEMORY;

We need to get rid of all these weird error codes?  You can add that
to the TODO file.

> +int idgen_destroycontext(void *idgenhandle)
> +{
> +	struct idgen_context *idcontext = (struct idgen_context *)idgenhandle;
> +	struct idgen_hdblk *hdblk;
> +
> +	if (!idcontext)
> +		return IMG_ERROR_INVALID_PARAMETERS;
> +
> +	/* If incrementing Ids, free the List of Incrementing Ids */
> +	if (idcontext->incids) {
> +		struct idgen_id *id;
> +		unsigned int i = 0;
> +
> +		for (i = 0; i < idcontext->blksize; i++) {
> +			id = lst_removehead(&idcontext->incidlist[i]);

You're not allowed to invent your own list API. :P  Generally there just
seems like too much extra helper functions and wrappers.  Removing this
kind of stuff is standard for staging drivers so that's normall.  Add it
to the TODO.

regards,
dan carpenter