[PATCH 1/1] media: v4l: subdev: Make link validation safer

[PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Link validation currently accesses invalid pointers if the link passed to it
is not between two sub-devices. This is of course a driver bug.

Ignore the error but print a debug message, as this is how it used to work
previously.

Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 1bebcda2bd20c..dd911180ec899 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1209,6 +1209,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
 	struct v4l2_subdev_state *source_state, *sink_state;
 	int ret;
 
+	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
+		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			     link->sink->entity->name);
+		return 0;
+	}
+	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
+		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			     link->source->entity->name);
+		return 0;
+	}
+
 	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
 	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
 
-- 
2.30.2

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

Hi Sakari,

On 09/03/2023 14:27, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 1bebcda2bd20c..dd911180ec899 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1209,6 +1209,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>   	struct v4l2_subdev_state *source_state, *sink_state;
>   	int ret;
>   
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->sink->entity->name);
> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->source->entity->name);
> +		return 0;
> +	}
> +
>   	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>   	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>   

I think this is supposed to be v2. You missed my RB, but here it is again:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Also, the commit message says "debug message", but it's a warn now.

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 02:34:28PM +0200, Tomi Valkeinen wrote:
> I think this is supposed to be v2. You missed my RB, but here it is again:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> Also, the commit message says "debug message", but it's a warn now.

Thanks, I'll address this while applying it.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 02:27:16PM +0200, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

This was intended to be v2. V1 is here:

<URL:https://lore.kernel.org/linux-media/ZAIryJV7XNpW8lbY@kekkonen.localdomain/T/#m12c7e39e154a799b1034388d0c63ce0f65979a1b>

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi Sakari, 

On 3/9/23 13:27, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

FWIW you have my Reported-by in there twice now ...

Regards,

Hans




> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 1bebcda2bd20c..dd911180ec899 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1209,6 +1209,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  	struct v4l2_subdev_state *source_state, *sink_state;
>  	int ret;
>  
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->sink->entity->name);
> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->source->entity->name);
> +		return 0;
> +	}
> +
>  	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>  	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>  

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 01:43:50PM +0100, Hans de Goede wrote:
> Hi Sakari, 
> 
> On 3/9/23 13:27, Sakari Ailus wrote:
> > Link validation currently accesses invalid pointers if the link passed to it
> > is not between two sub-devices. This is of course a driver bug.
> > 
> > Ignore the error but print a debug message, as this is how it used to work
> > previously.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> FWIW you have my Reported-by in there twice now ...

Ah, thanks. I'll drop the first tag.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

On 09/03/2023 14:56, Sakari Ailus wrote:
> On Thu, Mar 09, 2023 at 01:43:50PM +0100, Hans de Goede wrote:
>> Hi Sakari,
>>
>> On 3/9/23 13:27, Sakari Ailus wrote:
>>> Link validation currently accesses invalid pointers if the link passed to it
>>> is not between two sub-devices. This is of course a driver bug.
>>>
>>> Ignore the error but print a debug message, as this is how it used to work
>>> previously.
>>>
>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> FWIW you have my Reported-by in there twice now ...
> 
> Ah, thanks. I'll drop the first tag.

$ git grep Reported-and-tested-by Documentation/
Documentation/process/maintainer-tip.rst:Please do not use combined tags, e.g. ``Reported-and-tested-by``, as

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 02:57:52PM +0200, Tomi Valkeinen wrote:
> On 09/03/2023 14:56, Sakari Ailus wrote:
> > On Thu, Mar 09, 2023 at 01:43:50PM +0100, Hans de Goede wrote:
> > > Hi Sakari,
> > > 
> > > On 3/9/23 13:27, Sakari Ailus wrote:
> > > > Link validation currently accesses invalid pointers if the link passed to it
> > > > is not between two sub-devices. This is of course a driver bug.
> > > > 
> > > > Ignore the error but print a debug message, as this is how it used to work
> > > > previously.
> > > > 
> > > > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > > > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
> > > 
> > > FWIW you have my Reported-by in there twice now ...
> > 
> > Ah, thanks. I'll drop the first tag.
> 
> $ git grep Reported-and-tested-by Documentation/
> Documentation/process/maintainer-tip.rst:Please do not use combined tags, e.g. ``Reported-and-tested-by``, as

