lkml.org 
[lkml]   [2018]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 4.4 089/105] media: dvb_frontend: fix locking issues at dvb_frontend_get_event()
From
Date
On Sun, 2018-07-01 at 18:02 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>
> commit 76d81243a487c09619822ef8e7201a756e58a87d upstream.
>
> As warned by smatch:
> drivers/media/dvb-core/dvb_frontend.c:314 dvb_frontend_get_event() warn: inconsistent returns 'sem:&fepriv->sem'.
>   Locked on:   line 288
>                line 295
>                line 306
>                line 314
>   Unlocked on: line 303
>
> The lock implementation for get event is wrong, as, if an
> interrupt occurs, down_interruptible() will fail, and the
> routine will call up() twice when userspace calls the ioctl
> again.
>
> The bad code is there since when Linux migrated to git, in
> 2005.
[...] 
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
[...]
> +static int dvb_frontend_test_event(struct dvb_frontend_private *fepriv,
> +    struct dvb_fe_events *events)
> +{
> + int ret;
> +
> + up(&fepriv->sem);
> + ret = events->eventw != events->eventr;
> + down(&fepriv->sem);
> +
> + return ret;
> +}

This is broken. You must not wait inside a wait condition. (It would
be nice if this rule was explicitly documented.)

Why not leave the condition as it was and only change the
down_interruptible() to down()?

Ben.

>  static int dvb_frontend_get_event(struct dvb_frontend *fe,
> -     struct dvb_frontend_event *event, int flags)
> +           struct dvb_frontend_event *event, int flags)
>  {
>   struct dvb_frontend_private *fepriv = fe->frontend_priv;
>   struct dvb_fe_events *events = &fepriv->events;
> @@ -249,13 +261,8 @@ static int dvb_frontend_get_event(struct
>   if (flags & O_NONBLOCK)
>   return -EWOULDBLOCK;
>  
> - up(&fepriv->sem);
> -
> - ret = wait_event_interruptible (events->wait_queue,
> - events->eventw != events->eventr);
> -
> - if (down_interruptible (&fepriv->sem))
> - return -ERESTARTSYS;
> + ret = wait_event_interruptible(events->wait_queue,
> +        dvb_frontend_test_event(fepriv, events));
>  
>   if (ret < 0)
>   return ret;
>
>
>
--
Ben Hutchings, Software Developer   Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
Manchester, M1 2HF, United Kingdom

\
 
 \ /
  Last update: 2018-07-20 20:57    [W:0.505 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site