hi, I'm getting a crash in v4l2_async_unregister_subdev on an error path, due to sd->subdev_notifier being NULL
I don't see where this one is supposed to be allocated
does anyone have a clue?
oh I see it's v4l2_async_register_subdev_sensor_common
looks to me like this should be conditional then, right?
ooh found another good one
v4l2_async_notifier_add_fwnode_remote_subdev doesn't alloc the async_subdev although it's always kfreed in __v4l2_async_notifier_cleanup
will send patches
now I no longer have crashes on my error path \o/
hverkuil: I'm in the middle of rebasing the patches you asked about last week, but decided I need to audit all subdev ioctl handlers to make sure I get it right
I came across c3fda7f835b0 ("V4L/DVB (10537): saa6588: convert to v4l2_subdev."), which you added 12 years ago, and this seems very suspicious
is it possible to call SAA6588_CMD_READ from user space and end up in this driver with an invalid pointer?
arnd: it's not a userspace interface, it's used in the kernel to communicate between a media bridge driver and the i2c subdevice driver. Nothing to do with userspace ioctls.
hverkuil: ok, good. How do I know which drivers can get called from subdev_do_ioctl() and which cannot?
static const struct v4l2_subdev_core_ops saa6588_core_ops = {
        .ioctl = saa6588_ioctl,
};
The .ioctl callback in the struct v4l2_subdev_core_ops is for kernel-internal use only.
It's similar to the command callback in struct i2c_driver.
I didn't answer your question, give me a moment...
forget what I said. Sorry, 12 years is a long time, I need to dig a bit deeper.
hverkuil: I see adv7842_ioctl() and si4713_ioctl() use similar constructs that appear to be designed to be called from user space, so I made a patch to add .compat_ioctl32() support for those
https://www.irccloud.com/pastebin/T6AYm9Xz/
this is my current draft in case it turns out I was right
adv7842_ioctl is definitely meant to be called from userspace.
but the saa6588 'ioctl' is indeed meant for internal use only. I think this was missed that the core ops ioctl callback had two uses. Now, in practice the saa6588 'ioctl' will never be exposed to userspace, but it would be a very good idea to split the two use-cases of core.ioctl.
I think rather than calling it internal_ioctl, just call it 'command' (like struct i2c_driver), that avoids any future confusion when grepping for 'ioctl'.
ok, I'll change my patch that way.
it seems that venc_ioctl() from davinci should be in there as well
yes
Originally all the v4l2_subdev ops where kernel internal, later v4l-subdevX device nodes were created (optionally), and then the ioctl op got hijacked for use of implementing subdev-specific userspace ioctls. But nobody realized that it was already used for communication between drivers.
ok, I see
other than those four drivers I mentioned, the remaining subdev ioctl functions are for omap3_isp and atomisp, which in theory need a compat handler. omap3/am3 is never 64-bit though (and am6 does not use this isp), and atomisp has a broken compat handler that I'm removing
hverkuil: I pushed a preliminary branch with seven patches to "git://git.kernel.org:/pub/scm/linux/kernel/git/arnd/playground.git v4l2-compat-ioctl" now. I haven't run it through the usual build testing yet, will post them to the list once that has passed, but you can take a look already if you want
it took me the longest time to figure out whether or not an #ifdef is needed in the v4l2_subdev_do_ioctl(), and in the end I decided that the code I had added the #ifdef to was completely bogus to start with, and I instead made a patch to remove it
(and yes, I had added that code originally, and no, I have no clue about why I thought it was necessary)