| Message ID | 25daeee8-f7ef-4595-b2e9-7e4d80ce3f1d@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for,v6.10] media: v4l2-ctrls: drop unnecessary locking that caused potential deadlock | expand |
Hi Hans, Thank you for the patch. On Wed, May 08, 2024 at 11:11:01AM +0200, Hans Verkuil wrote: > When logging the control values through VIDIOC_LOG_STATUS you could > get into a potential deadlock situation in the vivid driver: > > [Wed May 8 10:02:06 2024] Possible unsafe locking scenario: > > [Wed May 8 10:02:06 2024] CPU0 CPU1 > [Wed May 8 10:02:06 2024] ---- ---- > [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1620:(hdl_vid_cap)->_lock); > [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1608:(hdl_user_vid)->_lock); > [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1620:(hdl_vid_cap)->_lock); > [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1608:(hdl_user_vid)->_lock); This looks scary. How was this triggered, can you provide the full lockdep report ? I'm worried there could be other scenarios where the locks could be taken in different orders. > That's because both the main control handler was locked and the > inner control handler containing the control that you want to log. > > In this particular case there is no need to lock the inner control > handler since it is guaranteed that that handler won't delete controls > unexpectedly. All the v4l2_ctrl_type_ops operations currently rely on the control handler being locked when they're called. I don't like the idea of lifting that requirement without an audit of the implementations. We should then clearly document that the log operation can be called without the lock held. > > Fixes: 9801b5b28c692 ("media: v4l2-ctrls: show all owned controls in log_status") > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > index c59dd691f79f6..4e6748bd419cf 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > @@ -2518,11 +2518,7 @@ static void log_ctrl(const struct v4l2_ctrl_handler *hdl, You can also drop the hdl argument to this function, which was introduced by the commit referenced by Fixes:. > > pr_info("%s%s%s: ", prefix, colon, ctrl->name); > > - if (ctrl->handler != hdl) > - v4l2_ctrl_lock(ctrl); > ctrl->type_ops->log(ctrl); > - if (ctrl->handler != hdl) > - v4l2_ctrl_unlock(ctrl); > > if (ctrl->flags & (V4L2_CTRL_FLAG_INACTIVE | > V4L2_CTRL_FLAG_GRABBED |
Another comment. The subject line states v6.10, but this is a v6.9 regression. How about reverting the offending commit for v6.9, and working on a better implementation for v6.11 ? On Fri, May 10, 2024 at 03:02:25AM +0300, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Wed, May 08, 2024 at 11:11:01AM +0200, Hans Verkuil wrote: > > When logging the control values through VIDIOC_LOG_STATUS you could > > get into a potential deadlock situation in the vivid driver: > > > > [Wed May 8 10:02:06 2024] Possible unsafe locking scenario: > > > > [Wed May 8 10:02:06 2024] CPU0 CPU1 > > [Wed May 8 10:02:06 2024] ---- ---- > > [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1620:(hdl_vid_cap)->_lock); > > [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1608:(hdl_user_vid)->_lock); > > [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1620:(hdl_vid_cap)->_lock); > > [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1608:(hdl_user_vid)->_lock); > > This looks scary. How was this triggered, can you provide the full > lockdep report ? I'm worried there could be other scenarios where the > locks could be taken in different orders. > > > That's because both the main control handler was locked and the > > inner control handler containing the control that you want to log. > > > > In this particular case there is no need to lock the inner control > > handler since it is guaranteed that that handler won't delete controls > > unexpectedly. > > All the v4l2_ctrl_type_ops operations currently rely on the control > handler being locked when they're called. I don't like the idea of > lifting that requirement without an audit of the implementations. We > should then clearly document that the log operation can be called > without the lock held. > > > > > Fixes: 9801b5b28c692 ("media: v4l2-ctrls: show all owned controls in log_status") > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > --- > > drivers/media/v4l2-core/v4l2-ctrls-core.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > > index c59dd691f79f6..4e6748bd419cf 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > > @@ -2518,11 +2518,7 @@ static void log_ctrl(const struct v4l2_ctrl_handler *hdl, > > You can also drop the hdl argument to this function, which was > introduced by the commit referenced by Fixes:. > > > > > pr_info("%s%s%s: ", prefix, colon, ctrl->name); > > > > - if (ctrl->handler != hdl) > > - v4l2_ctrl_lock(ctrl); > > ctrl->type_ops->log(ctrl); > > - if (ctrl->handler != hdl) > > - v4l2_ctrl_unlock(ctrl); > > > > if (ctrl->flags & (V4L2_CTRL_FLAG_INACTIVE | > > V4L2_CTRL_FLAG_GRABBED |
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c index c59dd691f79f6..4e6748bd419cf 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c @@ -2518,11 +2518,7 @@ static void log_ctrl(const struct v4l2_ctrl_handler *hdl, pr_info("%s%s%s: ", prefix, colon, ctrl->name); - if (ctrl->handler != hdl) - v4l2_ctrl_lock(ctrl); ctrl->type_ops->log(ctrl); - if (ctrl->handler != hdl) - v4l2_ctrl_unlock(ctrl); if (ctrl->flags & (V4L2_CTRL_FLAG_INACTIVE | V4L2_CTRL_FLAG_GRABBED |
When logging the control values through VIDIOC_LOG_STATUS you could get into a potential deadlock situation in the vivid driver: [Wed May 8 10:02:06 2024] Possible unsafe locking scenario: [Wed May 8 10:02:06 2024] CPU0 CPU1 [Wed May 8 10:02:06 2024] ---- ---- [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1620:(hdl_vid_cap)->_lock); [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1608:(hdl_user_vid)->_lock); [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1620:(hdl_vid_cap)->_lock); [Wed May 8 10:02:06 2024] lock(vivid_ctrls:1608:(hdl_user_vid)->_lock); That's because both the main control handler was locked and the inner control handler containing the control that you want to log. In this particular case there is no need to lock the inner control handler since it is guaranteed that that handler won't delete controls unexpectedly. Fixes: 9801b5b28c692 ("media: v4l2-ctrls: show all owned controls in log_status") Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/v4l2-core/v4l2-ctrls-core.c | 4 ---- 1 file changed, 4 deletions(-)