linux-kernel - Re: [PATCH v2 10/15] staging: vc04_services: bcm2835-isp: Add a more complex ISP processing component

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: <fe7b0a08-941f-4dbc-b89f-d3da0a96ac32@suswa.mountain>
Date:   Mon, 20 Nov 2023 05:33:59 -0500
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Umang Jain <umang.jain@...asonboard.com>
Cc:     linux-media@...r.kernel.org, kernel-list@...pberrypi.com,
        linux-kernel@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-staging@...ts.linux.dev,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Kieran Bingham <kieran.bingham@...asonboard.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        "Ricardo B . Marliere" <ricardo@...liere.net>,
        Dan Carpenter <error27@...il.com>,
        Stefan Wahren <stefan.wahren@...e.com>,
        Naushir Patuck <naush@...pberrypi.com>
Subject: Re: [PATCH v2 10/15] staging: vc04_services: bcm2835-isp: Add a more
 complex ISP processing component

On Thu, Nov 09, 2023 at 04:03:02PM -0500, Umang Jain wrote:
> +static int media_controller_register(struct bcm2835_isp_dev *dev)
> +{
> +	char *name;
> +	unsigned int i;
> +	int ret;
> +
> +	v4l2_dbg(2, debug, &dev->v4l2_dev, "Registering with media controller\n");
> +	dev->mdev.dev = dev->dev;
> +	strscpy(dev->mdev.model, "bcm2835-isp",
> +		sizeof(dev->mdev.model));
> +	strscpy(dev->mdev.bus_info, "platform:bcm2835-isp",
> +		sizeof(dev->mdev.bus_info));
> +	media_device_init(&dev->mdev);
> +	dev->v4l2_dev.mdev = &dev->mdev;
> +
> +	v4l2_dbg(2, debug, &dev->v4l2_dev, "Register entity for nodes\n");
> +
> +	name = kmalloc(BCM2835_ISP_ENTITY_NAME_LEN, GFP_KERNEL);
> +	if (!name) {
> +		ret = -ENOMEM;
> +		goto done;

Oh crap.  This function doesn't clean up after itself, but instead
returns to a One Magical Cleanup Function...  This style of error
handling is *ALWAYS* buggy.  But in this case I only see a very minor
leak.

These two cleanups are under one ->registered flag but they are
allocated separately.  So if we only complete one action and not the
second then the flag is not set and we don't call media_device_cleanup().
(This is one of many typical problems with One Magical Cleanup Function
Style).

> +	if (dev->media_device_registered) {
> +		media_device_unregister(&dev->mdev);
> +		media_device_cleanup(&dev->mdev);
> +		dev->media_device_registered = false;
> +	}

It's just such a headache to review...  (That's why it's so bug prone).

If you wrote it in Free the Last Thing Style then you could get rid
of a bunch of flags like ->media_device_registered and
->media_entity_registered because at that point you would always know
what you had done and hadn't done.

regards,
dan carpenter


> +	}
> +	snprintf(name, BCM2835_ISP_ENTITY_NAME_LEN, "bcm2835_isp0");
> +	dev->entity.name = name;
> +	dev->entity.obj_type = MEDIA_ENTITY_TYPE_BASE;
> +	dev->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
> +
> +	for (i = 0; i < BCM2835_ISP_NUM_NODES; i++) {
> +		dev->pad[i].flags = node_is_output(&dev->node[i]) ?
> +					MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> +	}
> +
> +	ret = media_entity_pads_init(&dev->entity, BCM2835_ISP_NUM_NODES,
> +				     dev->pad);
> +	if (ret)
> +		goto done;
> +
> +	ret = media_device_register_entity(&dev->mdev, &dev->entity);
> +	if (ret)
> +		goto done;
> +
> +	dev->media_entity_registered = true;
> +	for (i = 0; i < BCM2835_ISP_NUM_NODES; i++) {
> 		ret = media_controller_register_node(dev, i);
> +		if (ret)
> +			goto done;
> +	}
> +
> +	ret = media_device_register(&dev->mdev);
> +	if (!ret)
> +		dev->media_device_registered = true;
> +done:
> +	return ret;
> +}