Uh-oh. Well, I'll just split that. Thanks.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi,

On 3/9/23 14:00, Sakari Ailus wrote:
> On Thu, Mar 09, 2023 at 02:57:52PM +0200, Tomi Valkeinen wrote:
>> On 09/03/2023 14:56, Sakari Ailus wrote:
>>> On Thu, Mar 09, 2023 at 01:43:50PM +0100, Hans de Goede wrote:
>>>> Hi Sakari,
>>>>
>>>> On 3/9/23 13:27, Sakari Ailus wrote:
>>>>> Link validation currently accesses invalid pointers if the link passed to it
>>>>> is not between two sub-devices. This is of course a driver bug.
>>>>>
>>>>> Ignore the error but print a debug message, as this is how it used to work
>>>>> previously.
>>>>>
>>>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> FWIW you have my Reported-by in there twice now ...
>>>
>>> Ah, thanks. I'll drop the first tag.
>>
>> $ git grep Reported-and-tested-by Documentation/
>> Documentation/process/maintainer-tip.rst:Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
> 
> Uh-oh. Well, I'll just split that. Thanks.

This is new to me too, but good to know. I'll try to get rid of the habbit
of using the combined tags then.

Regards,

Hans

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Mar 09, 2023 at 02:27:16PM +0200, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 1bebcda2bd20c..dd911180ec899 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1209,6 +1209,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  	struct v4l2_subdev_state *source_state, *sink_state;
>  	int ret;
>  
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->sink->entity->name);

Printing the whole link would make it easier to debug the problem:

		pr_warn_once("sink of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
			     link->source->entity->name, link->source->index);
			     link->sink->entity->name, link->sink->index);

Same below. With this, and the other comments in the mail thread
addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->source->entity->name);
> +		return 0;
> +	}
> +
>  	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>  	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>  

-- 
Regards,

Laurent Pinchart

IPU3 cameras not working with latest kernel code ?

[Hans de Goede]

Hi All,

While trying to test Kate's tps68470 patches for the privacy LED
on the back of the Surface Go 1/2 I hit this oops when trying
to run qcam:

