Re: [PATCH 4.4 089/105] media: dvb_frontend: fix locking issues at dvb_frontend_get_event()

[Ben Hutchings]

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@xxxxxxxxxxxxxxxx>
> 
> 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