[PATCH] [media] dvb-frontends/stv0910: prevent consecutive mutex_unlock()'s

[PATCH] [media] dvb-frontends/stv0910: prevent consecutive mutex_unlock()'s

[Daniel Scheller]

From: Daniel Scheller <d.scheller@gmx.net>

When calling gate_ctrl() with enable=0 if previously the mutex wasn't
locked (ie. on enable=1 failure and subdrivers not handling this properly,
or by otherwise badly behaving drivers), the i2c_lock could be unlocked
consecutively which isn't allowed. Prevent this by keeping track of the
lock state, and actually call mutex_unlock() only when certain the lock
is held.

Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
Tested-by: Jasmin Jessich <jasmin@anw.at>
---
 drivers/media/dvb-frontends/stv0910.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb-frontends/stv0910.c b/drivers/media/dvb-frontends/stv0910.c
index 73f6df0abbfe..36ef96ec64c1 100644
--- a/drivers/media/dvb-frontends/stv0910.c
+++ b/drivers/media/dvb-frontends/stv0910.c
@@ -80,6 +80,7 @@ struct stv_base {
 	u8                   adr;
 	struct i2c_adapter  *i2c;
 	struct mutex         i2c_lock; /* shared I2C access protect */
+	u8                   i2c_islocked; /* I2C lock state */
 	struct mutex         reg_lock; /* shared register write protect */
 	int                  count;
 
@@ -1233,6 +1234,7 @@ static int gate_ctrl(struct dvb_frontend *fe, int enable)
 
 	if (enable) {
 		mutex_lock(&state->base->i2c_lock);
+		state->base->i2c_islocked = 1;
 		i2crpt |= 0x80;
 	} else {
 		i2crpt |= 0x02;
@@ -1240,8 +1242,15 @@ static int gate_ctrl(struct dvb_frontend *fe, int enable)
 
 	if (write_reg(state, state->nr ? RSTV0910_P2_I2CRPT :
 		      RSTV0910_P1_I2CRPT, i2crpt) < 0) {
-		/* don't hold the I2C bus lock on failure */
-		mutex_unlock(&state->base->i2c_lock);
+		/*
+		 * don't hold the I2C bus lock on failure while preventing
+		 * consecutive and disallowed calls to mutex_unlock()
+		 */
+		if (state->base->i2c_islocked) {
+			state->base->i2c_islocked = 0;
+			mutex_unlock(&state->base->i2c_lock);
+		}
+
 		dev_err(&state->base->i2c->dev,
 			"%s() write_reg failure (enable=%d)\n",
 			__func__, enable);
@@ -1250,8 +1259,13 @@ static int gate_ctrl(struct dvb_frontend *fe, int enable)
 
 	state->i2crpt = i2crpt;
 
-	if (!enable)
-		mutex_unlock(&state->base->i2c_lock);
+	if (!enable) {
+		if (state->base->i2c_islocked) {
+			state->base->i2c_islocked = 0;
+			mutex_unlock(&state->base->i2c_lock);
+		}
+	}
+
 	return 0;
 }
 
@@ -1795,6 +1809,7 @@ struct dvb_frontend *stv0910_attach(struct i2c_adapter *i2c,
 
 		mutex_init(&base->i2c_lock);
 		mutex_init(&base->reg_lock);
+		base->i2c_islocked = 0;
 		state->base = base;
 		if (probe(state) < 0) {
 			dev_info(&i2c->dev, "No demod found at adr %02X on %s\n",
-- 
2.13.6

[PATCH] [media] dvb-frontends/stv0910: prevent consecutive mutex_unlock()'s

[Ralph Metzler]

Daniel Scheller writes:
 > From: Daniel Scheller <d.scheller@gmx.net>
 > 
 > When calling gate_ctrl() with enable=0 if previously the mutex wasn't
 > locked (ie. on enable=1 failure and subdrivers not handling this properly,
 > or by otherwise badly behaving drivers), the i2c_lock could be unlocked

I think drivers and subdrivers should rather be fixed so that this
cannot happen.
But to do this we will first need to define exactly how a failure in
gate_ctrl() is supposed to be handled, both inside gate_ctrl() and
by calling drivers.


 > consecutively which isn't allowed. Prevent this by keeping track of the
 > lock state, and actually call mutex_unlock() only when certain the lock
 > is held.

Why not use mutex_is_locked()?
And there should be a debug message if it (tried double unlocking) happens.


Regards,
Ralph

Re: [PATCH] [media] dvb-frontends/stv0910: prevent consecutive mutex_unlock()'s

[Daniel Scheller]

Am Sat, 21 Oct 2017 11:28:10 +0200
schrieb Ralph Metzler <rjkm@metzlerbros.de>:

> Daniel Scheller writes:
>  > From: Daniel Scheller <d.scheller@gmx.net>
>  > 
>  > When calling gate_ctrl() with enable=0 if previously the mutex
>  > wasn't locked (ie. on enable=1 failure and subdrivers not handling
>  > this properly, or by otherwise badly behaving drivers), the
>  > i2c_lock could be unlocked  
> 
> I think drivers and subdrivers should rather be fixed so that this
> cannot happen.

As long as stv6111 remains the only chip/driver interfacing with the
stv0910, that's an easy task. However, if other hardware has some other
stv0910+tunerchip combination, things get interesting. In a perfect
world with unicorns and such, every component interacts as intended,
but that's not the case, so I believe this should be handled at the
root.

> But to do this we will first need to define exactly how a failure in
> gate_ctrl() is supposed to be handled, both inside gate_ctrl() and
> by calling drivers.

Well, IMHO (and thats the intention) if gate_ctrl fails due to a
hardware/I2C problem, it isn't opened so there's no need to hold the
lock (since the gate isn't - exclusively - opened). For reasons stated
above this keeps things safe from deadlocking (and we want to avoid
that, even more than double unlocking).

>  > consecutively which isn't allowed. Prevent this by keeping track
>  > of the lock state, and actually call mutex_unlock() only when
>  > certain the lock is held.  
> 
> Why not use mutex_is_locked()?

Good catch (I should try harder finding out what the kernel API has to
offer...). If you prefer that, I'll respin with this and without the
var as v2.

> And there should be a debug message if it (tried double unlocking)
> happens.

Ok. Should IMHO go to dev_dbg then - if drivers don't catch that
situation, this may else lead do kernel log spam.

Best regards,
Daniel Scheller
-- 
https://github.com/herrnst

Re: [PATCH] [media] dvb-frontends/stv0910: prevent consecutive mutex_unlock()'s

[Mauro Carvalho Chehab]

Em Sat, 21 Oct 2017 11:57:57 +0200
Daniel Scheller <d.scheller.oss@gmail.com> escreveu:

> Am Sat, 21 Oct 2017 11:28:10 +0200
> schrieb Ralph Metzler <rjkm@metzlerbros.de>:
> 
> > Daniel Scheller writes:  
> >  > From: Daniel Scheller <d.scheller@gmx.net>
> >  > 
> >  > When calling gate_ctrl() with enable=0 if previously the mutex
> >  > wasn't locked (ie. on enable=1 failure and subdrivers not handling
> >  > this properly, or by otherwise badly behaving drivers), the
> >  > i2c_lock could be unlocked    
> > 
> > I think drivers and subdrivers should rather be fixed so that this
> > cannot happen.  

Agreed.

> 
> As long as stv6111 remains the only chip/driver interfacing with the
> stv0910, that's an easy task. However, if other hardware has some other
> stv0910+tunerchip combination, things get interesting. In a perfect
> world with unicorns and such, every component interacts as intended,
> but that's not the case, so I believe this should be handled at the
> root.

No matter what tuner/demod is used, if the locks there are not
properly handled, bad things will happen. So, if this patch is
needed for the driver(s) to work, it means that we have a serious
issue urging a real fix.

This ugly hack won't prevent it to happen. It just masks the
issue, by keeping the driver locked for more time than needed,
with possibly cause other issues.

> 
> > But to do this we will first need to define exactly how a failure in
> > gate_ctrl() is supposed to be handled, both inside gate_ctrl() and
> > by calling drivers.  
> 
> Well, IMHO (and thats the intention) if gate_ctrl fails due to a
> hardware/I2C problem, it isn't opened so there's no need to hold the
> lock (since the gate isn't - exclusively - opened). For reasons stated
> above this keeps things safe from deadlocking (and we want to avoid
> that, even more than double unlocking).
> 
> >  > consecutively which isn't allowed. Prevent this by keeping track
> >  > of the lock state, and actually call mutex_unlock() only when
> >  > certain the lock is held.    
> > 
> > Why not use mutex_is_locked()?  
> 
> Good catch (I should try harder finding out what the kernel API has to
> offer...). If you prefer that, I'll respin with this and without the
> var as v2.

I may consider accepting something like:

	WARN_ON(mutex_is_locked(mutex));

That will actually cause a bug at the driver with a stack dump,
with may lead to a proper fix on the drivers, instead of a hackish
solution that would just let the bug to stay there forever.

Thanks,
Mauro