[  162.502551] BUG: unable to handle page fault for address: 000000010000008f
[  162.502561] #PF: supervisor read access in kernel mode
[  162.502566] #PF: error_code(0x0000) - not-present page
[  162.502570] PGD 0 P4D 0 
[  162.502577] Oops: 0000 [#1] PREEMPT SMP PTI
[  162.502583] CPU: 3 PID: 2030 Comm: qcam Tainted: G         C         6.2.0+ #132
[  162.502589] Hardware name: Microsoft Corporation Surface Go/Surface Go, BIOS 1.0.30 12/22/2020
[  162.502593] RIP: 0010:v4l2_subdev_link_validate+0x12a/0x1b0 [videodev]
[  162.502631] Code: ed 74 56 8b 0d 53 c8 59 f0 85 c9 75 1d 48 8b bd 90 00 00 00 31 f6 e8 95 bc 5d ef 48 89 df e8 5d fc ff ff 89 c3 e9 5b ff ff ff <48> 8b bd 90 00 00 00 be ff ff ff ff 48 83 c7 68 e8 b1 d2 5c ef 83
[  162.502637] RSP: 0018:ffffb65766b93bb8 EFLAGS: 00010202
[  162.502642] RAX: ffff95d443bf0b18 RBX: ffff95d46a84a3c0 RCX: 0000000000000001
[  162.502646] RDX: 0000000000000001 RSI: 610f11f85a19d2aa RDI: ffff95d46a84a3c0
[  162.502650] RBP: 00000000ffffffff R08: 0000000000000004 R09: ffff95d443bf3f40
[  162.502654] R10: 0000000000000004 R11: 000000000c3a0bc4 R12: 0000000000000000
[  162.502657] R13: ffff95d443bf04e8 R14: ffff95d443bf3de8 R15: ffff95d443bf3f40
[  162.502661] FS:  00007f8181d9e6c0(0000) GS:ffff95d4aad80000(0000) knlGS:0000000000000000
[  162.502666] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  162.502670] CR2: 000000010000008f CR3: 000000014506e003 CR4: 00000000003706e0
[  162.502675] Call Trace:
[  162.502678]  <TASK>
[  162.502685]  __media_pipeline_start+0x505/0x6f0 [mc]
[  162.502710]  media_pipeline_start+0x32/0x50 [mc]
[  162.502728]  imgu_vb2_start_streaming+0x90/0x270 [ipu3_imgu]
[  162.502750]  vb2_start_streaming+0x6d/0x120 [videobuf2_common]
[  162.502767]  vb2_core_streamon+0xa4/0x100 [videobuf2_common]
[  162.502782]  __video_do_ioctl+0x18b/0x3c0 [videodev]
[  162.502822]  video_usercopy+0x2b9/0x8f0 [videodev]
[  162.502852]  ? __pfx___video_do_ioctl+0x10/0x10 [videodev]
[  162.502895]  ? lock_release+0x135/0x2d0
[  162.502911]  v4l2_ioctl+0x46/0x50 [videodev]
[  162.502937]  __x64_sys_ioctl+0x8d/0xd0
[  162.502946]  do_syscall_64+0x58/0x80
[  162.502956]  ? do_syscall_64+0x67/0x80
[  162.502962]  ? lockdep_hardirqs_on+0x7d/0x100
[  162.502969]  ? do_syscall_64+0x67/0x80
[  162.502975]  ? do_syscall_64+0x67/0x80
[  162.502982]  ? asm_exc_page_fault+0x22/0x30
[  162.502991]  ? lockdep_hardirqs_on+0x7d/0x100
[  162.502997]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
[  162.503004] RIP: 0033:0x7f8194b23baf
[  162.503010] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 18 48 8b 44 24 18 64 48 2b 04 25 28 00 00
[  162.503015] RSP: 002b:00007f8181d9c460 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  162.503021] RAX: ffffffffffffffda RBX: 00007f8178011120 RCX: 00007f8194b23baf
[  162.503025] RDX: 00007f8178011350 RSI: 0000000040045612 RDI: 0000000000000010
[  162.503028] RBP: 00007f8178010bd0 R08: 00007f817800db10 R09: 00007f8178016670
[  162.503032] R10: 0000000000000000 R11: 0000000000000246 R12: 00007f81780127f0
[  162.503035] R13: 00007f8178010ab0 R14: 00007f8178010bd0 R15: 00007f8178012950
[  162.503050]  </TASK>
[  162.503053] Modules linked in: rfcomm snd_seq_dummy snd_hrtimer snd_sof_pci_intel_skl snd_sof_intel_hda_common bnep soundwire_intel soundwire_generic_allocation soundwire_cadence snd_sof_intel_hda snd_sof_pci snd_sof_xtensa_dsp snd_sof dw9719 snd_sof_utils soundwire_bus mei_pxp mei_hdcp snd_soc_avs snd_soc_hda_codec gpio_tps68470 intel_rapl_msr snd_soc_skl snd_soc_hdac_hda snd_hda_ext_core snd_soc_sst_ipc snd_hda_codec_hdmi snd_soc_sst_dsp snd_soc_acpi_intel_match snd_hda_codec_realtek snd_soc_acpi snd_hda_codec_generic ledtrig_audio qrtr snd_soc_core snd_compress ac97_bus intel_tcc_cooling snd_pcm_dmaengine x86_pkg_temp_thermal intel_powerclamp coretemp snd_hda_intel snd_intel_dspcfg kvm_intel ath10k_pci snd_intel_sdw_acpi ath10k_core kvm snd_hda_codec irqbypass rapl intel_cstate intel_uncore mac80211 snd_hda_core btusb snd_hwdep snd_seq btrtl snd_seq_device btbcm intel_wmi_thunderbolt libarc4 pcspkr snd_pcm btintel ath btmtk bluetooth cfg80211 snd_timer snd hid_sensor_als
[  162.503166]  hid_sensor_rotation hid_sensor_gyro_3d hid_sensor_accel_3d ecdh_generic soundcore joydev hid_sensor_trigger hid_sensor_iio_common industrialio_triggered_buffer kfifo_buf rfkill industrialio intel_xhci_usb_role_switch ipu3_imgu(C) ipu3_cio2 processor_thermal_device_pci_legacy processor_thermal_device processor_thermal_rfim processor_thermal_mbox videobuf2_dma_sg mei_me videobuf2_memops mei processor_thermal_rapl intel_rapl_common videobuf2_v4l2 videobuf2_common intel_soc_dts_iosf idma64 intel_pch_thermal intel_skl_int3472_tps68470 leds_tps68470 tps68470_regulator clk_tps68470 ov5693 soc_button_array vfat ov7251 fat ov8865 v4l2_fwnode v4l2_async intel_skl_int3472_discrete videodev int3403_thermal mc int340x_thermal_zone intel_hid int3400_thermal sparse_keymap acpi_thermal_rel acpi_pad zram hid_sensor_hub intel_ishtp_hid i915 mmc_block crct10dif_pclmul crc32_pclmul crc32c_intel drm_buddy drm_display_helper sdhci_pci cqhci rtsx_pci_sdmmc sdhci cec intel_ish_ipc ucsi_acpi
[  162.503269]  ghash_clmulni_intel hid_multitouch sha512_ssse3 typec_ucsi serio_raw mmc_core intel_ishtp rtsx_pci ttm typec i2c_hid_acpi i2c_hid video wmi pinctrl_sunrisepoint ip6_tables ip_tables i2c_dev fuse
[  162.503304] CR2: 000000010000008f
[  162.503309] ---[ end trace 0000000000000000 ]---
[  162.503313] RIP: 0010:v4l2_subdev_link_validate+0x12a/0x1b0 [videodev]
[  162.503347] Code: ed 74 56 8b 0d 53 c8 59 f0 85 c9 75 1d 48 8b bd 90 00 00 00 31 f6 e8 95 bc 5d ef 48 89 df e8 5d fc ff ff 89 c3 e9 5b ff ff ff <48> 8b bd 90 00 00 00 be ff ff ff ff 48 83 c7 68 e8 b1 d2 5c ef 83
[  162.503351] RSP: 0018:ffffb65766b93bb8 EFLAGS: 00010202
[  162.503356] RAX: ffff95d443bf0b18 RBX: ffff95d46a84a3c0 RCX: 0000000000000001
[  162.503360] RDX: 0000000000000001 RSI: 610f11f85a19d2aa RDI: ffff95d46a84a3c0
[  162.503363] RBP: 00000000ffffffff R08: 0000000000000004 R09: ffff95d443bf3f40
[  162.503366] R10: 0000000000000004 R11: 000000000c3a0bc4 R12: 0000000000000000
[  162.503370] R13: ffff95d443bf04e8 R14: ffff95d443bf3de8 R15: ffff95d443bf3f40
[  162.503373] FS:  00007f8181d9e6c0(0000) GS:ffff95d4aad80000(0000) knlGS:0000000000000000
[  162.503378] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  162.503381] CR2: 000000010000008f CR3: 000000014506e003 CR4: 00000000003706e0

