Re: [PATCH] drivers: misc: goldfish_address_space: add a driver

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Jan 03, 2019 at 06:13:11PM -0800, rkir@google.com wrote:
> +static int as_release(struct inode *inode, struct file *filp)
> +{
> +	struct as_allocated_blocks *allocated_blocks = filp->private_data;
> +	struct as_device_state *state;
> +	int blocks_size;
> +	int i;
> +
> +	WARN_ON(!allocated_blocks);
> +	WARN_ON(!allocated_blocks->state);
> +	WARN_ON(!allocated_blocks->blocks);
> +	WARN_ON(allocated_blocks->blocks_size < 0);
> +
> +	state = allocated_blocks->state;
> +	blocks_size = allocated_blocks->blocks_size;
> +
> +	if (mutex_lock_interruptible(&state->registers_lock))

I just took this chunk of code as an example of what you do all over
this file.  Please do not use WARN_ON() as a lazy way of saying "I have
no idea how to handle this random error that might happen, so I'm going
to punt to the user and crash the machine."

If these things really can happen, then properly check for them and
handle the error correctly.  If they can not, then just remove the
WARN_ON check as it is not needed at all.

As it is, this code is obviously broken because if
allocated_blocks->state is NULL, you just crashed on the line after the
check.  So even if you did somehow want to "warn" for something like
this happening, you did not handle it and killed the machine.

All of the WARN_ON can be removed here as I bet you are testing for
things that can never happen.  And if it could happen, then properyl
test for it.

as-is, this code is not ok at all.

Also, along these lines, who else is reviewing this code before you send
it out?  Surely you are not reyling on just me to do that, you are
taking advantage of the huge numbers of reviewers inside your company
that could have told you this before posting it, right?

Please do so.

greg k-h