[PATCH 0/2] media protect enable and disable source handler paths

[PATCH 0/2] media protect enable and disable source handler paths

[Shuah Khan]

These two patches fix enable and disable source handler paths. These
aren't dependent patches, grouped because they fix similar problems.

This work is triggered by a review comment from Mauro Chehab on a
snd_usb_audio patch about protecting the enable and disabel handler
path in it.

Ran tests to make sure enable and disable handler paths work. When
digital stream is active, analog app finds the tuner busy and vice
versa. Also ran the Sakari's unbind while video stream is active test.

Shuah Khan (2):
  media: au0828 fix to protect enable/disable source set and clear
  media: protect enable and disable source handler checks and calls

 drivers/media/dvb-core/dvb_frontend.c  | 24 ++++++++++++++++++------
 drivers/media/usb/au0828/au0828-core.c | 21 +++++++++------------
 drivers/media/v4l2-core/v4l2-mc.c      | 26 ++++++++++++++++++--------
 3 files changed, 45 insertions(+), 26 deletions(-)

-- 
2.7.4

[PATCH 1/2] media: au0828 fix to protect enable/disable source set and clear

[Shuah Khan]

Protect enable/disable source set and clear to avoid races with callers.
There is a possibility clear could occur while dvb-core and v4l2 try to
access these handlers.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/usb/au0828/au0828-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index bf53553..a1f696a 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -153,9 +153,11 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
 	}
 
 	/* clear enable_source, disable_source */
+	mutex_lock(&mdev->graph_mutex);
 	dev->media_dev->source_priv = NULL;
 	dev->media_dev->enable_source = NULL;
 	dev->media_dev->disable_source = NULL;
+	mutex_unlock(&mdev->graph_mutex);
 
 	media_device_unregister(dev->media_dev);
 	media_device_cleanup(dev->media_dev);
@@ -549,9 +551,11 @@ static int au0828_media_device_register(struct au0828_dev *dev,
 		return ret;
 	}
 	/* set enable_source */
+	mutex_lock(&dev->media_dev->graph_mutex);
 	dev->media_dev->source_priv = (void *) dev;
 	dev->media_dev->enable_source = au0828_enable_source;
 	dev->media_dev->disable_source = au0828_disable_source;
+	mutex_unlock(&dev->media_dev->graph_mutex);
 #endif
 	return 0;
 }
-- 
2.7.4

[PATCH 2/2] media: protect enable and disable source handler checks and calls

[Shuah Khan]

Protect enable and disable source handler checks and calls from dvb-core
and v4l2-core. Hold graph_mutex to check if enable and disable source
handlers are present and invoke them while holding the mutex. This change
ensures these handlers will not be removed while they are being checked
and invoked.