Note this is with a 6.2 kernel with:

https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

merged into it, which brings in most of media_stage.git/master, so chances are
this is caused by some of recent v4l2-core changes heading towards 6.3 .

I have checked if later media_stage.git/master changes may fix this,
but the only possibly relevant from after I rebased my media-atomisp
on top of media_stage.git/master is:

https://git.linuxtv.org/media_stage.git/commit/?id=a967a3a788028f541e4db54beabcebc3648997db

which does not directly seem likes a likely candidate for this.

Before I spend time on debugging this I was wondering if anyone has any
idea of what might be going on here ?

Also note that the invalid pointer causing this does not look like
a pointer at all, it looks more like 2 32 bit int fields to me?  :

"BUG: unable to handle page fault for address: 000000010000008f"

Regards,

Hans

[PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Link validation currently accesses invalid pointers if the link passed to it
is not between two sub-devices. This is of course a driver bug.

Ignore the error but print a debug message, as this is how it used to work
previously.

Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Hans,

Could you test this?

The bug is of course in the ImgU driver and this reverts to the old
pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
needs to be fixed and I think we could make this return an error at the same
time. Right now I can't be sure the ImgU driver is the only one suffering
from this, but if so, it's likely to be broken anyway.

- Sakari

 drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index dff1d9be7841..a6c80096586e 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1224,6 +1224,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
 	struct v4l2_subdev_state *source_state, *sink_state;
 	int ret;
 
+	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
+		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			 link->sink->entity->name);
+		return 0;
+	}
+	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
+		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			 link->source->entity->name);
+		return 0;
+	}
+
 	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
 	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
 
