Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1532112984.21552.39.camel@codethink.co.uk>
Subject: Re: [PATCH 4.4 089/105] media: dvb_frontend: fix locking issues at
 dvb_frontend_get_event()
From:   Ben Hutchings <ben.hutchings@codethink.co.uk>
To:     Mauro Carvalho Chehab <mchehab@s-opensource.com>
Cc:     stable@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>
Date:   Fri, 20 Jul 2018 19:56:24 +0100
In-Reply-To: <20180701153155.855633462@linuxfoundation.org>
References: <20180701153149.382300170@linuxfoundation.org>
         <20180701153155.855633462@linuxfoundation.org>
Organization: Codethink Ltd.
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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