Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 24 Jun 2021 14:12:52 +0300
From:   Sakari Ailus <sakari.ailus@iki.fi>
To:     Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc:     linuxarm@huawei.com, mauro.chehab@huawei.com,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
Subject: Re: [PATCH 3/5] media: v4l2-flash-led-class: drop an useless check
Message-ID: <20210624111252.GL3@valkosipuli.retiisi.eu>
References: <cover.1624276137.git.mchehab+huawei@kernel.org>
 <e1629ac223470630eed4096361965d154aff70b7.1624276138.git.mchehab+huawei@kernel.org>
 <20210624093153.GJ3@valkosipuli.retiisi.eu>
 <20210624115925.357f98b6@coco.lan>
 <20210624101443.GK3@valkosipuli.retiisi.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210624101443.GK3@valkosipuli.retiisi.eu>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Thu, Jun 24, 2021 at 01:14:43PM +0300, Sakari Ailus wrote:

...

> > > On Mon, Jun 21, 2021 at 01:56:47PM +0200, Mauro Carvalho Chehab wrote:
> > > > As pointed by smatch:
> > > > 	drivers/media/v4l2-core/v4l2-flash-led-class.c:264 v4l2_flash_s_ctrl() error: we previously assumed 'fled_cdev' could be null (see line 197)
> > > > 
> > > > It is too late to check if fled_cdev is NULL there. If such check is
> > > > needed, it should be, instead, inside v4l2_flash_init().
> > > > 
> > > > On other words, if v4l2_flash->fled_cdev() is NULL at
> > > > v4l2_flash_s_ctrl(), all led_*() function calls inside the function
> > > > would try to de-reference a NULL pointer, as the logic won't prevent
> > > > it.
> > > > 
> > > > So, remove the useless check.
> > > > 
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-flash-led-class.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-flash-led-class.c b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > > index 10ddcc48aa17..a1653c635d82 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-flash-led-class.c
> > > > @@ -194,7 +194,7 @@ static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > > >  {
> > > >  	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> > > >  	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > > -	struct led_classdev *led_cdev = fled_cdev ? &fled_cdev->led_cdev : NULL;
> > > > +	struct led_classdev *led_cdev = &fled_cdev->led_cdev;  
> > > 
> > > fled_cdev may be NULL here. The reason is that some controls are for flash
> > > LEDs only but the same sub-device may also control an indicator. This is
> > > covered when the controls are created, so that the NULL pointer isn't
> > > dereferenced.
> > 
> > I double-checked the code: if a a NULL pointer is passed, the calls
> > to the leds framework will try to de-reference it or will return an
> > error.
> > 
> > For instance, those will return an error:
> > 
> > 	static inline int led_set_flash_strobe(struct led_classdev_flash *fled_cdev,
> > 	                                        bool state)
> > 	{
> > 	        if (!fled_cdev)
> > 	                return -EINVAL;
> > 	        return fled_cdev->ops->strobe_set(fled_cdev, state);
> > 	}
> > 
> > 	#define call_flash_op(fled_cdev, op, args...)           \
> > 	        ((has_flash_op(fled_cdev, op)) ?                        \
> > 	                        (fled_cdev->ops->op(fled_cdev, args)) : \
> > 	                        -EINVAL)
> > 
> > No big issue here (except that the function will do nothing but
> > return an error).
> > 
> > However, there are places that it will cause it to de-reference 
> > a NULL pointer:
> > 
> > 	int led_set_brightness_sync(struct led_classdev *led_cdev, unsigned int value)
> > 	{
> > 	        if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
> > 	                return -EBUSY;
> > 	
> > 	        led_cdev->brightness = min(value, led_cdev->max_brightness);
> > 
> > 	        if (led_cdev->flags & LED_SUSPENDED)
> > 	                return 0;
> > 
> > 	        return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
> > 	}
> > 
> > So, this is not a false-positive, but, instead, a real issue.
> > 
> > So, if led_cdev/fled_cdev can indeed be NULL, IMO, the right solution would be
> > to explicitly check it, and return an error, e. g.:
> > 
> > 	static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
> > 	{
> >         	struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
> >         	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > 		struct led_classdev *led_cdev;
> >         	struct v4l2_ctrl **ctrls = v4l2_flash->ctrls;
> >         	bool external_strobe;
> >         	int ret = 0;
> > 
> > 		if (!fled_cdev)
> > 			return -EINVAL;
> 
> The approach is correct, but as noted, the check needs to be done later.

I checked that the same pattern is used throughout much of the file. I
suppose if smatch isn't happy with this instance, it may not be happy with
the rest either. Admittedly, the pattern isn't entirely trouble-free, as it
requires the parts of the file to be in sync.

Addressing this takes probably a few patches at least.

-- 
Sakari Ailus