-- 
2.30.2

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

On 02/03/2023 22:22, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Hans,
> 
> Could you test this?
> 
> The bug is of course in the ImgU driver and this reverts to the old
> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> needs to be fixed and I think we could make this return an error at the same
> time. Right now I can't be sure the ImgU driver is the only one suffering
> from this, but if so, it's likely to be broken anyway.

Maybe it should be at least a warn? How do we catch other broken drivers 
otherwise?

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Hi Tomi,

On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
> On 02/03/2023 22:22, Sakari Ailus wrote:
> > Link validation currently accesses invalid pointers if the link passed to it
> > is not between two sub-devices. This is of course a driver bug.
> > 
> > Ignore the error but print a debug message, as this is how it used to work
> > previously.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Hans,
> > 
> > Could you test this?
> > 
> > The bug is of course in the ImgU driver and this reverts to the old
> > pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> > needs to be fixed and I think we could make this return an error at the same
> > time. Right now I can't be sure the ImgU driver is the only one suffering
> > from this, but if so, it's likely to be broken anyway.
> 
> Maybe it should be at least a warn? How do we catch other broken drivers
> otherwise?

The purpose of this patch is just to restore the old behaviour, and merge
it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
error but I'd like that change to be present in the media tree for some
time first.

> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Thanks!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

On 03/03/2023 13:30, Sakari Ailus wrote:
> Hi Tomi,
> 
> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
>> On 02/03/2023 22:22, Sakari Ailus wrote:
>>> Link validation currently accesses invalid pointers if the link passed to it
>>> is not between two sub-devices. This is of course a driver bug.
>>>
>>> Ignore the error but print a debug message, as this is how it used to work
>>> previously.
>>>
>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>> Hi Hans,
>>>
>>> Could you test this?
>>>
>>> The bug is of course in the ImgU driver and this reverts to the old
>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
>>> needs to be fixed and I think we could make this return an error at the same
>>> time. Right now I can't be sure the ImgU driver is the only one suffering
>>> from this, but if so, it's likely to be broken anyway.
>>
>> Maybe it should be at least a warn? How do we catch other broken drivers
>> otherwise?
> 
> The purpose of this patch is just to restore the old behaviour, and merge
> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
> error but I'd like that change to be present in the media tree for some
> time first.

I meant that keep it returning 0 (no error), but instead of a debug 
print, use pr_warn. Or maybe pr_warn_once for now.

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi,

