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:4.018 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site