au08282 enable and disable source handlers are changed to not hold the
graph_mutex.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 drivers/media/dvb-core/dvb_frontend.c  | 24 ++++++++++++++++++------
 drivers/media/usb/au0828/au0828-core.c | 17 +++++------------
 drivers/media/v4l2-core/v4l2-mc.c      | 26 ++++++++++++++++++--------
 3 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 01511e5..2f09c7e 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
 		fepriv->voltage = -1;
 
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-		if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
-			ret = fe->dvb->mdev->enable_source(dvbdev->entity,
+		if (fe->dvb->mdev) {
+			mutex_lock(&fe->dvb->mdev->graph_mutex);
+			if (fe->dvb->mdev->enable_source)
+				ret = fe->dvb->mdev->enable_source(
+							   dvbdev->entity,
 							   &fepriv->pipe);
+			mutex_unlock(&fe->dvb->mdev->graph_mutex);
 			if (ret) {
 				dev_err(fe->dvb->device,
 					"Tuner is busy. Error %d\n", ret);
@@ -2553,8 +2557,12 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
 
 err3:
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-	if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
-		fe->dvb->mdev->disable_source(dvbdev->entity);
+	if (fe->dvb->mdev) {
+		mutex_lock(&fe->dvb->mdev->graph_mutex);
+		if (fe->dvb->mdev->disable_source)
+			fe->dvb->mdev->disable_source(dvbdev->entity);
+		mutex_unlock(&fe->dvb->mdev->graph_mutex);
+	}
 err2:
 #endif
 	dvb_generic_release(inode, file);
@@ -2586,8 +2594,12 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
 	if (dvbdev->users == -1) {
 		wake_up(&fepriv->wait_queue);
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-		if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
-			fe->dvb->mdev->disable_source(dvbdev->entity);
+		if (fe->dvb->mdev) {
+			mutex_lock(&fe->dvb->mdev->graph_mutex);
+			if (fe->dvb->mdev->disable_source)
+				fe->dvb->mdev->disable_source(dvbdev->entity);
+			mutex_unlock(&fe->dvb->mdev->graph_mutex);
+		}
 #endif
 		if (fe->exit != DVB_FE_NO_EXIT)
 			wake_up(&dvbdev->wait_queue);
diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index a1f696a..bfd6482 100644
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -280,6 +280,7 @@ static void au0828_media_graph_notify(struct media_entity *new,
 	}
 }
 
+/* Callers should hold graph_mutex */
 static int au0828_enable_source(struct media_entity *entity,
 				struct media_pipeline *pipe)
 {
@@ -293,8 +294,6 @@ static int au0828_enable_source(struct media_entity *entity,
 	if (!mdev)
 		return -ENODEV;
 
-	mutex_lock(&mdev->graph_mutex);
-
 	dev = mdev->source_priv;
 
 	/*
@@ -421,12 +420,12 @@ static int au0828_enable_source(struct media_entity *entity,
 		 dev->active_source->name, dev->active_sink->name,
 		 dev->active_link_owner->name, ret);
 end:
-	mutex_unlock(&mdev->graph_mutex);
 	pr_debug("au0828_enable_source() end %s %d %d\n",
 		 entity->name, entity->function, ret);
 	return ret;
 }
 
+/* Callers should hold graph_mutex */
 static void au0828_disable_source(struct media_entity *entity)
 {
 	int ret = 0;
@@ -436,13 +435,10 @@ static void au0828_disable_source(struct media_entity *entity)
 	if (!mdev)
 		return;
 
-	mutex_lock(&mdev->graph_mutex);
 	dev = mdev->source_priv;
 
-	if (!dev->active_link) {
-		ret = -ENODEV;
-		goto end;
-	}
+	if (!dev->active_link)
+		return;
 
 	/* link is active - stop pipeline from source (tuner) */
 	if (dev->active_link->sink->entity == dev->active_sink &&
@@ -452,7 +448,7 @@ static void au0828_disable_source(struct media_entity *entity)
 		 * has active pipeline
 		*/
 		if (dev->active_link_owner != entity)
-			goto end;
+			return;
 		__media_entity_pipeline_stop(entity);
 		ret = __media_entity_setup_link(dev->active_link, 0);
 		if (ret)
@@ -467,9 +463,6 @@ static void au0828_disable_source(struct media_entity *entity)
 		dev->active_source = NULL;
 		dev->active_sink = NULL;
 	}
-
-end:
-	mutex_unlock(&mdev->graph_mutex);
 }
 #endif
 
diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
index 8bef433..b169d24 100644
--- a/drivers/media/v4l2-core/v4l2-mc.c
+++ b/drivers/media/v4l2-core/v4l2-mc.c
@@ -198,14 +198,20 @@ EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph);
 int v4l_enable_media_source(struct video_device *vdev)
 {
 	struct media_device *mdev = vdev->entity.graph_obj.mdev;
-	int ret;
+	int ret = 0, err;
 
-	if (!mdev || !mdev->enable_source)
+	if (!mdev)
 		return 0;
-	ret = mdev->enable_source(&vdev->entity, &vdev->pipe);
-	if (ret)
-		return -EBUSY;
-	return 0;
+
+	mutex_lock(&mdev->graph_mutex);
+	if (!mdev->enable_source)
+		goto end;
+	err = mdev->enable_source(&vdev->entity, &vdev->pipe);
+	if (err)
+		ret = -EBUSY;
+end:
+	mutex_unlock(&mdev->graph_mutex);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l_enable_media_source);
 
@@ -213,8 +219,12 @@ void v4l_disable_media_source(struct video_device *vdev)
 {
 	struct media_device *mdev = vdev->entity.graph_obj.mdev;
 
-	if (mdev && mdev->disable_source)
-		mdev->disable_source(&vdev->entity);
+	if (mdev) {
+		mutex_lock(&mdev->graph_mutex);
+		if (mdev->disable_source)
+			mdev->disable_source(&vdev->entity);
+		mutex_unlock(&mdev->graph_mutex);
+	}
 }
 EXPORT_SYMBOL_GPL(v4l_disable_media_source);
 
-- 
2.7.4

Re: [PATCH 0/2] media protect enable and disable source handler paths

[Mauro Carvalho Chehab]

Em Mon, 28 Nov 2016 19:15:12 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> These two patches fix enable and disable source handler paths. These
> aren't dependent patches, grouped because they fix similar problems.

Those two patches should be fold, as applying just the first patch
would cause au0828 to try to double lock.

> 
> This work is triggered by a review comment from Mauro Chehab on a
> snd_usb_audio patch about protecting the enable and disabel handler
> path in it.
> 
> Ran tests to make sure enable and disable handler paths work. When
> digital stream is active, analog app finds the tuner busy and vice
> versa. Also ran the Sakari's unbind while video stream is active test.

Sorry, but your patches descriptions don't make things clear:

- It doesn't present any OOPS or logs that would help to
  understand what you're trying to fix;

- From what I understood, you're moving the lock out of
  enable/disable handlers, and letting their callers to do
  the locks themselves. Why? Are there any condition where it
  won't need to be locked?

- It is not touching documentation. If now the callbacks should
  not implement locks, this should be explicitly described.

Btw, I think it is a bad idea to let the callers to handle
the locks. The best would be, instead, to change the code in
some other way to avoid it, if possible. If not possible at all,
clearly describe why it is not possible and insert some comments
inside the code, to avoid some cleanup patch to mess up with this.

Regards

Thanks,
Mauro

Re: [PATCH 2/2] media: protect enable and disable source handler checks and calls

[Sakari Ailus]

Hi Shuah,

On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote:
> Protect enable and disable source handler checks and calls from dvb-core
> and v4l2-core. Hold graph_mutex to check if enable and disable source
> handlers are present and invoke them while holding the mutex. This change
> ensures these handlers will not be removed while they are being checked
> and invoked.
> 
> au08282 enable and disable source handlers are changed to not hold the
> graph_mutex.
> 
> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>  drivers/media/dvb-core/dvb_frontend.c  | 24 ++++++++++++++++++------
>  drivers/media/usb/au0828/au0828-core.c | 17 +++++------------
>  drivers/media/v4l2-core/v4l2-mc.c      | 26 ++++++++++++++++++--------
>  3 files changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index 01511e5..2f09c7e 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>  		fepriv->voltage = -1;
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
> -		if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
> -			ret = fe->dvb->mdev->enable_source(dvbdev->entity,
> +		if (fe->dvb->mdev) {
> +			mutex_lock(&fe->dvb->mdev->graph_mutex);
> +			if (fe->dvb->mdev->enable_source)
> +				ret = fe->dvb->mdev->enable_source(
> +							   dvbdev->entity,
>  							   &fepriv->pipe);
> +			mutex_unlock(&fe->dvb->mdev->graph_mutex);

You have to make sure the media device actually will stay aronud while it is
being accessed. In this case, when dvb_frontend_open() runs, it will proceed
to access the media device without knowing whether it's going to stay around
or not. Without doing so, it may well be in the process of being removed by
au0828_unregister_media_device() at the same time.

The approach I took in my patchset was that the device that requires the
media device will acquire a reference to it, this way the media device will
stick around as long as other data structures have references to it. The
current set did not yet implement this to dvb devices but I can add that.
Then there's no even a need for the frontend driver to acquire the graph
lock just to call the enable_source() callback.

>  			if (ret) {
>  				dev_err(fe->dvb->device,
>  					"Tuner is busy. Error %d\n", ret);
> @@ -2553,8 +2557,12 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>  
>  err3:
>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
> -	if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
> -		fe->dvb->mdev->disable_source(dvbdev->entity);
> +	if (fe->dvb->mdev) {
> +		mutex_lock(&fe->dvb->mdev->graph_mutex);
> +		if (fe->dvb->mdev->disable_source)
> +			fe->dvb->mdev->disable_source(dvbdev->entity);
> +		mutex_unlock(&fe->dvb->mdev->graph_mutex);
> +	}
>  err2:
>  #endif
>  	dvb_generic_release(inode, file);
> @@ -2586,8 +2594,12 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
>  	if (dvbdev->users == -1) {
>  		wake_up(&fepriv->wait_queue);
>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
> -		if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
> -			fe->dvb->mdev->disable_source(dvbdev->entity);
> +		if (fe->dvb->mdev) {
> +			mutex_lock(&fe->dvb->mdev->graph_mutex);
> +			if (fe->dvb->mdev->disable_source)
> +				fe->dvb->mdev->disable_source(dvbdev->entity);
> +			mutex_unlock(&fe->dvb->mdev->graph_mutex);
> +		}
>  #endif
>  		if (fe->exit != DVB_FE_NO_EXIT)
>  			wake_up(&dvbdev->wait_queue);
> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> index a1f696a..bfd6482 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -280,6 +280,7 @@ static void au0828_media_graph_notify(struct media_entity *new,
>  	}
>  }
>  
> +/* Callers should hold graph_mutex */
>  static int au0828_enable_source(struct media_entity *entity,
>  				struct media_pipeline *pipe)
>  {
> @@ -293,8 +294,6 @@ static int au0828_enable_source(struct media_entity *entity,
>  	if (!mdev)
>  		return -ENODEV;
>  
> -	mutex_lock(&mdev->graph_mutex);
> -
>  	dev = mdev->source_priv;
>  
>  	/*
> @@ -421,12 +420,12 @@ static int au0828_enable_source(struct media_entity *entity,
>  		 dev->active_source->name, dev->active_sink->name,
>  		 dev->active_link_owner->name, ret);
>  end:
> -	mutex_unlock(&mdev->graph_mutex);
>  	pr_debug("au0828_enable_source() end %s %d %d\n",
>  		 entity->name, entity->function, ret);
>  	return ret;
>  }
>  
> +/* Callers should hold graph_mutex */
>  static void au0828_disable_source(struct media_entity *entity)
>  {
>  	int ret = 0;
> @@ -436,13 +435,10 @@ static void au0828_disable_source(struct media_entity *entity)
>  	if (!mdev)
>  		return;
>  
> -	mutex_lock(&mdev->graph_mutex);
>  	dev = mdev->source_priv;
>  
> -	if (!dev->active_link) {
> -		ret = -ENODEV;
> -		goto end;
> -	}
> +	if (!dev->active_link)
> +		return;
>  
>  	/* link is active - stop pipeline from source (tuner) */
>  	if (dev->active_link->sink->entity == dev->active_sink &&
> @@ -452,7 +448,7 @@ static void au0828_disable_source(struct media_entity *entity)
>  		 * has active pipeline
>  		*/
>  		if (dev->active_link_owner != entity)
> -			goto end;
> +			return;
>  		__media_entity_pipeline_stop(entity);
>  		ret = __media_entity_setup_link(dev->active_link, 0);
>  		if (ret)
> @@ -467,9 +463,6 @@ static void au0828_disable_source(struct media_entity *entity)
>  		dev->active_source = NULL;
>  		dev->active_sink = NULL;
>  	}
> -
> -end:
> -	mutex_unlock(&mdev->graph_mutex);
>  }
>  #endif
>  
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index 8bef433..b169d24 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -198,14 +198,20 @@ EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph);
>  int v4l_enable_media_source(struct video_device *vdev)
>  {
>  	struct media_device *mdev = vdev->entity.graph_obj.mdev;
> -	int ret;
> +	int ret = 0, err;
>  
> -	if (!mdev || !mdev->enable_source)
> +	if (!mdev)
>  		return 0;
> -	ret = mdev->enable_source(&vdev->entity, &vdev->pipe);
> -	if (ret)
> -		return -EBUSY;
> -	return 0;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +	if (!mdev->enable_source)
> +		goto end;
> +	err = mdev->enable_source(&vdev->entity, &vdev->pipe);
> +	if (err)
> +		ret = -EBUSY;
> +end:
> +	mutex_unlock(&mdev->graph_mutex);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(v4l_enable_media_source);
>  
> @@ -213,8 +219,12 @@ void v4l_disable_media_source(struct video_device *vdev)
>  {
>  	struct media_device *mdev = vdev->entity.graph_obj.mdev;
>  
> -	if (mdev && mdev->disable_source)
> -		mdev->disable_source(&vdev->entity);
> +	if (mdev) {
> +		mutex_lock(&mdev->graph_mutex);
> +		if (mdev->disable_source)
> +			mdev->disable_source(&vdev->entity);
> +		mutex_unlock(&mdev->graph_mutex);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(v4l_disable_media_source);
>  

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 0/2] media protect enable and disable source handler paths

[Shuah Khan]

On 11/29/2016 02:15 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 28 Nov 2016 19:15:12 -0700
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> These two patches fix enable and disable source handler paths. These
>> aren't dependent patches, grouped because they fix similar problems.
> 
> Those two patches should be fold, as applying just the first patch
> would cause au0828 to try to double lock.
> 

No it doesn't. The first patch holds the lock to just clear and set
enable disable source handlers and doesn't change any other paths.
The second patch removes lock hold from enable and disable source
handlers and the callers to hold the lock.

However, I can easily fold them together and not a problem.

>>
>> This work is triggered by a review comment from Mauro Chehab on a
>> snd_usb_audio patch about protecting the enable and disabel handler
>> path in it.
>>
>> Ran tests to make sure enable and disable handler paths work. When
>> digital stream is active, analog app finds the tuner busy and vice
>> versa. Also ran the Sakari's unbind while video stream is active test.
> 
> Sorry, but your patches descriptions don't make things clear:

Right. I should have explained it better.

> 
> - It doesn't present any OOPS or logs that would help to
>   understand what you're trying to fix;
> 
> - From what I understood, you're moving the lock out of
>   enable/disable handlers, and letting their callers to do
>   the locks themselves. Why? Are there any condition where it
>   won't need to be locked?

So here is the scenario these patches fix. Say user app starts
and during start of video streaming v4l2 checks to see if enable
source handler is defined. This check is done without holding the
graph_mutex. If unbind happens to be in progress, au0828 could
clear enable and disable source handlers. So these could race.
I am not how large this window is, but could happen.

If graph_mutex protects the check for enable source handler not
being null, then it has to be released before calling enable source
handler as shown below:

if (mdev) {
	mutex_lock(&mdev->graph_mutex);
	if (mdev->disable_source) {
		mutex_unlock(&mdev->graph_mutex);
		mdev->disable_source(&vdev->entity);
	} else
		mutex_unlock(&mdev->graph_mutex);
}

The above will leave another window for handlers to be cleared.
That is why it would make sense for the caller to hold the lock
and the call enable and disable source handlers.

> 
> - It is not touching documentation. If now the callbacks should
>   not implement locks, this should be explicitly described.

Yes documentation needs to be updated and I can do that in v2 if
we are okay with this approach.

> 
> Btw, I think it is a bad idea to let the callers to handle
> the locks. The best would be, instead, to change the code in
> some other way to avoid it, if possible. If not possible at all,
> clearly describe why it is not possible and insert some comments
> inside the code, to avoid some cleanup patch to mess up with this.
> 

Hope the above explanation helps answer the question. We do need a
way to protect enable and disable handler access and the call itself.
I am using the same graph_mutex for both, hence I decided to have the
caller hold the lock. Any other ideas welcome.

thanks,
-- Shuah

Re: [PATCH 0/2] media protect enable and disable source handler paths

[Mauro Carvalho Chehab]

Em Tue, 29 Nov 2016 10:07:21 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 11/29/2016 02:15 AM, Mauro Carvalho Chehab wrote:
> > Em Mon, 28 Nov 2016 19:15:12 -0700
> > Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> >   
> >> These two patches fix enable and disable source handler paths. These
> >> aren't dependent patches, grouped because they fix similar problems.  
> > 
> > Those two patches should be fold, as applying just the first patch
> > would cause au0828 to try to double lock.
> >   
> 
> No it doesn't. The first patch holds the lock to just clear and set
> enable disable source handlers and doesn't change any other paths.
> The second patch removes lock hold from enable and disable source
> handlers and the callers to hold the lock.
> 
> However, I can easily fold them together and not a problem.
> 
> >>
> >> This work is triggered by a review comment from Mauro Chehab on a
> >> snd_usb_audio patch about protecting the enable and disabel handler
> >> path in it.
> >>
> >> Ran tests to make sure enable and disable handler paths work. When
> >> digital stream is active, analog app finds the tuner busy and vice
> >> versa. Also ran the Sakari's unbind while video stream is active test.  
> > 
> > Sorry, but your patches descriptions don't make things clear:  
> 
> Right. I should have explained it better.
> 
> > 
> > - It doesn't present any OOPS or logs that would help to
> >   understand what you're trying to fix;
> > 
> > - From what I understood, you're moving the lock out of
> >   enable/disable handlers, and letting their callers to do
> >   the locks themselves. Why? Are there any condition where it
> >   won't need to be locked?  
> 
> So here is the scenario these patches fix. Say user app starts
> and during start of video streaming v4l2 checks to see if enable
> source handler is defined. This check is done without holding the
> graph_mutex.

Why? You need to hold the graph_mutex before navigating at the
media graph, or to convert MC to use a lockless protection (like 
RCU or restartable sequence).

> If unbind happens to be in progress, au0828 could
> clear enable and disable source handlers. So these could race.
> I am not how large this window is, but could happen.
> 
> If graph_mutex protects the check for enable source handler not
> being null, then it has to be released before calling enable source
> handler as shown below:
> 
> if (mdev) {
> 	mutex_lock(&mdev->graph_mutex);
> 	if (mdev->disable_source) {
> 		mutex_unlock(&mdev->graph_mutex);
> 		mdev->disable_source(&vdev->entity);
> 	} else
> 		mutex_unlock(&mdev->graph_mutex);
> }
> 
> The above will leave another window for handlers to be cleared.
> That is why it would make sense for the caller to hold the lock
> and the call enable and disable source handlers.

I see. Please add such explanation at the patch description,
showing how it should happen, instead.
> 
> > 
> > - It is not touching documentation. If now the callbacks should
> >   not implement locks, this should be explicitly described.  
> 
> Yes documentation needs to be updated and I can do that in v2 if
> we are okay with this approach.
> 
> > 
> > Btw, I think it is a bad idea to let the callers to handle
> > the locks. The best would be, instead, to change the code in
> > some other way to avoid it, if possible. If not possible at all,
> > clearly describe why it is not possible and insert some comments
> > inside the code, to avoid some cleanup patch to mess up with this.
> >   
> 
> Hope the above explanation helps answer the question. We do need a
> way to protect enable and disable handler access and the call itself.
> I am using the same graph_mutex for both, hence I decided to have the
> caller hold the lock. Any other ideas welcome.
> 
> thanks,
> -- Shuah
> 



Thanks,
Mauro

Re: [PATCH 2/2] media: protect enable and disable source handler checks and calls

[Shuah Khan]

On 11/29/2016 02:22 AM, Sakari Ailus wrote:
> Hi Shuah,
> 
> On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote:
>> Protect enable and disable source handler checks and calls from dvb-core
>> and v4l2-core. Hold graph_mutex to check if enable and disable source
>> handlers are present and invoke them while holding the mutex. This change
>> ensures these handlers will not be removed while they are being checked
>> and invoked.
>>
>> au08282 enable and disable source handlers are changed to not hold the
>> graph_mutex.
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/dvb-core/dvb_frontend.c  | 24 ++++++++++++++++++------
>>  drivers/media/usb/au0828/au0828-core.c | 17 +++++------------
>>  drivers/media/v4l2-core/v4l2-mc.c      | 26 ++++++++++++++++++--------
>>  3 files changed, 41 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
>> index 01511e5..2f09c7e 100644
>> --- a/drivers/media/dvb-core/dvb_frontend.c
>> +++ b/drivers/media/dvb-core/dvb_frontend.c
>> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>>  		fepriv->voltage = -1;
>>  
>>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> -		if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
>> -			ret = fe->dvb->mdev->enable_source(dvbdev->entity,
>> +		if (fe->dvb->mdev) {
>> +			mutex_lock(&fe->dvb->mdev->graph_mutex);
>> +			if (fe->dvb->mdev->enable_source)
>> +				ret = fe->dvb->mdev->enable_source(
>> +							   dvbdev->entity,
>>  							   &fepriv->pipe);
>> +			mutex_unlock(&fe->dvb->mdev->graph_mutex);
> 
> You have to make sure the media device actually will stay aronud while it is
> being accessed. In this case, when dvb_frontend_open() runs, it will proceed
> to access the media device without knowing whether it's going to stay around
> or not. Without doing so, it may well be in the process of being removed by
> au0828_unregister_media_device() at the same time.

Right. What this is trying to protect is just the check for enable_source
and disable handlers before calling them.

> 
> The approach I took in my patchset was that the device that requires the
> media device will acquire a reference to it, this way the media device will
> stick around as long as other data structures have references to it. The
> current set did not yet implement this to dvb devices but I can add that.
> Then there's no even a need for the frontend driver to acquire the graph
> lock just to call the enable_source() callback.

Taking reference to media_device alone will not solve this problem of enable
and disable handlers going away. au0828_unregister_media_device() will clear
the handlers and then call media_device_unregister() and it also does
media_device_cleanup(). Your patch set and media dev allocator api I did solve
the problem of media_device not going away, however, it doesn't fix this race
where callers of enable and disable source handlers checking for them and calling
them while the driver might be clearing them.

So here is the scenario these patches fix. Say user app starts
and during start of video streaming v4l2 checks to see if enable
source handler is defined. This check is done without holding the
graph_mutex. If unbind happens to be in progress, au0828 could
clear enable and disable source handlers. So these could race.
I am not how large this window is, but could happen.

If graph_mutex protects the check for enable source handler not
being null, then it has to be released before calling enable source
handler as shown below:

if (mdev) {
	mutex_lock(&mdev->graph_mutex);
	if (mdev->disable_source) {
		mutex_unlock(&mdev->graph_mutex);
		mdev->disable_source(&vdev->entity);
	} else
		mutex_unlock(&mdev->graph_mutex);
}

The above will leave another window for handlers to be cleared.
That is why it would make sense for the caller to hold the lock
and the call enable and disable source handlers.

We do need a way to protect enable and disable handler access and the
call itself. I am using the same graph_mutex for both, hence I decided
to have the caller hold the lock.

Hope this helps.

thanks,
-- Shuah

Re: [PATCH 2/2] media: protect enable and disable source handler checks and calls

[Sakari Ailus]

Hi Shuah,

On Tue, Nov 29, 2016 at 10:41:51AM -0700, Shuah Khan wrote:
> On 11/29/2016 02:22 AM, Sakari Ailus wrote:
> > Hi Shuah,
> > 
> > On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote:
> >> Protect enable and disable source handler checks and calls from dvb-core
> >> and v4l2-core. Hold graph_mutex to check if enable and disable source
> >> handlers are present and invoke them while holding the mutex. This change
> >> ensures these handlers will not be removed while they are being checked
> >> and invoked.
> >>
> >> au08282 enable and disable source handlers are changed to not hold the
> >> graph_mutex.
> >>
> >> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> >> ---
> >>  drivers/media/dvb-core/dvb_frontend.c  | 24 ++++++++++++++++++------
> >>  drivers/media/usb/au0828/au0828-core.c | 17 +++++------------
> >>  drivers/media/v4l2-core/v4l2-mc.c      | 26 ++++++++++++++++++--------
> >>  3 files changed, 41 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> >> index 01511e5..2f09c7e 100644
> >> --- a/drivers/media/dvb-core/dvb_frontend.c
> >> +++ b/drivers/media/dvb-core/dvb_frontend.c
> >> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
> >>  		fepriv->voltage = -1;
> >>  
> >>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
> >> -		if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
> >> -			ret = fe->dvb->mdev->enable_source(dvbdev->entity,
> >> +		if (fe->dvb->mdev) {
> >> +			mutex_lock(&fe->dvb->mdev->graph_mutex);
> >> +			if (fe->dvb->mdev->enable_source)
> >> +				ret = fe->dvb->mdev->enable_source(
> >> +							   dvbdev->entity,
> >>  							   &fepriv->pipe);
> >> +			mutex_unlock(&fe->dvb->mdev->graph_mutex);
> > 
> > You have to make sure the media device actually will stay aronud while it is
> > being accessed. In this case, when dvb_frontend_open() runs, it will proceed
> > to access the media device without knowing whether it's going to stay around
> > or not. Without doing so, it may well be in the process of being removed by
> > au0828_unregister_media_device() at the same time.
> 
> Right. What this is trying to protect is just the check for enable_source
> and disable handlers before calling them.

Yes, but that's not enough.

The other handlers in the ops structure must stay there as long as the media
device does. So we need to make sure it does. One, perhaps the only way to
do that could be to obtain a reference to the device that first set those
callbacks.

> 
> > 
> > The approach I took in my patchset was that the device that requires the
> > media device will acquire a reference to it, this way the media device will
> > stick around as long as other data structures have references to it. The
> > current set did not yet implement this to dvb devices but I can add that.
> > Then there's no even a need for the frontend driver to acquire the graph
> > lock just to call the enable_source() callback.
> 
> Taking reference to media_device alone will not solve this problem of enable
> and disable handlers going away. au0828_unregister_media_device() will clear
> the handlers and then call media_device_unregister() and it also does
> media_device_cleanup(). Your patch set and media dev allocator api I did solve

Then, that should be applied to all the other callbacks in the ops structure
as well. Not only to the callbacks that the au0828 driver needs. All the
callbacks are really need to stay unchanged as long as the device may be in
use.

Acquiring the graph mutex is hardly a workable solution to fix this.

> the problem of media_device not going away, however, it doesn't fix this race
> where callers of enable and disable source handlers checking for them and calling
> them while the driver might be clearing them.
> 
> So here is the scenario these patches fix. Say user app starts
> and during start of video streaming v4l2 checks to see if enable
> source handler is defined. This check is done without holding the
> graph_mutex. If unbind happens to be in progress, au0828 could
> clear enable and disable source handlers. So these could race.
> I am not how large this window is, but could happen.
> 
> If graph_mutex protects the check for enable source handler not
> being null, then it has to be released before calling enable source
> handler as shown below:
> 
> if (mdev) {
> 	mutex_lock(&mdev->graph_mutex);
> 	if (mdev->disable_source) {
> 		mutex_unlock(&mdev->graph_mutex);
> 		mdev->disable_source(&vdev->entity);
> 	} else
> 		mutex_unlock(&mdev->graph_mutex);
> }
> 
> The above will leave another window for handlers to be cleared.
> That is why it would make sense for the caller to hold the lock
> and the call enable and disable source handlers.
> 
> We do need a way to protect enable and disable handler access and the
> call itself. I am using the same graph_mutex for both, hence I decided
> to have the caller hold the lock.
> 
> Hope this helps.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 2/2] media: protect enable and disable source handler checks and calls

[Shuah Khan]

Hi Sakari,

On 12/01/2016 06:51 AM, Sakari Ailus wrote:
> Hi Shuah,
> 
> On Tue, Nov 29, 2016 at 10:41:51AM -0700, Shuah Khan wrote:
>> On 11/29/2016 02:22 AM, Sakari Ailus wrote:
>>> Hi Shuah,
>>>
>>> On Mon, Nov 28, 2016 at 07:15:14PM -0700, Shuah Khan wrote:
>>>> Protect enable and disable source handler checks and calls from dvb-core
>>>> and v4l2-core. Hold graph_mutex to check if enable and disable source
>>>> handlers are present and invoke them while holding the mutex. This change
>>>> ensures these handlers will not be removed while they are being checked
>>>> and invoked.
>>>>
>>>> au08282 enable and disable source handlers are changed to not hold the
>>>> graph_mutex.
>>>>
>>>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>>>> ---
>>>>  drivers/media/dvb-core/dvb_frontend.c  | 24 ++++++++++++++++++------
>>>>  drivers/media/usb/au0828/au0828-core.c | 17 +++++------------
>>>>  drivers/media/v4l2-core/v4l2-mc.c      | 26 ++++++++++++++++++--------
>>>>  3 files changed, 41 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
>>>> index 01511e5..2f09c7e 100644
>>>> --- a/drivers/media/dvb-core/dvb_frontend.c
>>>> +++ b/drivers/media/dvb-core/dvb_frontend.c
>>>> @@ -2527,9 +2527,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>>>>  		fepriv->voltage = -1;
>>>>  
>>>>  #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>>>> -		if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
>>>> -			ret = fe->dvb->mdev->enable_source(dvbdev->entity,
>>>> +		if (fe->dvb->mdev) {
>>>> +			mutex_lock(&fe->dvb->mdev->graph_mutex);
>>>> +			if (fe->dvb->mdev->enable_source)
>>>> +				ret = fe->dvb->mdev->enable_source(
>>>> +							   dvbdev->entity,
>>>>  							   &fepriv->pipe);
>>>> +			mutex_unlock(&fe->dvb->mdev->graph_mutex);
>>>
>>> You have to make sure the media device actually will stay aronud while it is
>>> being accessed. In this case, when dvb_frontend_open() runs, it will proceed
>>> to access the media device without knowing whether it's going to stay around
>>> or not. Without doing so, it may well be in the process of being removed by
>>> au0828_unregister_media_device() at the same time.
>>
>> Right. What this is trying to protect is just the check for enable_source
>> and disable handlers before calling them.
> 
> Yes, but that's not enough.
> 
> The other handlers in the ops structure must stay there as long as the media
> device does. So we need to make sure it does. One, perhaps the only way to
> do that could be to obtain a reference to the device that first set those
> callbacks.
> 
>>
>>>
>>> The approach I took in my patchset was that the device that requires the
>>> media device will acquire a reference to it, this way the media device will
>>> stick around as long as other data structures have references to it. The
>>> current set did not yet implement this to dvb devices but I can add that.
>>> Then there's no even a need for the frontend driver to acquire the graph
>>> lock just to call the enable_source() callback.
>>
>> Taking reference to media_device alone will not solve this problem of enable
>> and disable handlers going away. au0828_unregister_media_device() will clear
>> the handlers and then call media_device_unregister() and it also does
>> media_device_cleanup(). Your patch set and media dev allocator api I did solve
> 
> Then, that should be applied to all the other callbacks in the ops structure
> as well. Not only to the callbacks that the au0828 driver needs. All the
> callbacks are really need to stay unchanged as long as the device may be in
> use.

I agree with you that media_device should stick around until all the users go
away. That is what the Media Dev Allocator API patch series does. When the
media_device shared across two drivers, We are looking at two different lifetimes.

media_device lufetimes (starts when the first driver creates it)

These handlers (enable_source and disable_source) are tied to au02828
driver (bridge driver) lifetime, not the media_device lifetime.

Hence, it is important for au0828 to clear them from the media_device when
au0828 is going away via unbind, so these become invalid and don't get run.
This similar to when the driver (au0828) goes away, it removes its graph
nodes. Hence, it makes sense to use graph_mutex for this part of cleanup
just like when a media graph node is deleted and/or added.

I think you might be thinking about a simpler scenario where media_device
lifetime is same as the driver lifetime + until the last app. released
media_device reference. When you have two drivers ion mix, we also have
lifetimes for graph nodes each driver owns as well as some handlers bridge
driver provides to manage the graph nodes. Here is a visual:

bridge driver:
    creates media_device (gets reference to it when it allocates)
    registers it with its graph
    adds enable_source and disable_source handlers

second driver:
    finds media_device and gets reference to it
    adds its graph

bridge driver and second driver share access to resources
using enable_source and disable_source handlers

bridge driver unbind:
     clears enable_source and disable_source handlers - when
     this driver unbinds, enable_source and disable_source
     handler lifetime ends
     gives up media_device reference

second driver unbind:
     gives up media_device reference and no media_device
     gets unregistered and released if there are no other
     holds such as an application keeping media device open.

In summary we have media_device lifetime itself and the lifetime
of individual graph nodes and ops provided by one of the drivers.

This patch and the media dev allocator patch series I sent out handle
all of the above cases as tested on 4.9-rc7 base.

thanks,
-- Shuah

> 
> Acquiring the graph mutex is hardly a workable solution to fix this.
> 
>> the problem of media_device not going away, however, it doesn't fix this race
>> where callers of enable and disable source handlers checking for them and calling
>> them while the driver might be clearing them.
>>
>> So here is the scenario these patches fix. Say user app starts
>> and during start of video streaming v4l2 checks to see if enable
>> source handler is defined. This check is done without holding the
>> graph_mutex. If unbind happens to be in progress, au0828 could
>> clear enable and disable source handlers. So these could race.
>> I am not how large this window is, but could happen.
>>
>> If graph_mutex protects the check for enable source handler not
>> being null, then it has to be released before calling enable source
>> handler as shown below:
>>
>> if (mdev) {
>> 	mutex_lock(&mdev->graph_mutex);
>> 	if (mdev->disable_source) {
>> 		mutex_unlock(&mdev->graph_mutex);
>> 		mdev->disable_source(&vdev->entity);
>> 	} else
>> 		mutex_unlock(&mdev->graph_mutex);
>> }
>>
>> The above will leave another window for handlers to be cleared.
>> That is why it would make sense for the caller to hold the lock
>> and the call enable and disable source handlers.
>>
>> We do need a way to protect enable and disable handler access and the
>> call itself. I am using the same graph_mutex for both, hence I decided
>> to have the caller hold the lock.
>>
>> Hope this helps.
>