On 3/3/23 12:36, Tomi Valkeinen wrote:
> On 03/03/2023 13:30, Sakari Ailus wrote:
>> Hi Tomi,
>>
>> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
>>> On 02/03/2023 22:22, Sakari Ailus wrote:
>>>> Link validation currently accesses invalid pointers if the link passed to it
>>>> is not between two sub-devices. This is of course a driver bug.
>>>>
>>>> Ignore the error but print a debug message, as this is how it used to work
>>>> previously.
>>>>
>>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>> Hi Hans,
>>>>
>>>> Could you test this?
>>>>
>>>> The bug is of course in the ImgU driver and this reverts to the old
>>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
>>>> needs to be fixed and I think we could make this return an error at the same
>>>> time. Right now I can't be sure the ImgU driver is the only one suffering
>>>> from this, but if so, it's likely to be broken anyway.
>>>
>>> Maybe it should be at least a warn? How do we catch other broken drivers
>>> otherwise?
>>
>> The purpose of this patch is just to restore the old behaviour, and merge
>> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
>> error but I'd like that change to be present in the media tree for some
>> time first.
> 
> I meant that keep it returning 0 (no error), but instead of a debug print, use pr_warn. Or maybe pr_warn_once for now.

Switching to pr_warn_once() sounds reasonable to me.

I'll try to give this a test-run sometime next week.

Regards,

Hans

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Hi,

On Fri, Mar 03, 2023 at 04:06:11PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/3/23 12:36, Tomi Valkeinen wrote:
> > On 03/03/2023 13:30, Sakari Ailus wrote:
> >> Hi Tomi,
> >>
> >> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
> >>> On 02/03/2023 22:22, Sakari Ailus wrote:
> >>>> Link validation currently accesses invalid pointers if the link passed to it
> >>>> is not between two sub-devices. This is of course a driver bug.
> >>>>
> >>>> Ignore the error but print a debug message, as this is how it used to work
> >>>> previously.
> >>>>
> >>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> >>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>> ---
> >>>> Hi Hans,
> >>>>
> >>>> Could you test this?
> >>>>
> >>>> The bug is of course in the ImgU driver and this reverts to the old
> >>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> >>>> needs to be fixed and I think we could make this return an error at the same
> >>>> time. Right now I can't be sure the ImgU driver is the only one suffering
> >>>> from this, but if so, it's likely to be broken anyway.
> >>>
> >>> Maybe it should be at least a warn? How do we catch other broken drivers
> >>> otherwise?
> >>
> >> The purpose of this patch is just to restore the old behaviour, and merge
> >> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
> >> error but I'd like that change to be present in the media tree for some
> >> time first.
> > 
> > I meant that keep it returning 0 (no error), but instead of a debug print, use pr_warn. Or maybe pr_warn_once for now.
> 
> Switching to pr_warn_once() sounds reasonable to me.

I'll send v2 with that.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi Sakari,

On 3/2/23 21:22, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Hans,
> 
> Could you test this?

Yes, thank you for the fix.

I can confirm that this fixes the oops on my Surface Go 1 and
that it makes the IPU3 attached cameras work again:

Tested-by: Hans de Goede <hdegoede@redhat.com>

or better just replace the Reported-by with:

Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> The bug is of course in the ImgU driver and this reverts to the old
> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> needs to be fixed and I think we could make this return an error at the same
> time. Right now I can't be sure the ImgU driver is the only one suffering
> from this, but if so, it's likely to be broken anyway.
> 
> - Sakari
> 
>  drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index dff1d9be7841..a6c80096586e 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1224,6 +1224,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  	struct v4l2_subdev_state *source_state, *sink_state;
>  	int ret;
>  
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			 link->sink->entity->name);
> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			 link->source->entity->name);
> +		return 0;
> +	}
> +
>  	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>  	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>  

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 01:09:11PM +0100, Hans de Goede wrote:
> Hi Sakari,
> 
> On 3/2/23 21:22, Sakari Ailus wrote:
> > Link validation currently accesses invalid pointers if the link passed to it
> > is not between two sub-devices. This is of course a driver bug.
> > 
> > Ignore the error but print a debug message, as this is how it used to work
> > previously.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Hans,
> > 
> > Could you test this?
> 
> Yes, thank you for the fix.
> 
> I can confirm that this fixes the oops on my Surface Go 1 and
> that it makes the IPU3 attached cameras work again:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> or better just replace the Reported-by with:
> 
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

Thanks!

I'm putting this to my fixes PR I'm about to send soon.

-- 
Sakari Ailus