Received-SPF: pass (google.com: domain of linux-kernel+bounces-181764-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 17 May 2024 14:31:50 +0800
From: ChiYuan Huang <cy_huang@richtek.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
CC: Mauro Carvalho Chehab <mchehab@kernel.org>, Daniel Scally
	<djrscally@gmail.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>,
	<linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] media: v4l: async: Fix NULL pointer when v4l2 flash
 subdev binding
Message-ID: <20240517063150.GA12245@linuxcarl2.richtek.com>
References: <e2f9f2b7b7de956d70b8567a2ab285409fff988b.1715136478.git.cy_huang@richtek.com>
 <ZkXi_U5Js34dUQsA@kekkonen.localdomain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZkXi_U5Js34dUQsA@kekkonen.localdomain>
User-Agent: Mutt/1.5.21 (2010-09-15)

Hi, Sakari:

	Thanks for your reply.
If any misunderstanding, please correct me.

On Thu, May 16, 2024 at 10:42:05AM +0000, Sakari Ailus wrote:
> Hi Chi Yuan,
> 
> On Wed, May 08, 2024 at 10:51:49AM +0800, cy_huang@richtek.com wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> > 
> > In v4l2_async_create_ancillary_links(), if v4l2 async notifier is
> > created from v4l2 device, the v4l2 flash subdev async binding will enter
> > the logic to create media link. Due to the subdev of notifier is NULL,
> > this will cause NULL pointer to access the subdev entity. Therefore, add
> > the check to bypass it.
> > 
> > Fixes: aa4faf6eb271 ("media: v4l2-async: Create links during v4l2_async_match_notify()")
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> > Hi,
> > 
> >   I'm trying to bind the v4l2 subdev for flashlight testing. It seems
> > some logic in v4l2 asynd binding is incorrect.
> > 
> > From the change, I modified vim2m as the test driver to bind mt6370 flashlight.
> > 
> > Here's the backtrace log.
> > 
> >  vim2m soc:vim2m: bound [white:flash-2]
> >  Unable to handle kernel NULL pointer dereference at virtual address 0000000000000058
> >  ......skipping
> >  Call trace:
> >   media_create_ancillary_link+0x48/0xd8 [mc]
> >   v4l2_async_match_notify+0x17c/0x208 [v4l2_async]
> >   v4l2_async_register_subdev+0xb8/0x1d0 [v4l2_async]
> 
> There's something wrong obviously somewhere but wherea?
> 
In vim2m driver, I added v4l2_async_nf_init -> v4l2_async_nf_add_fwnode_remote ->
v4l2_async_nf_register.

From the async flow, in notifier complete ops to create v4l-subdevX node for the 
specified subdev.
> A sub-notifier does have a sub-device after the notifier initialisation.

Why? Are you saying to the notifier can only be used for subdev and subdev binding, 
not v4l2 and subdev binding?

But to create v4l-subdevX, the key is only v4l2 device and its needed subdev.

> Maybe the initialisation does not happen in the right order?
AFAIK, Async flow can solve the probe order and makes the user no need to care
the probe order.

From the stacktrace, I'm pretty sure it's not the probe order issue.
> 
> >   __v4l2_flash_init.part.0+0x3b4/0x4b0 [v4l2_flash_led_class]
> >   v4l2_flash_init+0x28/0x48 [v4l2_flash_led_class]
> >   mt6370_led_probe+0x348/0x690 [leds_mt6370_flash]
> > 
> > After tracing the code, it will let the subdev labeled as F_LENS or
> > F_FLASH function to create media link. To prevent the NULL pointer
> > issue, the simplest way is add a check when 'n->sd' is NULL and bypass
> > the later media link creataion.
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 3ec323bd528b..9d3161c51954 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -324,6 +324,9 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> >  		return 0;
> >  
> > +	if (!n->sd)
> > +		return 0;
> 
> This isn't the right fix: the ancillary link won't be created as a result.
> 
Due to the notifier is created by v4l2 device not subdev, this 'n->sd' is NULL.
The NULL 'n->sd' will be referenced by the next flow 'media_create_ancillary_link'.

Or is it caused by the wrong usage? 

> > +
> >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> >  
> >  #endif
> 
> -- 
> Regards,
> 
> Sakari Ailus